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");
 
 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");
 
 /**
- * Creates a pool of tabs and allocates them to tasks on request.
+ * 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)
 {
-  browser.removeAllTabsBut(browser.tabs[0])
-
-  this._tabs = [];
-  for (let i = 0; i < maxtabs; i++)
-    this._tabs.push(browser.addTab("about:blank"));
-
-  browser.removeTab(browser.tabs[0]);
-
-  this._deferred = [];
+  this._browser = browser;
+  this._tabs = 0;
+  this._maxtabs = maxtabs;
+  // The queue containing resolve functions of promises waiting for a tab.
+  this._resolvers = [];
+  // Keep at least one tab alive to prevent browser from closing itself.
+  let tabToRemove = this._browser.tabs[0];
+  this._browser.removeAllTabsBut(tabToRemove);
+  // this._tab is a keep alive tab
+  this._tab = this._createTab().then(tab =>
+  {
+    // Starting from Firefox 48 (nightly) the sequence of calls addTab and
+    // removeTab can cause a closing of the browser because a new tab is still
+    // not here. Because of that we need to remove the previous tab only after
+    // the new tab is ready.
+    this._browser.removeTab(tabToRemove);
+    return tab;
+  });

[Wladimir Palant, 2016/09/14 15:00:13]

There is little point pre-allocating a tab, you can just as well have
this._tabKeepingWindowAlive and remove that tab in _createTab before resolving.
You can also use the same code to get rid of the race condition on release -
just set this._tabKeepingWindowAlive instead of removing the last tab. You won't
need to mess with the bookkeeping then, tab count is updated either way -
currently verifying that the tab count is correct under all circumstances is
really complicated.

[sergei, 2016/09/15 15:33:27]

Done.

 }
 TabAllocator.prototype = {
   /**
-   * Returns a promise that will resolve into a tab once a tab can be allocated.
+   * 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)
+      return Promise.resolve(tab);
+    return new Promise((resolve, reject) =>
+    {
+      let onBrowserInit = (msg) =>
+      {
+        // https://bugzilla.mozilla.org/show_bug.cgi?id=1256602#c1

[Wladimir Palant, 2016/09/14 15:00:13]

Please don't use URL-only comments, it should be obvious what this is about. For
example:

// "Browser:Init" message is sent once the browser is ready, see
https://bugzil.la/1256602#c1

Also, this comment is misplaced, should be above the addMessageListener call.

[sergei, 2016/09/15 15:33:27]

Done.

+        tab.linkedBrowser.messageManager.removeMessageListener("Browser:Init", onBrowserInit);
+        resolve(tab);
+      };
+      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}
+   * @result {Promise.<tab>}
    */
   getTab: function()
   {
-    if (this._tabs.length)
-      return this._tabs.shift();
-    else
+    if (this._tab)
     {
-      let deferred = Promise.defer();
-      this._deferred.push(deferred);
-      return deferred.promise;
+      let tab = this._tab;
+      delete this._tab;
+      return tab;
     }
+    if (this._tabs < this._maxtabs)
+      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);
-    tab = browser.addTab("about:blank");
+    // If we are about to close last tab don't close it immediately rather
+    // allocate a new blank tab and close the current one afterwards.
+    if (this._tabs == 1)
+    {
+      this._tab = this._createTab().then((resultTab) =>
+      {
+        this.releaseTab(tab);
+        return resultTab;
+      });
+      return;
+    }
 
-    if (this._deferred.length)
-      this._deferred.shift().resolve(tab);
-    else
-      this._tabs.push(tab);
-  }
+    this._browser.removeTab(tab);
+    this._tabs--;
+    if (this._resolvers.length)
+    {
+      if (this._tab)
+      {
+        this._resolvers.shift()(this._tab);
+        delete this._tab;
+      }
+      else if (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");
 }