Adblock Plus Crawler rewrite

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

[saroyanm, 2015/05/04 18:13:43]

I think this should be file overview, maybe smth like:
@fileOverview loads a range of websites and records which elements are filtered
by Adblock Plus

[Wladimir Palant, 2015/05/07 00:04:59]

As Sebastian noted elsewhere, @module is actually appropriate here. And - yes,
we need to fix that in Adblock Plus for Firefox as well.

  */
 
 Cu.import("resource://gre/modules/Services.jsm");
 Cu.import("resource://gre/modules/Task.jsm");
 Cu.import("resource://gre/modules/Promise.jsm");
+
+let {reportException} = require("debug");
 
 function abprequire(module)
 {
   let result = {};

[saroyanm, 2015/05/04 18:13:43]

We can create here Object without Object prototype:
Object.create(null);

[Sebastian Noack, 2015/05/04 20:39:01]

It seems that we don't need to bother about conflicts with properties inherited
form Object.prototypte here. Also the lookup below isn't a hotspot. So IMO using
an object literal is totally fine, and even preferable to keep it simple.

[Wladimir Palant, 2015/05/07 00:04:59]

Yes, this isn't a place where property conflicts could be an issue. This code is
actually identical to the require() function defined in
chrome/content/ui/utils.js in adblockplus repository.

   result.wrappedJSObject = result;
   Services.obs.notifyObservers(result, "adblockplus-require", module);
   return result.exports;
 }
 
 let {Policy} = abprequire("contentPolicy");
 let {RequestNotifier} = abprequire("requestNotifier");
 let {Utils} = abprequire("utils");
 
 let dataForTab = new WeakMap();
 
 /**
  * Creates a pool of tabs and allocates them to tasks on request.
  *
  * @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)

[saroyanm, 2015/05/04 18:13:43]

Why don't we use default amount of maxtab equal to the urls count specified in
the textfile ?
And can you please give me a hint how it's being calculated in case it's not
specified ?

[Wladimir Palant, 2015/05/07 00:04:59]

Because the text file has 50 thousand URLs.
run.py sets it to 15 by default.

 {
   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._queue = [];
 }
 TabAllocator.prototype = {
   /**
    * Returns a promise that will resolve into a tab once a tab can be allocated.
    * The tab cannot be used by other tasks until releaseTab() is called.
    *
-   * @result {Promise}

[saroyanm, 2015/05/04 18:13:43]

Nit: @return ?

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+   * @return {Promise}
    */
   getTab: function()
   {
     if (this._tabs.length)
       return this._tabs.shift();
     else
-    {
-      let deferred = Promise.defer();

[saroyanm, 2015/05/04 18:13:43]

The Deffered object is obsolete starting from Gecko 30:
https://developer.mozilla.org/en-US/docs/Mozilla/JavaScript_code_modules/Prom...

Please use Promise constructor instead.
 
Also please update the metadata.gecko for the compatibility. Not sure if it make
sense to make it backward compatible with Deffered. Promises should be supported
by Firefox starting from v29 according to HTML5Rocks article, couldn't find
official info:
http://www.html5rocks.com/en/tutorials/es6/promises/#toc-browser-support

[Wladimir Palant, 2015/05/07 00:04:59]

Promise constructor in Promise.jsm is actually supported starting with Firefox
30 (https://bugzilla.mozilla.org/show_bug.cgi?id=941757).

Done.

-      this._deferred.push(deferred);
-      return deferred.promise;
-    }
+      return new Promise((resolve, reject) => this._queue.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 (this._deferred.length)
-      this._deferred.shift().resolve(tab);

[saroyanm, 2015/05/04 18:13:43]

As mentioned above, the Deffered object is obsolete.

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+    if (this._queue.length)
+      this._queue.shift()(tab);
     else
       this._tabs.push(tab);
   }
 };
 
 /**
  * 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
  */
 function LoadListener(browser, timeout)
 {
   this._browser = browser;
-  this._deferred = new Map();
+  this._queue = new Map();
   this._timeout = timeout;
   browser.addTabsProgressListener(this);
 }
 LoadListener.prototype = {
   /**
    * Returns a promise that will be resolved when the page in the specified tab
    * finishes loading. Loading will be stopped if the timeout is reached.
    *
    * @param {tab} tab
-   * @result {Promise}

[saroyanm, 2015/05/04 18:13:43]

Nit: @return

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+   * @return {Promise}
    */
   waitForLoad: function(tab)
   {
-    let deferred = Promise.defer();

[saroyanm, 2015/05/04 18:13:43]

As mentioned above this is Obsolete, this comment actually goes to other
Deffered object usage in the script. Will stop comment on each related row.

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

-    this._deferred.set(tab.linkedBrowser, deferred);
-
-    tab.ownerDocument.defaultView.setTimeout(function()
-    {
-      tab.linkedBrowser.stop();
-    }, this._timeout);
-
-    return deferred.promise;
+    return new Promise((resolve, reject) =>
+    {
+      this._queue.set(tab.linkedBrowser, resolve);
+
+      tab.ownerDocument.defaultView.setTimeout(function()
+      {
+        tab.linkedBrowser.stop();
+      }, this._timeout);
+    });
   },
 
   /**
    * Deactivates this object.
    */
   stop: function()
   {
     this._browser.removeTabsProgressListener(this);
   },
 
   onStateChange: function(browser, progress, request, flags, status)
   {
-    if ((flags & Ci.nsIWebProgressListener.STATE_STOP) && (flags & Ci.nsIWebProgressListener.STATE_IS_WINDOW))

[saroyanm, 2015/05/04 18:13:43]

Nit: This case can be splitted into multiple lines.

-    {
-      let deferred = this._deferred.get(browser);
-      if (deferred)
+    if ((flags & Ci.nsIWebProgressListener.STATE_STOP) &&

[saroyanm, 2015/05/07 13:19:00]

Nit: We usually also move operators to new line, for consistency.
if ((flags & Ci.nsIWebProgressListener.STATE_STOP)
    && (flags & Ci.nsIWebProgressListener.STATE_IS_WINDOW))

+        (flags & Ci.nsIWebProgressListener.STATE_IS_WINDOW))
+    {
+      let resolve = this._queue.get(browser);
+      if (resolve)
       {
-        this._deferred.delete(browser);
+        this._queue.delete(browser);
 
         let headers = [];
         if (request instanceof Ci.nsIHttpChannel)
         {
           try
           {
             headers.push("HTTP/x.x " + request.responseStatus + " " + request.responseStatusText);
             request.visitResponseHeaders((header, value) => headers.push(header + ": " + value));
           }
           catch (e)
           {
             // Exceptions are expected here

[saroyanm, 2015/05/04 18:13:43]

Please also handle the exception. 
reportException(e);

[Wladimir Palant, 2015/05/07 00:04:59]

As the comment explains, exceptions are expected here. If the connection wasn't
established, request.responseStatus will throw an exception. Spamming the
console with that is pointless, we are saving the channel status anyway.

           }
         }
-        deferred.resolve([status, headers]);
+        resolve([status, headers]);
       }
     }
   }
 };
 
 /**
  * Once created, this object will make sure all new windows are dismissed
  * immediately.
  *
  * @constructor
  */
 function WindowCloser()
 {
-  Services.obs.addObserver(this, "xul-window-registered", true)

[saroyanm, 2015/05/04 18:13:43]

Missing semicolon

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+  Services.obs.addObserver(this, "xul-window-registered", true);
 }
 WindowCloser.prototype = {
   /**
    * Deactivates this object.
    */
   stop: function()
   {
-    Services.obs.removeObserver(this, "xul-window-registered")

[saroyanm, 2015/05/04 18:13:43]

Missing semicolon

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+    Services.obs.removeObserver(this, "xul-window-registered");
   },
 
   observe: function(subject, topic, data)
   {
     let window = subject.QueryInterface(Ci.nsIInterfaceRequestor)
-                        .getInterface(Ci.nsIDOMWindow)

[saroyanm, 2015/05/04 18:13:43]

Missing semicolon

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+                        .getInterface(Ci.nsIDOMWindow);
     window.addEventListener("load", function()
     {
-      if (window.document.documentElement.localName == 'dialog')

[saroyanm, 2015/05/04 18:13:43]

Nit: Please use double quote.

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+      if (window.document.documentElement.localName == "dialog")
         window.document.documentElement.acceptDialog();
       else
         window.close();
     }, false);
   },
 
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver, Ci.nsISupportsWeakReference])
 };
 
 /**
  * Retrieves crawler results associated with a particular content window.
  *
  * @param {Window} window
  *    Content window to retrieve crawler results for
- * @result {Object}

[saroyanm, 2015/05/04 18:13:43]

Nit: @return

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+ * @return {Object}
  *    Crawler results or undefined if the window wasn't created by the crawler.
  */
 function getDataForWindow(window)
 {
   let topWindow = window.top;
   if (!topWindow.document)
     throw new Error("No document associated with the node's top window");
   let tabbrowser = Utils.getChromeWindow(topWindow).getBrowser();
   if (!tabbrowser)
     throw new Error("Unable to get a tabbrowser reference from the window");
   let browser = tabbrowser.getBrowserForDocument(topWindow.document);
   if (!browser)
     throw new Error("Unable to get browser for the content window");
   let tab = tabbrowser.getTabForBrowser(browser);
   if (!tab)
     throw new Error("Unable to get tab for the browser");
   return dataForTab.get(tab);
-};

