Crawler frontend

lib/crawler.js

 Cu.import("resource://gre/modules/Services.jsm");
 
 function abprequire(module)
 {
   let result = {};
   result.wrappedJSObject = result;
   Services.obs.notifyObservers(result, "adblockplus-require", module);
   return result.exports;
 }
 
 let {Storage} = require("storage");
 let {Client} = require("client");
 
 let {Policy} = abprequire("contentPolicy");
 let {Filter} = abprequire("filterClasses");
 let {Utils} = abprequire("utils");
 
 let origProcessNode = Policy.processNode;
 
 let siteTabs;
 let currentTabs;
 
 function processNode(wnd, node, contentType, location, collapse)
 {
   let result = origProcessNode.apply(this, arguments);
   let url = (contentType === Policy.type.ELEMHIDE) ? location.text :
       location.spec;
+
   let topWindow = wnd.top;
+  if (!topWindow.document)
+  {
+    Cu.reportError("No document associated with the node's top window");
+    return result;
+  }
+
   let tabbrowser = Utils.getChromeWindow(topWindow).gBrowser;
+  if (!tabbrowser)
+  {
+    Cu.reportError("Unable to get a tabbrowser reference");
+    return result;
+  }
+
   let browser = tabbrowser.getBrowserForDocument(topWindow.document);
+  if (!browser)
+  {
+    Cu.reportError("Unable to get browser for the tab");
+    return result;
+  }
+
   let site = siteTabs.get(browser);

[Wladimir Palant, 2012/09/25 16:51:30]

I would probably add a few more checks here. The three lines above failing if
ABP and Crawler are the only extensions installed is unexpected - but it isn't
impossible. Meaning: the chrome window doesn't necessarily have a gBrowser
property (e.g. a non-browser window making a request), the tabbrowser doesn't
necessarily know the document (e.g. a request in a browser sidebar) and we don't
necessarily know the browser (e.g. a browser belonging to a pop-up window that
was somehow opened despite the pop-up blocker).

   let filtered = !result;
   Storage.write([url, site, filtered]);
   return result;
 }
 
 function loadSite(site, window, callback)
 {
   if (!site)
     return;
 
   let tabbrowser = window.gBrowser;
   let tab = tabbrowser.addTab(site);
   let browser = tabbrowser.getBrowserForTab(tab);
 
+  siteTabs.set(browser, site);
+
   let progressListener = {
     onStateChange: function(aBrowser, aWebProgress, aRequest, aStateFlags, aStatus)
     {
       if (browser !== aBrowser)
         return;
 
       if (!(aStateFlags & Ci.nsIWebProgressListener.STATE_STOP))
         return;
 
       tabbrowser.removeTabsProgressListener(progressListener);
       tabbrowser.removeTab(tab);
       callback();
-    },
-    onLocationChange: function(aBrowser, aWebProgress, aRequest, aLocation, aFlags)
-    {
-      if (browser === aBrowser)
-        siteTabs.set(browser, site);

[Wladimir Palant, 2012/09/25 16:51:30]

Why are we doing this in the progress listener and not at the top of loadSite?
We already know the browser there, onLocationChange() shouldn't be necessary.

     }
   };
   tabbrowser.addTabsProgressListener(progressListener);
 }
 
 function loadSites(backendUrl, parallelTabs, window, sites, callback)
 {
   while (currentTabs < parallelTabs && sites.length)
   {
     currentTabs++;
@@ skipping 19 matching lines @@
 
 let Crawler = exports.Crawler = {};
 
 Crawler.crawl = function(backendUrl, parallelTabs, window, callback)
 {
   if (Policy.processNode != origProcessNode)
     return;
 
   Policy.processNode = processNode;
 
   siteTabs = new WeakMap();

[Wladimir Palant, 2012/09/25 16:51:30]

You should null out this variable when you are done, to make sure the map can be
garbage collected (the usual trouble with global variables).

   currentTabs = 0;
 
   Storage.init();
 
   Client.fetchCrawlableSites(backendUrl, function(sites)
   {
     loadSites(backendUrl, parallelTabs, window, sites, function()
     {
       Policy.processNode = origProcessNode;
+      siteTabs = null;
       callback();
     });
   });
 };