Merge pull request #4082 from atom/web-contents-lifetime

Fix some junk output when running tests

atom/browser/api/atom_api_web_contents.cc

@@ -280,11 +280,18 @@ WebContents::WebContents(v8::Isolate* isolate,
 }
 
 WebContents::~WebContents() {
-  // The webview's lifetime is completely controlled by GuestViewManager, so
+  // The destroy() is called.
-  // it is always destroyed by calling webview.destroy(), we need to make
+  if (managed_web_contents()) {
-  // sure the "destroyed" event is emitted manually.
+    // For webview we need to tell content module to do some cleanup work before
-  if (type_ == WEB_VIEW && managed_web_contents())
+    // destroying it.
+    if (type_ == WEB_VIEW)
+      guest_delegate_->Destroy();
+
+    // The WebContentsDestroyed will not be called automatically because we
+    // unsubscribe from webContents before destroying it. So we have to manually
+    // call it here to make sure "destroyed" event is emitted.
     WebContentsDestroyed();
+  }
 }
 
 bool WebContents::AddMessageToConsole(content::WebContents* source,
@@ -617,6 +624,18 @@ bool WebContents::OnMessageReceived(const IPC::Message& message) {
   return handled;
 }
 
+// There are three ways of destroying a webContents:
+// 1. call webContents.destory();
+// 2. garbage collection;
+// 3. user closes the window of webContents;
+// For webview only #1 will happen, for BrowserWindow both #1 and #3 may
+// happen. The #2 should never happen for webContents, because webview is
+// managed by GuestViewManager, and BrowserWindow's webContents is managed
+// by api::Window.
+// For #1, the destructor will do the cleanup work and we only need to make
+// sure "destroyed" event is emitted. For #3, the content::WebContents will
+// be destroyed on close, and WebContentsDestroyed would be called for it, so
+// we need to make sure the api::WebContents is also deleted.
 void WebContents::WebContentsDestroyed() {
   // The RenderViewDeleted was not called when the WebContents is destroyed.
   RenderViewDeleted(web_contents()->GetRenderViewHost());
@@ -627,8 +646,6 @@ void WebContents::WebContentsDestroyed() {
 
   // Cleanup relationships with other parts.
   RemoveFromWeakMap();
-  if (type_ == WEB_VIEW)
-    guest_delegate_->Destroy();
 
   // We can not call Destroy here because we need to call Emit first, but we
   // also do not want any method to be used, so just mark as destroyed here.

atom/browser/api/lib/web-contents.js

@@ -70,6 +70,10 @@ let wrapWebContents = function(webContents) {
   var controller, method, name, ref1;
   webContents.__proto__ = EventEmitter.prototype;
 
+  // Every remote callback from renderer process would add a listenter to the
+  // render-view-deleted event, so ignore the listenters warning.
+  webContents.setMaxListeners(0);
+
   // WebContents::send(channel, args..)
   webContents.send = function() {
     var args, channel;

atom/browser/lib/rpc-server.js

@@ -1,3 +1,5 @@
+'use strict';
+
 const path = require('path');
 const electron = require('electron');
 const ipcMain = electron.ipcMain;
@@ -41,7 +43,7 @@ var valueToMeta = function(sender, value, optimizeSimpleObject) {
   }
 
   // Treat the arguments object as array.
-  if (meta.type === 'object' && (value.callee != null) && (value.length != null)) {
+  if (meta.type === 'object' && (value.hasOwnProperty('callee')) && (value.length != null)) {
     meta.type = 'array';
   }
   if (meta.type === 'array') {
@@ -113,7 +115,7 @@ var exceptionToMeta = function(error) {
 var unwrapArgs = function(sender, args) {
   var metaToValue;
   metaToValue = function(meta) {
-    var i, len, member, ref, rendererReleased, ret, returnValue;
+    var i, len, member, ref, rendererReleased, returnValue;
     switch (meta.type) {
       case 'value':
         return meta.value;
@@ -130,7 +132,7 @@ var unwrapArgs = function(sender, args) {
           then: metaToValue(meta.then)
         });
       case 'object':
-        ret = v8Util.createObjectWithName(meta.name);
+        let ret = v8Util.createObjectWithName(meta.name);
         ref = meta.members;
         for (i = 0, len = ref.length; i < len; i++) {
           member = ref[i];
@@ -147,31 +149,30 @@ var unwrapArgs = function(sender, args) {
         if (!sender.callbacks) {
           sender.callbacks = new IDWeakMap;
           sender.on('render-view-deleted', function() {
-            return sender.callbacks.clear();
+            return this.callbacks.clear();
           });
         }
-        if (sender.callbacks.has(meta.id)) {
+
+        if (sender.callbacks.has(meta.id))
           return sender.callbacks.get(meta.id);
-        }
+
+        // Prevent the callback from being called when its page is gone.
         rendererReleased = false;
-        objectsRegistry.once("clear-" + (sender.getId()), function() {
+        sender.once('render-view-deleted', function() {
-          return rendererReleased = true;
+          rendererReleased = true;
         });
-        ret = function() {
+
-          if (rendererReleased) {
+        let callIntoRenderer = function(...args) {
-            throw new Error("Attempting to call a function in a renderer window that has been closed or released. Function provided here: " + meta.location + ".");
+          if (rendererReleased)
-          }
+            throw new Error(`Attempting to call a function in a renderer window that has been closed or released. Function provided here: ${meta.location}.`);
-          return sender.send('ATOM_RENDERER_CALLBACK', meta.id, valueToMeta(sender, arguments));
+          sender.send('ATOM_RENDERER_CALLBACK', meta.id, valueToMeta(sender, args));
         };
-        v8Util.setDestructor(ret, function() {
+        v8Util.setDestructor(callIntoRenderer, function() {
-          if (rendererReleased) {
+          if (!rendererReleased)
-            return;
+            sender.send('ATOM_RENDERER_RELEASE_CALLBACK', meta.id);
-          }
-          sender.callbacks.remove(meta.id);
-          return sender.send('ATOM_RENDERER_RELEASE_CALLBACK', meta.id);
         });
-        sender.callbacks.set(meta.id, ret);
+        sender.callbacks.set(meta.id, callIntoRenderer);
-        return ret;
+        return callIntoRenderer;
       default:
         throw new TypeError("Unknown type: " + meta.type);
     }

spec/api-browser-window-spec.js

@@ -455,31 +455,39 @@ describe('browser-window module', function() {
       });
     });
   });
-  describe('save page', function() {
+
-    var savePageCssPath, savePageDir, savePageHtmlPath, savePageJsPath;
+  describe('savePage method', function() {
-    savePageDir = path.join(fixtures, 'save_page');
+    const savePageDir = path.join(fixtures, 'save_page');
-    savePageHtmlPath = path.join(savePageDir, 'save_page.html');
+    const savePageHtmlPath = path.join(savePageDir, 'save_page.html');
-    savePageJsPath = path.join(savePageDir, 'save_page_files', 'test.js');
+    const savePageJsPath = path.join(savePageDir, 'save_page_files', 'test.js');
-    savePageCssPath = path.join(savePageDir, 'save_page_files', 'test.css');
+    const savePageCssPath = path.join(savePageDir, 'save_page_files', 'test.css');
-    return it('should save page', function(done) {
+
+    after(function() {
+      try {
+        fs.unlinkSync(savePageCssPath);
+        fs.unlinkSync(savePageJsPath);
+        fs.unlinkSync(savePageHtmlPath);
+        fs.rmdirSync(path.join(savePageDir, 'save_page_files'));
+        fs.rmdirSync(savePageDir);
+      } catch (e) {
+      }
+    });
+
+    it('should save page to disk', function(done) {
       w.webContents.on('did-finish-load', function() {
-        return w.webContents.savePage(savePageHtmlPath, 'HTMLComplete', function(error) {
+        w.webContents.savePage(savePageHtmlPath, 'HTMLComplete', function(error) {
           assert.equal(error, null);
           assert(fs.existsSync(savePageHtmlPath));
           assert(fs.existsSync(savePageJsPath));
           assert(fs.existsSync(savePageCssPath));
-          fs.unlinkSync(savePageCssPath);
+          done();
-          fs.unlinkSync(savePageJsPath);
-          fs.unlinkSync(savePageHtmlPath);
-          fs.rmdirSync(path.join(savePageDir, 'save_page_files'));
-          fs.rmdirSync(savePageDir);
-          return done();
         });
       });
-      return w.loadURL("file://" + fixtures + "/pages/save_page/index.html");
+      w.loadURL("file://" + fixtures + "/pages/save_page/index.html");
     });
   });
-  return describe('BrowserWindow options argument is optional', function() {
+
+  describe('BrowserWindow options argument is optional', function() {
     return it('should create a window with default size (800x600)', function() {
       var size;
       w.destroy();

spec/api-session-spec.js

@@ -28,80 +28,51 @@ describe('session module', function() {
   afterEach(function() {
     return w.destroy();
   });
-  it('should get cookies', function(done) {
+
-    var server;
+  describe('session.cookies', function() {
-    server = http.createServer(function(req, res) {
+    it('should get cookies', function(done) {
-      res.setHeader('Set-Cookie', ['0=0']);
+      var server;
-      res.end('finished');
+      server = http.createServer(function(req, res) {
-      return server.close();
+        res.setHeader('Set-Cookie', ['0=0']);
-    });
+        res.end('finished');
-    return server.listen(0, '127.0.0.1', function() {
+        return server.close();
-      var port;
+      });
-      port = server.address().port;
+      return server.listen(0, '127.0.0.1', function() {
-      w.loadURL(url + ":" + port);
+        var port;
-      return w.webContents.on('did-finish-load', function() {
+        port = server.address().port;
-        return w.webContents.session.cookies.get({
+        w.loadURL(url + ":" + port);
-          url: url
+        return w.webContents.on('did-finish-load', function() {
-        }, function(error, list) {
+          return w.webContents.session.cookies.get({
-          var cookie, i, len;
+            url: url
-          if (error) {
+          }, function(error, list) {
-            return done(error);
+            var cookie, i, len;
-          }
+            if (error) {
-          for (i = 0, len = list.length; i < len; i++) {
+              return done(error);
-            cookie = list[i];
+            }
-            if (cookie.name === '0') {
+            for (i = 0, len = list.length; i < len; i++) {
-              if (cookie.value === '0') {
+              cookie = list[i];
-                return done();
+              if (cookie.name === '0') {
-              } else {
+                if (cookie.value === '0') {
-                return done("cookie value is " + cookie.value + " while expecting 0");
+                  return done();
+                } else {
+                  return done("cookie value is " + cookie.value + " while expecting 0");
+                }
               }
             }
-          }
+            return done('Can not find cookie');
-          return done('Can not find cookie');
+          });
         });
       });
     });
-  });
+    it('should over-write the existent cookie', function(done) {
-  it('should over-write the existent cookie', function(done) {
+      return session.defaultSession.cookies.set({
-    return session.defaultSession.cookies.set({
+        url: url,
-      url: url,
+        name: '1',
-      name: '1',
+        value: '1'
-      value: '1'
+      }, function(error) {
-    }, function(error) {
-      if (error) {
-        return done(error);
-      }
-      return session.defaultSession.cookies.get({
-        url: url
-      }, function(error, list) {
-        var cookie, i, len;
         if (error) {
           return done(error);
         }
-        for (i = 0, len = list.length; i < len; i++) {
-          cookie = list[i];
-          if (cookie.name === '1') {
-            if (cookie.value === '1') {
-              return done();
-            } else {
-              return done("cookie value is " + cookie.value + " while expecting 1");
-            }
-          }
-        }
-        return done('Can not find cookie');
-      });
-    });
-  });
-  it('should remove cookies', function(done) {
-    return session.defaultSession.cookies.set({
-      url: url,
-      name: '2',
-      value: '2'
-    }, function(error) {
-      if (error) {
-        return done(error);
-      }
-      return session.defaultSession.cookies.remove(url, '2', function() {
         return session.defaultSession.cookies.get({
           url: url
         }, function(error, list) {
@@ -111,15 +82,48 @@ describe('session module', function() {
           }
           for (i = 0, len = list.length; i < len; i++) {
             cookie = list[i];
-            if (cookie.name === '2') {
+            if (cookie.name === '1') {
-              return done('Cookie not deleted');
+              if (cookie.value === '1') {
+                return done();
+              } else {
+                return done("cookie value is " + cookie.value + " while expecting 1");
+              }
             }
           }
-          return done();
+          return done('Can not find cookie');
+        });
+      });
+    });
+    it('should remove cookies', function(done) {
+      return session.defaultSession.cookies.set({
+        url: url,
+        name: '2',
+        value: '2'
+      }, function(error) {
+        if (error) {
+          return done(error);
+        }
+        return session.defaultSession.cookies.remove(url, '2', function() {
+          return session.defaultSession.cookies.get({
+            url: url
+          }, function(error, list) {
+            var cookie, i, len;
+            if (error) {
+              return done(error);
+            }
+            for (i = 0, len = list.length; i < len; i++) {
+              cookie = list[i];
+              if (cookie.name === '2') {
+                return done('Cookie not deleted');
+              }
+            }
+            return done();
+          });
         });
       });
     });
   });
+
   describe('session.clearStorageData(options)', function() {
     fixtures = path.resolve(__dirname, 'fixtures');
     return it('clears localstorage data', function(done) {

spec/fixtures/pages/save_page/index.html

@@ -1,6 +1,6 @@
 <html>
 <script type="text/javascript" src="test.js"></script>
-<script type="text/javascript" src="test.css"></script>
+<link href="test.css" rel="stylesheet">
 <body>
 </body>
 </html>

spec/fixtures/pages/save_page/test.js

@@ -1 +1 @@
-console.log('save_page');
+// do nothing