[saroyanm, 2015/05/04 18:13:43]

Nit: Semicolon is redundant here.

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+}
 
 /**
  * Starts the crawling session. The crawler opens each URL in a tab and stores
  * the results.
  *
  * @param {Window} window
  *    The browser window we're operating in
  * @param {String[]} urls
  *    URLs to be crawled
  * @param {int} number_of_tabs
  *    Maximum number of tabs to be opened
  * @param {String} targetURL
  *    URL that should receive the results

[saroyanm, 2015/05/04 18:13:43]

Nit: Please also document onDone parameter for consistency.

[Sebastian Noack, 2015/05/04 20:39:01]

Also note that JsDoc 3 will mistakenly document this (and similarly defined)
function(s) as @inner (because it doesn't see that it is assigned to exports.*).
In order to avoid that you could do one of following:

1. Explicitly add the @static tag
2. Directly assign the function to exports.*

  /**
   * ..
   */
  exports.run = function(..) {..}

3. Assign the function both, a local variable and exports.* in one expression,
but add the docs after the local variable:

  let run =
  /**
   * ..
   */
  exports.run = function(..) {..}

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+ * @param {Function} onDone
+ *    Callback to be executed once the processing finishes
+ * @static
  */
 function run(window, urls, timeout, maxtabs, targetURL, onDone)
 {
   let requestNotifier = new RequestNotifier(null, function() {});
 
   let origProcessNode = Policy.processNode;
   Policy.processNode = processNodeReplacement.bind(null, origProcessNode, requestNotifier);
 
   let tabAllocator = new TabAllocator(window.getBrowser(), maxtabs);
   let loadListener = new LoadListener(window.getBrowser(), timeout);
@@ skipping 39 matching lines @@
   }
 }
 exports.run = run;
 
 /**
  * Crawls a URL. This is a generator meant to be used via a Task object.
  *
  * @param {String} url
  * @param {TabAllocator} tabAllocator
  * @param {loadListener} loadListener
- * @result {Object}

[saroyanm, 2015/05/04 18:13:43]

Nit: @return {Object} Crawling result

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+ * @return {Object}
  *    Crawling result
  */
 function* crawl_url(url, tabAllocator, loadListener)

