Issue 3815 - Fix TabAllocator. Now it returns tab with initialized outerWindowID

lib/crawler.js

 /*
  * This Source Code is subject to the terms of the Mozilla Public License
  * version 2.0 (the "License"). You can obtain a copy of the License at
  * http://mozilla.org/MPL/2.0/.
  */
 
 /**
  * @module crawler
  */
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/Promise.jsm");

[Wladimir Palant, 2016/03/16 14:52:52]

Please remove this if you don't use Promise.defer() - we should be using the
built-in Promise class.

[sergei, 2016/03/16 15:47:00]

Currently defer is still used by LoadListener.

 
 function abprequire(module)
 {
   let result = {};
   result.wrappedJSObject = result;
   Services.obs.notifyObservers(result, "adblockplus-require", module);
   return result.exports;
 }
 
 let {RequestNotifier} = abprequire("requestNotifier");
 let {FilterNotifier} = abprequire("filterNotifier");
 let {FilterStorage} = abprequire("filterStorage");
 
 /**
- * Allocates a new tab "about:blank" in the `browser`.

[Wladimir Palant, 2016/03/16 14:52:52]

Please don't use backticks, these have no special meaning for JSDoc. Also,
'Allocates a new tab "about:blank"' => "Creates a blank tab"

[sergei, 2016/03/16 15:47:00]

Done.

- * The method returns a {Promise} promise which is resolved with the `tab`

[Wladimir Palant, 2016/03/16 14:52:52]

"{Promise} promise" => "promise" please, the type doesn't add any information.

[sergei, 2016/03/16 15:47:00]

Done.

- * when `outerWindowID` is already initialized.

[Wladimir Palant, 2016/03/16 14:52:52]

Too many implementation details here. "which is resolved with the tab when
`outerWindowID` is already initialized" => "which resolves once the tab is fully
initialized." Besides, why do you describe the return value twice? That text
belongs under @return below.

[sergei, 2016/03/16 15:47:00]

Done.

- *
- * See: https://bugzilla.mozilla.org/show_bug.cgi?id=1256602#c1

[Wladimir Palant, 2016/03/16 14:52:52]

This belongs into a comment above the actual implementation, not into JSDoc.

[sergei, 2016/03/16 15:47:00]

Done.

- *
- * @param {tabbrowser} browser
- *    The tabbed browser where tabs should be created

[Wladimir Palant, 2016/03/16 14:52:52]

"tabs" => "the tab"

[sergei, 2016/03/16 15:47:01]

Done.

- * @return {Promise} promise which will be resolved with the tab. 

[Wladimir Palant, 2016/03/16 14:52:52]

Please use the more specific Promise.<tab> as type here.

[sergei, 2016/03/16 15:47:00]

Done.

- */
-function createTab(browser)
-{
-  let tab = browser.addTab("about:blank");
-  if (tab.linkedBrowser.outerWindowID)
-    return Promise.resolve(tab);
-  return new Promise((resolve, reject)=>

[Wladimir Palant, 2016/03/16 14:52:52]

Nit: Space before => please.

[sergei, 2016/03/16 15:47:00]

Done.

-  {
-    let onBrowserInit = (msg) =>
-    {
-      tab.linkedBrowser.messageManager.removeMessageListener("Browser:Init", onBrowserInit);
-      resolve(tab);
-    };
-    tab.linkedBrowser.messageManager.addMessageListener("Browser:Init", onBrowserInit);
-  });
-}
-
-/**
- * Allocates tabs on request but not more than `maxtabs` at the same time.
+ * Allocates tabs on request but not more than maxtabs at the same time.
  *
  * @param {tabbrowser} browser
  *    The tabbed browser where tabs should be created
  * @param {int} maxtabs
  *    The maximum number of tabs to be allocated
  * @constructor
  */
 function TabAllocator(browser, maxtabs)
 {
   this._browser = browser;
   this._tabs = 0;
   this._maxtabs = maxtabs;
-  // the array of `resolve` functions of {Promise} promises returned by `getTab`.

[Wladimir Palant, 2016/03/16 14:52:52]

This is a code comment, it's intended to be read by humans. Please (please!)
don't use backticks or type annotations.

Nit: how about "The queue containing resolve functions of promises waiting for a
tab."

[sergei, 2016/03/16 15:47:01]

Sounds good!

+  // The queue containing resolve functions of promises waiting for a tab.
   this._resolvers = [];
-  // Keep at least one tab alive to prevent browser from closing of it self.

[Wladimir Palant, 2016/03/16 14:52:52]

"browser from closing of it self" => "the browser from closing itself"?

[sergei, 2016/03/16 15:47:00]

Done.

-  // That tab will be removed when the first tab is requested.
-  browser.removeAllTabsBut(browser.tabs[0])
+  // Keep at least one tab alive to prevent browser from closing itself.
+  this._tabKeepingWindowAlive = this._browser.tabs[0];
+  this._browser.removeAllTabsBut(this._tabKeepingWindowAlive);
 }
 TabAllocator.prototype = {
+  _removeTabKeepingWindowAlive: function()
+  {
+    if (!this._tabKeepingWindowAlive)
+      return;
+    this._browser.removeTab(this._tabKeepingWindowAlive);
+    delete this._tabKeepingWindowAlive;
+  },
+
+  /**
+   * Creates a blank tab in this._browser.
+   *
+   * @return {Promise.<tab>} promise which resolves once the tab is fully initialized.
+   */
+  _createTab: function()
+  {
+    this._tabs++;
+    let tab = this._browser.addTab("about:blank");
+    if (tab.linkedBrowser.outerWindowID)
+    {
+      this._removeTabKeepingWindowAlive();
+      return Promise.resolve(tab);
+    }
+    return new Promise((resolve, reject) =>
+    {
+      let onBrowserInit = (msg) =>
+      {
+        tab.linkedBrowser.messageManager.removeMessageListener("Browser:Init", onBrowserInit);
+        this._removeTabKeepingWindowAlive();
+        resolve(tab);
+      };
+      // "Browser:Init" message is sent once the browser is ready, see
+      // https://bugzil.la/1256602#c1
+      tab.linkedBrowser.messageManager.addMessageListener("Browser:Init", onBrowserInit);
+    });
+  },
+
   /**
    * Returns a promise that will resolve into a tab once a tab is allocated.
    * The tab cannot be used by other tasks until releaseTab() is called.
    *
-   * @result {Promise}

[Wladimir Palant, 2016/03/16 14:52:52]

Nit: Promise.<tab> here as well.

+   * @result {Promise.<tab>}
    */
   getTab: function()
   {
     if (this._tabs < this._maxtabs)
-    {
-      let tab = createTab(this._browser);
-      // close initial tab, we don't need it anymore.

[Wladimir Palant, 2016/03/16 14:52:52]

Nit: Please start sentences with a capital letter.

-      if (this._tabs == 0)
-        this._browser.removeTab(this._browser.tabs[0]);
-      this._tabs++;
-      return tab;
-    }
-    return new Promise((resolve, reject) =>
-    {
-      this._resolvers.push(resolve);
-    });
+      return this._createTab();
+    return new Promise((resolve, reject) => this._resolvers.push(resolve));
   },
 
   /**
    * Adds a tab back to the pool so that it can be used by other tasks.
    *
    * @param {tab} tab
    */
   releaseTab: function(tab)
   {
-    let browser = tab.parentNode.tabbrowser;
-    browser.removeTab(tab);
-
-    if (this._resolvers.length)
-      this._resolvers.shift()(createTab(this._browser));
+    // If we are about to close last tab don't close it immediately to keep
+    // the window alive. It will be closed when a new tab is created.
+    if (this._tabs > 1)
+      this._browser.removeTab(tab);
     else
-      this._tabs--;

[Wladimir Palant, 2016/03/16 14:52:52]

Now you have the problem that in the theoretical scenario where tabs are closed
faster than new allocation requests come in you will close the browser window
along the way (last tab removed).

[sergei, 2016/03/16 15:47:00]

Yes. However I think it should be addressed at another level. What about setting
browser.tabs.closeWindowWithLastTab to false? Do we need it and should it be in
that commit or should I revert it to the array of tabs?

[sergei, 2016/04/11 15:20:22]

I have addressed that issue.

BTW, having an array of tabs generates a lot of erros like

###!!! [Child][MessageChannel] Error:
(msgtype=0x2C0040,name=PBrowser::Msg___delete__) Channel closing: too late to
send/recv, messages will be lost
###!!! [Child][OnMaybeDequeueOne] Error: Channel closing: too late to send/recv,
messages will be lost
###!!! [Child][MessageChannel] Error:
(msgtype=0x480108,name=PContent::Msg_AsyncMessage) Channel closing: too late to
send/recv, messages will be lost

at the end. The current version with one tab still can generate it, but it seems
fast enough, so it happens not so often. It also sometimes generates something
like the snippet below. I guess it comes from the race condition that the
browser is begin closed and some parts are already destroyed and some are still
running.

*************************
A coding exception was thrown in a Promise resolution callback.
See
https://developer.mozilla.org/Mozilla/JavaScript_code_modules/Promise.jsm/Pro...

Full message: TypeError: this._removingTabs is undefined
Full stack: _beginRemoveTab@chrome://browser/content/tabbrowser.xml:2306:13
removeTab@chrome://browser/content/tabbrowser.xml:2209:18
TabAllocator.prototype.releaseTab@resource://gre/modules/addons/XPIProvider.jsm
->
jar:file:///tmp/tmpPxNRXz.mozrunner/extensions/abpcrawler@adblockplus.org.xpi!/bootstrap.js
->
jar:file:///tmp/tmpPxNRXz.mozrunner/extensions/abpcrawler@adblockplus.org.xpi!/lib/crawler.js:119:5
TabAllocator.prototype.releaseTab/this._tab<@resource://gre/modules/addons/XPIProvider.jsm
->
jar:file:///tmp/tmpPxNRXz.mozrunner/extensions/abpcrawler@adblockplus.org.xpi!/bootstrap.js
->
jar:file:///tmp/tmpPxNRXz.mozrunner/extensions/abpcrawler@adblockplus.org.xpi!/lib/crawler.js:113:9
Handler.prototype.process@resource://gre/modules/Promise.jsm ->
resource://gre/modules/Promise-backend.js:937:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm ->
resource://gre/modules/Promise-backend.js:816:7
Promise*this.PromiseWalker.scheduleWalkerLoop@resource://gre/modules/Promise.jsm
-> resource://gre/modules/Promise-backend.js:747:11
this.PromiseWalker.schedulePromise@resource://gre/modules/Promise.jsm ->
resource://gre/modules/Promise-backend.js:779:7
this.PromiseWalker.completePromise@resource://gre/modules/Promise.jsm ->
resource://gre/modules/Promise-backend.js:714:7
TabAllocator.prototype._createTab/</onBrowserInit@resource://gre/modules/addons/XPIProvider.jsm
->
jar:file:///tmp/tmpPxNRXz.mozrunner/extensions/abpcrawler@adblockplus.org.xpi!/bootstrap.js
->
jar:file:///tmp/tmpPxNRXz.mozrunner/extensions/abpcrawler@adblockplus.org.xpi!/lib/crawler.js:76:9

*************************

-  }
+    {
+      // navigate away from early opened URL
+      tab.linkedBrowser.loadURI('about:blank', null, null);
+      this._tabKeepingWindowAlive = tab;
+    }
+
+    this._tabs--;
+    if (this._resolvers.length && this._tabs < this._maxtabs)
+    {
+      this._resolvers.shift()(this._createTab());
+    }
+  },
 };
 
 /**
  * Observes page loads in a particular tabbed browser.
  *
  * @param {tabbrowser} browser
  *    The tabbed browser to be observed
  * @param {int} timeout
  *    Load timeout in milliseconds
  * @constructor
@@ skipping 131 matching lines @@
         resolve();
       }
     };
     FilterNotifier.addListener(onFiltersLoaded);
   }).then(() => crawl_urls(window, urls, timeout, maxtabs, targetURL, onDone))
   .catch(reportException);
 }
 exports.run = run;
 
 /**
- * Spawns a {Task} task to crawl each url from `urls` argument and calls
- * `onDone` when all tasks are finished.
+ * Spawns a {Task} task to crawl each url from urls argument and calls
+ * onDone when all tasks are finished.
  * @param {Window} window
  *   The browser window we're operating in
  * @param {String[]} urls
  *   URLs to be crawled
  * @param {int} timeout
  *    Load timeout in milliseconds
  * @param {int} maxtabs
  *    Maximum number of tabs to be opened
  * @param {String} targetURL
  *    URL that should receive the results
@@ skipping 110 matching lines @@
 
 function reportException(e)
 {
   let stack = "";
   if (e && typeof e == "object" && "stack" in e)
     stack = e.stack + "\n";
 
   Cu.reportError(e);
   dump(e + "\n" + stack + "\n");
 }