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.
+  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;
+  },
+
   /**
-   * 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)
+    {
+      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.

+
+  /**
+   * 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));
   },
 
   /**
    * 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 to keep
+    // the window alive. It will be closed when a new tab is created.
+    if (this._tabs > 1)
+      this._browser.removeTab(tab);
+    else
+    {
+      // 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;
+    }
 
-    if (this._deferred.length)
-      this._deferred.shift().resolve(tab);
-    else
-      this._tabs.push(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");
 }