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

lib/crawler.js

Index: lib/crawler.js
diff --git a/lib/crawler.js b/lib/crawler.js
index 7bc28dbbe75187a1a752583c81d5c638a24c98ca..2e7051b8e6ff6ebb94d5f54ddf5e434d2ae246d1 100644
--- a/lib/crawler.js
+++ b/lib/crawler.js
@@ -25,7 +25,7 @@ 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
@@ -35,33 +35,64 @@ let {FilterStorage} = abprequire("filterStorage");
  */
 function TabAllocator(browser, maxtabs)
 {
-  browser.removeAllTabsBut(browser.tabs[0])
+  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.
+  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;
+  },
 
-  this._tabs = [];
-  for (let i = 0; i < maxtabs; i++)
-    this._tabs.push(browser.addTab("about:blank"));
+  /**
+   * 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);

[Wladimir Palant, 2016/09/16 07:10:37]

I think that rather than introducing a _removeTabKeepingWindowAlive() function I
would have added an nested function onReady():

  function onReady()
  {
    if (this._tabKeepingWindowAlive)
    {
      this._browser.removeTab(this._tabKeepingWindowAlive);
      delete this._tabKeepingWindowAlive;
    }
    resolve(tab);
  }

You can then call it when the tab is ready - regardless of whether that happens
synchronously. Means moving the tab.linkedBrowser.outerWindowID check inside the
promise of course. Up to you whether you want to do it this way but IMHO it
makes more sense to keep all related actions in one place.

[sergei, 2016/09/16 12:34:13]

It's a valid point but I would like to keep it this way because I find it
cleaner. There are some tiny things (bigger depth of callbacks calls, the
generalization is used merely in two places, it would be good to put the first
part of onReady in a separate named function _removeTabKeepingWindowAlive, the
heaviness of creation of functions on each call of createTab), whose
contribution seems negligible in the current context but I would like to avoid
it. In addition, I would like to have a simple API (it's better to have more
small functions with less responsibility than a few functions which do a lot),
dealing with that keeping alive tab is actually not related to createTab
function, so I thought rather about the following
_createTab()
{
  const tab = this._allocateTab(); // where _allocateTab is _createTab without
_removeTabKeepingWindowAlive.
  tab.then(() => this._removeTabKeepingWindowAlive());
  return tab;
}
So, finally, for the sake of simplicity, I decided that it would be better to
simply put two calls of this._removeTabKeepingWindowAlive() into _createTab.

+    }
+    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);
+    });
+  },
 

[Wladimir Palant, 2016/09/16 07:10:37]

Nit: Don't add this extra newline please.

[sergei, 2016/09/16 12:34:13]

Done.

-  browser.removeTab(browser.tabs[0]);
 
-  this._deferred = [];
-}
-TabAllocator.prototype = {
   /**
-   * Returns a promise that will resolve into a tab once a tab can be allocated.
+   * 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
-    {
-      let deferred = Promise.defer();
-      this._deferred.push(deferred);
-      return deferred.promise;
-    }
+    if (this._tabs < this._maxtabs)
+      return this._createTab();
+    return new Promise((resolve, reject) => this._resolvers.push(resolve));
   },
 
   /**
@@ -71,15 +102,23 @@ TabAllocator.prototype = {
    */
   releaseTab: function(tab)
   {
-    let browser = tab.parentNode.tabbrowser;
-    browser.removeTab(tab);
-    tab = browser.addTab("about:blank");
-
-    if (this._deferred.length)
-      this._deferred.shift().resolve(tab);
+    // 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.push(tab);
-  }
+    {
+      // navigate away from early opened URL
+      tab.linkedBrowser.loadURI('about:blank', null, null);

[Wladimir Palant, 2016/09/16 07:10:37]

What's the point of navigating away if we are ignoring this tab and will close
it soon anyway?

[sergei, 2016/09/16 12:34:13]

I have a version when the crawler is always running (of course with possibility
for graceful shutdown) and is waiting for a new tasks. If the tab is neither
immediately closed nor navigated away then the web page will be "alive" until
arriving of the next task. On practice it means that the web page can be opened
for hours between the scheduling of portions of URLs. Some web pages consume a
lot of resources (network traffic and leak memory) and I would not like their
counters/analytics scripts to send data for a long time.
It just puts Firefox in a cleaner state.

+      this._tabKeepingWindowAlive = tab;
+    }
+
+    this._tabs--;
+    if (this._resolvers.length && this._tabs < this._maxtabs)
+    {
+      this._resolvers.shift()(this._createTab());
+    }
+  },
 };
 
 /**
@@ -231,8 +270,8 @@ function run(window, urls, timeout, maxtabs, targetURL, onDone)
 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