[Sebastian Noack, 2015/05/07 12:33:04]

Nit: camel case

 {
   let tab = yield tabAllocator.getTab();
   let result = {url: url};
 
   dataForTab.set(tab, result);
   try
   {
     result.startTime = Date.now();
     tab.linkedBrowser.loadURI(url, null, null);
     [result.channelStatus, result.headers] = yield loadListener.waitForLoad(tab);
     result.endTime = Date.now();
     result.finalUrl = tab.linkedBrowser.currentURI.spec;
 
     let document = tab.linkedBrowser.contentDocument;
     if (document.documentElement)
     {
       try
       {
         let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
         canvas.width = document.documentElement.scrollWidth;
         canvas.height = document.documentElement.scrollHeight;
 
         let context = canvas.getContext("2d");
         context.drawWindow(document.defaultView, 0, 0, canvas.width, canvas.height, "rgb(255, 255, 255)");
         result.screenshot = canvas.toDataURL("image/jpeg", 0.8);

[saroyanm, 2015/05/04 18:13:43]

Maybe make sense to let user specify the quality of image same way it's done for
"maxtabs" ? 
What you think ?
Anyway we don't have any specifications for this review, so it's optional I
think.

[Wladimir Palant, 2015/05/07 00:04:59]

Well, changing maxtabs is very useful when testing. Given that there won't be a
"user" here but merely a server running this, I cannot really imagine anybody
tweaking this parameter. Still, we can easily add it if we need it.

[saroyanm, 2015/05/07 13:19:00]

Good point.

       }
       catch (e)
       {
         reportException(e);
         result.error = "Capturing screenshot failed: " + e;

[saroyanm, 2015/05/04 18:13:43]

Isn't result.error redundant ?

[Wladimir Palant, 2015/05/07 00:04:59]

No. The exception is merely reported to the console but not stored with the data
- useful for debugging, nothing beyond that. The result variable on the other
hand will be saved in a file, so somebody will be able to look up why the
screenshot for this particular site wasn't saved.

[saroyanm, 2015/05/07 13:19:00]

Yes, missed that. 
Good point.

       }
 
       // TODO: Capture frames as well?

[saroyanm, 2015/05/04 18:13:43]

Nit: I think we shouldn't have TODO comments in released product.

[Wladimir Palant, 2015/05/07 00:04:59]

Why not? This is something we might want to do in future, not really important
right now however.

       let serializer = new tab.ownerDocument.defaultView.XMLSerializer();
       result.source = serializer.serializeToString(document.documentElement);
     }
   }
   finally
   {
     tabAllocator.releaseTab(tab);
   }
   return result;
-}
-
-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");
 }
 
 /**
  * Wrapper for the Policy.processNode() function in ABP. Calls the original
  * function and records all the data.
  *
  * @param {Function} origProcessNode
  *      The original processNode function.
  * @param {RequestNotifier} requestNotifier
  *      The crawler's RequestNotifier object instance.
  * @param {nsIDOMWindow} wnd
  * @param {nsIDOMElement} node
  * @param {Number} contentType
  * @param {nsIURI} location
  * @param  {Boolean} collapse
  * @return {Boolean}
  */
 function processNodeReplacement(origProcessNode, requestNotifier, wnd, node, contentType, location, collapse)
 {
   let filters = [];
   let origListener = requestNotifier.listener;
   requestNotifier.listener = function(window, node, entry)
   {
     if (entry.filter)
       filters.push(entry.filter.text);
   };
 
-  /*
-   * Call the original processNode. If the original throws, then we will too, so this is outside a try clause.

[saroyanm, 2015/05/04 18:13:43]

Nit: the line is too long.

[Sebastian Noack, 2015/05/04 20:39:01]

Also I suggest to use simply // for comments explaining the logic inside a
function, instead the more verbose JsDoc style.

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

-   */
+  // Call the original processNode. If the original throws, then we will too,
+  // so this is outside a try clause.
   let result;
   try
   {
     result = origProcessNode(wnd, node, contentType, location, collapse);
   }
   finally
   {
     requestNotifier.listener = origListener;
   }
 
@@ skipping 10 matching lines @@
         blocked: result != Ci.nsIContentPolicy.ACCEPT,
         filters: filters
       });
     }
   }
   catch (e)
   {
     reportException(e);
   }
   return result;
-};

[saroyanm, 2015/05/04 18:13:43]

Nit: Semicolon is redundant here.

[Wladimir Palant, 2015/05/07 00:04:59]

Done.

+}