Issue 3780 - wait for the loading of filters and only afterwards start to fetch pages

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.
  *
  * @param {tabbrowser} browser
  *    The tabbed browser where tabs should be created
  * @param {int} maxtabs
  *    The maximum number of tabs to be allocated
  * @constructor
  */
@@ skipping 177 matching lines @@
   {
     running--;
     if (running <= 0)
     {
       loadListener.stop();
       windowCloser.stop();
       onDone();
     }
   };
 
-  for (let url of urls)
+  new Promise(function(resolve, reject)

[Wladimir Palant, 2016/03/15 09:13:10]

Nit: Why not use arrow functions consistently? (resolve, reject) =>

[sergei, 2016/03/15 12:16:23]

We don't need to capture `this` here, so I have not used arrow function to
indicate that we are not using anything from `this`.

Changed to arrow function for consistency.

[Wladimir Palant, 2016/03/15 13:44:22]

We generally use arrow functions for inline callbacks, it's simply a more
convenient syntax and one doesn't have to think about side-effects (meaning:
`this` pointer changing).

   {
-    running++;
-    Task.spawn(crawl_url.bind(null, url, tabAllocator, loadListener)).then(function(result)
+    if (FilterStorage.subscriptions.length > 0 && !FilterStorage._loading)

[Wladimir Palant, 2016/03/15 09:13:10]

_loading is an internal flag to prevent reentrance, you cannot rely on it.
Checking FilterStorage.subscriptions.length should do.

[Wladimir Palant, 2016/03/15 09:18:11]

Actually, I think that this is a bad assumption from the start - the crawler
might be run without any subscriptions whatsoever, just to see what requests are
being made. Since we are running on startup however we can assume that the
filters didn't load yet and just wait for the "load" notification.

[sergei, 2016/03/15 12:16:23]

I thought about it.

It does happen that "load" event has been already sent and we will never receive
it, thus wait forever, so there should be some way to know whether we should
wait for "load" or not. It happens not so often with easylist, to easier
reproduce it I used a custom filter file with merely a couple of filters (to
test one can add "-f custom-fitler-file=" to command line).
I'm not sure that testing `FilterStorage.subscriptions.length` against zero is
enough because I can easily assume that it's already bigger than zero but not
all subscriptions are loaded yet (because they can be loaded asynchronously),
can it be the truth?
`_loading` is set to false only right before sending of the "load" event, of
course except the initial value, so it seems the best candidate for the role of
the flag whether we need to wait for "load" or not.

In addition, it's not so easy to run it without subscribtions, one need to
change "run.py", either set `extensions.adblockplus.currentVersion` or specify
some pre-configured profile, otherwise there are always several subscriptions.
So I think that if someone wants to run with zero subscriptions he knows what he
is doing and can also adapt crawler.

[Wladimir Palant, 2016/03/15 13:44:22]

No, subscriptions are added all at once when loading finishes. So checking
_loading is unnecessary.

Fair enough - I guess when we decide to run this without filters we'll simply
add an empty subscription.

[sergei, 2016/03/15 14:44:25]

Acknowledged.

     {
-      let request = new XMLHttpRequest();
-      request.open("POST", targetURL);
-      request.addEventListener("load", taskDone, false);
-      request.addEventListener("error", taskDone, false);
-      request.send(JSON.stringify(result));
-    }, function(url, exception)
+      resolve();
+      return;
+    }
+    FilterNotifier.addListener((action, item, newValue, oldValue) =>
     {
-      reportException(exception);
+      if (action === "load")

[Wladimir Palant, 2016/03/15 09:13:11]

Nit: We don't usually use strict equality, and it doesn't make much sense here.

[sergei, 2016/03/15 12:16:23]

Fixed.

+      {
+        resolve();
+      }
+    });
+  }).then(_ =>

[Wladimir Palant, 2016/03/15 09:13:11]

No pointless parameter please, () =>

[sergei, 2016/03/15 12:16:23]

Acknowledged.

+  {
+    for (let url of urls)
+    {
+      running++;
+      Task.spawn(crawl_url.bind(null, url, tabAllocator, loadListener)).then(function(result)
+      {
+        let request = new XMLHttpRequest();
+        request.open("POST", targetURL);
+        request.addEventListener("load", taskDone, false);
+        request.addEventListener("error", taskDone, false);
+        request.send(JSON.stringify(result));
+      }, function(url, exception)
+      {
+        reportException(exception);
 
-      let request = new XMLHttpRequest();
-      request.open("POST", targetURL);
-      request.addEventListener("load", taskDone, false);
-      request.addEventListener("error", taskDone, false);
-      request.send(JSON.stringify({
-        url: url,
-        startTime: Date.now(),
-        error: String(exception)
-      }));
-    }.bind(null, url));
-  }
+        let request = new XMLHttpRequest();
+        request.open("POST", targetURL);
+        request.addEventListener("load", taskDone, false);
+        request.addEventListener("error", taskDone, false);
+        request.send(JSON.stringify({
+          url: url,
+          startTime: Date.now(),
+          error: String(exception)
+        }));
+      }.bind(null, url));
+    }
+  // Be careful, `catch` does not catch exeptions from any asynchronous calls

[Wladimir Palant, 2016/03/15 09:13:11]

exeptions => exceptions.

[sergei, 2016/03/15 12:16:23]

Done. Basically, I have removed the comment.

+  // of this `then` handler because the latter one does not return an array of
+  // promises of asynchrounous tasks and does not contain any waiting code.
+  }).catch(reportException);

[Wladimir Palant, 2016/03/15 09:13:11]

I'm not really happy with the way this is structured. At the very least, then
callback should be moved into a separate function, maybe called crawl_urls. Then
this lengthy comment won't be necessary, nobody will expect that catch callback
to capture errors from everything above.

[sergei, 2016/03/15 12:16:23]

Done.
Yeah, the comment in general is not necessary, I added it rather to attract
attention to the structure of the code above.
I would say it's not a good idea to have such free-running tasks, instead of
"rejected" `function(url, exception)...` it would be better to use `.catch`
because if `function(result)`  somehow throws it will be lost,
 loadListener.stop();
 windowCloser.stop();
 onDone();
from `taskDone` should be rather in some-kind of "finally" block.

 }
 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}
@@ skipping 50 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");
 }