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
  */
 
@@ skipping 47 matching lines @@
    *
    * @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.<tab>}
    */
   getTab: function()
   {
     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)
   {
     // 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;
     }
 
     this._tabs--;
     if (this._resolvers.length && this._tabs < this._maxtabs)
     {
       this._resolvers.shift()(this._createTab());
     }
   },
 };
@@ skipping 272 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");
 }