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

lib/crawler.js

Index: lib/crawler.js
diff --git a/lib/crawler.js b/lib/crawler.js
index 54df1501957a2a585f608eaca8042c75bc477051..cc28c96ab280ccf51b16ef7b29f9bd57b66211ea 100644
--- a/lib/crawler.js
+++ b/lib/crawler.js
@@ -22,6 +22,8 @@ function abprequire(module)
 
 let {RequestNotifier} = abprequire("requestNotifier");
 
+let {FilterNotifier} = abprequire("filterNotifier");
+let {FilterStorage} = abprequire("filterStorage");
 
 /**
  * Creates a pool of tabs and allocates them to tasks on request.
@@ -219,31 +221,51 @@ function run(window, urls, timeout, maxtabs, targetURL, 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) =>
+    {
+      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)
     {
-      reportException(exception);
+      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;