Issue 3792 - Fix to support multiprocess firefox

lib/child/frameScript.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/.
  */
+
 "use strict";

[Wladimir Palant, 2016/09/14 16:11:47]

Nit: How about an empty line before that?

[sergei, 2016/09/29 09:58:11]

Done.

 
-const {classes: Cc, interfaces: Ci, utils: Cu, Cr: results} = Components;
+const {classes: Cc, interfaces: Ci, utils: Cu, results: Cr} = Components;
 
 /**
  * @param e exception
  */
 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");
 }
 
-let {Services} = Cu.import("resource://gre/modules/Services.jsm", {});
-let {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
+const {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
 
 /**
- * Waits for finishing of the page loading, calls `gatherPageInfo` and sends
- * gahter information using "abpcrawler:pageInfoGathered" message.
- * https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsIWebProgressListener

[Wladimir Palant, 2016/09/14 16:11:48]

Nit: How about:

* Progress listener capturing the data of any page to finish loading and *
sending this data via "abpcrawler:pageInfoGathered" message.
*
* @type nsIWebProgressListener

Note that type is enough to look up documentation.

[sergei, 2016/09/29 09:58:12]

I have changed the comment. In addition webProgressListener does not send the
message, instead it calls onPageLoaded because in future version this function
will be more complicated and it's better to keep such code out of
webProgressListener.

+ * Progress listener capturing the data of the current page and calling
+ * onPageLoaded(data) when loading is finished, where data contains
+ * HTTP status and headers.
+ *
+ * @type nsIWebProgressListener
  */
 let webProgressListener =
 {
   onStateChange: function(webProgress, request, flags, status)
   {
-    // use isTopLevel to filter beacon requests out

[Wladimir Palant, 2016/09/14 16:11:46]

This comment doesn't make sense - not just beacon requests, you don't want to be
bothered with frames either. Either way, we don't want to risk receiving
messages for the wrong window (e.g. another tab). How about checking
`webProgress.DOMWindow == content` instead of isTopLevel here? Note that
isTopLevel was introduced for the scenario where your progress listener runs in
a different process but that's not what you have here.

[sergei, 2016/09/29 09:58:12]

Done. It seems it does indeed work, however I think I have tried it and it was
not working.

-    if (webProgress.isTopLevel &&
-        (flags & Ci.nsIWebProgressListener.STATE_STOP) &&
-        (flags & Ci.nsIWebProgressListener.STATE_IS_WINDOW))
+    if (webProgress.DOMWindow == content &&
+        (flags & Ci.nsIWebProgressListener.STATE_STOP))
     {
+      // First time we receive STATE_STOP for about:blank and the second time
+      // for our interested URL which is distinct from about:blank.
+      // However we should not process about:blank because it can happen that
+      // the message with information about about:blank is delivered when the
+      // code in crawler.js is already waiting for a message from this tab.
+      // Another case we are not interested in is about:newtab.
+      if (content.location.protocol == "about:")
+        return;
+      let pageInfo = {channelStatus: status};
       if (request instanceof Ci.nsIHttpChannel)
       {
-        let pageInfo = {headers: []};

[Wladimir Palant, 2016/09/14 16:11:48]

What about `channelStatus: status`? We used to capture that.

[sergei, 2016/09/29 09:58:11]

Restored.

         try
         {
+          pageInfo.headers = [];
           pageInfo.headers.push("HTTP/x.x " + request.responseStatus + " " + request.responseStatusText);
           request.visitResponseHeaders((header, value) => pageInfo.headers.push(header + ": " + value));
         }
         catch (e)
         {
-          // Ignore if called before the response has been received (before
-          // onStartRequest()).
-          if (e.result == Cr.NS_ERROR_NOT_AVAILABLE)
-            return;

[Wladimir Palant, 2016/09/14 16:11:48]

So what happens if a host name doesn't resolve? We just time out without
collecting any data whatsoever? We have the channel status, it's useful
information and should be sent. Maybe it's a good idea to not report exceptions
in case of NS_ERROR_NOT_AVAILABLE, but just returning definitely isn't.

[sergei, 2016/09/29 09:58:13]

If the hostname is not resolved we don't get here anyway, so, yes, we do wait
for the timeout, https://issues.adblockplus.org/ticket/3975.

It was used here as workaround for race condition, the exception is thrown when
we try to access request.responseStatus but the page has not been loaded yet, so
I think that simply returning and waiting for the real finish of loading was a
good idea.

I have added the check of the location.protocol above and removed that check, so
if it happens on a real page it will dump the exception info and call
onPageLoaded.

[Wladimir Palant, 2016/09/29 11:44:57]

I don't really see why the page isn't loaded when you get STATE_STOP.

[sergei, 2016/09/29 15:36:21]

Sorry for confusion, I meant under the page the page with URL distinct from
about:blank. First time we receive STATE_STOP for about:blank and the second
time for our URL. We should not process about:blank because it can happen that
the message with information about about:blank is delivered when the code in
crawler.js is already waiting for a message from that tab. When about:blank is
loaded first time, request.responseStatus is not available.
I have put a comment about it into the code.

BTW, I have just discovered yet one issue with
https://github.com/adblockplus/abpcrawler/blob/master/lib/crawler.js#L111. When
we navigate to about:blank from our URL we get STATE_STOP the third time but the
value of content.location.href is sometimes (!) still old (our URL) and it tries
to get the page info yet one time, and according to the log it fails trying to
get screenshot. I guess it happens because the tab is going to be closed. It's
easy to reproduce if run the crawler with one tab because in that case it
happens very often.

I would propose to come back to creation of a new tab in TabAllocator.releaseTab
to avoid any side effects of state of already processed tab.

[sergei, 2016/10/04 10:57:31]

What about creation of a new tab in TabAllocator.releaseTab?

[Wladimir Palant, 2016/10/04 11:10:38]

No, I'd rather not make the code more complicated than it already is for this
edge case. I'd suggest that you push your patch and deal with it in a follow-up
issue - this took long enough already.
Sure that this is STATE_STOP for about:blank? I'd suspect that the page timed
out, so it's actually getting its first STATE_STOP when you navigate away to
about:blank - and content.location.href is set correctly at that point. I'm not
sure why creating a screenshot fails at that point but maybe document.readyState
will help you recognize this situation.

[sergei, 2016/10/06 14:26:58]

Of course it should be in a follow-up issue, the question is whether we need it
or not.
Yes, sure, there is simply no space to time out. Here is the log when we get
STATE_STOP and the number of tabs is 1.
console.log: owinID: 14, href: about:blank, rstate: uninitialized
console.log: owinID: 16, href: about:newtab, rstate: complete
console.log: owinID: 14, href: ftp://speedtest.tele2.net/, rstate: complete
console.log: owinID: 14, href: ftp://speedtest.tele2.net/, rstate: complete
console.log: owinID: 20, href: about:blank, rstate: uninitialized
console.log: owinID: 20, href: https://www.amazon.com/, rstate: complete
console.log: owinID: 20, href: https://www.amazon.com/, rstate: complete
console.log: owinID: 27, href: about:blank, rstate: uninitialized
console.log: owinID: 27, href: https://codereview.adblockplus.org/, rstate:
complete
console.log: owinID: 27, href: https://codereview.adblockplus.org/, rstate:
complete
console.log: owinID: 30, href: about:blank, rstate: uninitialized
console.log: owinID: 30, href: https://twitter.com/, rstate: complete
console.log: owinID: 30, href: about:blank, rstate: complete
console.log: owinID: 43, href: about:blank, rstate: complete
console.log: owinID: 43, href: http://edition.cnn.com/, rstate: complete
console.log: owinID: 43, href: http://edition.cnn.com/, rstate: complete
console.log: owinID: 94, href: about:blank, rstate: uninitialized
console.log: owinID: 94, href: http://bcc.com/, rstate: complete
console.log: owinID: 94, href: http://bcc.com/, rstate: complete
console.log: owinID: 97, href: about:blank, rstate: uninitialized
console.log: owinID: 97, href: https://adblockplus.org/, rstate: complete
console.log: owinID: 97, href: about:blank, rstate: complete

owinID - outerWindowID
rstate - document.readyState

I have tried it with commented line tab.linkedBrowser.loadURI("about:blank",
null, null); in releaseTab and it looks very similar however there is no any
third entry for the same outer window ID.

           reportException(e);
         }
-        Object.assign(pageInfo, gatherPageInfo(content));
-        sendAsyncMessage("abpcrawler:pageInfoGathered", pageInfo);

[Wladimir Palant, 2016/09/14 16:11:47]

The two lines above don't depend on the request object and should be outside the
`if` block. If we happen to have a page served via FTP and we cannot get the
headers - we would still like to capture all other page data.

[sergei, 2016/09/29 09:58:12]

Moved.

       }
+      onPageLoaded(pageInfo);
     }
   },
 
-  // definitions of the remaining functions see related documentation

[Wladimir Palant, 2016/09/14 16:11:48]

Nit: this comment doesn't add any value, remove?

[sergei, 2016/09/29 09:58:11]

Done.

-  onLocationChange: function(webProgress, request, URI, flag) {},
-  onProgressChange: function(aWebProgress, aRequest, curSelf, maxSelf, curTot, maxTot) {},

[Wladimir Palant, 2016/09/14 16:11:47]

So, Hungarian notation or not? Our style guide says: not. But you can also just
leave out the parameters since you aren't actually implementing these methods
(and they won't even be called with proper filtering in place).

[sergei, 2016/09/29 09:58:12]

Done.

-  onStatusChange: function(aWebProgress, aRequest, aStatus, aMessage) {},
-  onSecurityChange: function(aWebProgress, aRequest, aState) {},
- 
+  onLocationChange: function() {},
+  onProgressChange: function() {},
+  onStatusChange: function() {},
+  onSecurityChange: function() {},
+
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference])

[Wladimir Palant, 2016/09/14 16:11:48]

I don't think that progress listeners can be weak references, exposing
nsIWebProgressListener should be sufficient.

[sergei, 2016/09/29 09:58:13]

According to the documentation, "This object must also implement
nsISupportsWeakReference."
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interfa....

 };
 
-let filter = Cc["@mozilla.org/appshell/component/browser-status-filter;1"]
-             .createInstance(Ci.nsIWebProgress);

[Wladimir Palant, 2016/09/14 16:11:48]

Nit: we usually align the dot with the [ on the previous line.

Either way, why do we even bother with browser-status-filter if we already
registered our progress listener on the docShell for the current tab?

[sergei, 2016/09/29 09:58:12]

Removed. BTW, in the example from mozilla they are also using filter but it does
indeed work without it.

[Wladimir Palant, 2016/09/29 11:44:57]

Yes, there is an example mentioning it - but no documentation explaining what it
does. The point of this filter is apparently limiting the number of times C++
needs to call into JavaScript, for performance reasons. For that it filters out
some messages using an undocumented approach. This has all the signs on an
internal component meant to work with the progress listener installed by Firefox
but non necessary doing the right thing for us. We shouldn't use it, even if the
results are correct right now they won't necessarily be in the next Firefox
release.

[sergei, 2016/09/29 15:36:21]

Acknowledged.

-filter.addProgressListener(webProgressListener, Ci.nsIWebProgress.NOTIFY_ALL);
+function onPageLoaded(pageInfo)
+{
+  Object.assign(pageInfo, gatherPageInfo(content));
+  sendAsyncMessage("abpcrawler:pageInfoGathered", pageInfo);
+};
 
-let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor)
-                          .getInterface(Ci.nsIWebProgress);
-webProgress.addProgressListener(filter, Ci.nsIWebProgress.NOTIFY_ALL);

[Wladimir Palant, 2016/09/14 16:11:48]

The reason why the original code is receiving all progress events: tabbrowser
doesn't support filtering. nsIWebProgress supports filtering however, so you
should use it rather than requesting all kind of stuff you don't need.
Ci.nsIWebProgress.NOTIFY_STATE_WINDOW should do.

[sergei, 2016/09/29 09:58:12]

Done.

+let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebProgress);
+webProgress.addProgressListener(webProgressListener, Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
 
 /**
- * Gathers information about page using DOM window.

[Wladimir Palant, 2016/09/14 16:11:47]

Just "Gathers information about a DOM window." maybe?

[sergei, 2016/09/29 09:58:13]

Done.

+ * Gathers information about a DOM window.
  * Currently
  *  - creates a screenshot of the page
  *  - serializes the page source code
  * @param {nsIDOMWindow} wnd window to process
  * @return {Object} the object containing "screenshot" and "source" properties.
  */
 function gatherPageInfo(wnd)
 {
   let document = wnd.document;
   let result = {errors:[]};
-  if (document.documentElement)
+  if (!document.documentElement)
   {
-    try
-    {
-      let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
-      // Firefox does not work with large canvas elements.
-      // http://stackoverflow.com/questions/6081483/maximum-size-of-a-canvas-element

[Wladimir Palant, 2016/09/14 16:11:48]

Great, you could have linked to http://stackoverflow.com/a/12644047/785541 just
as well :-(

As I mentioned back in 2012 - these are all implementation details and subject
to change any time. Nor are the values you hardcoded here actually correct
(didn't you consider the number 472907776 rather unlikely?). The reason why I
decided to just let the canvas fail is because the actual restrictions are hard
to pinpoint and one wouldn't want to cut off screenshots unnecessarily. This is
an unrelated change, please put it into a separate review and we can discuss the
best approach there.

[sergei, 2016/09/29 09:58:11]

Removed from here.

-      canvas.width = document.documentElement.scrollWidth;
-      canvas.height = document.documentElement.scrollHeight;
-      if (canvas.width > 0 && canvas.height > 0)
-      {
-        if (canvas.width > 32767)
-        {
-          result.errors.push("Width exceeds supported limit, " + canvas.width);
-          canvas.width = 32767;
-        }
-        if (canvas.height > 32767)
-        {
-          result.errors.push("Height exceeds supported limit, " + canvas.height);
-          canvas.height = 32767;
-        }
-        if (canvas.width * canvas.height > 472907776)
-        {
-          result.errrors.push("Area exceeds supported limit, " + canvas.height + " * " + canvas.width);
-          canvas.height = 472907776 / canvas.width;
-        }
-        let context = canvas.getContext("2d");
-        context.drawWindow(wnd, 0, 0, canvas.width, canvas.height, "rgb(255, 255, 255)");
-        result.screenshot = canvas.toDataURL("image/jpeg", 0.8);
-      }
-      // TODO: Capture frames as well?
-      let serializer = new wnd.XMLSerializer();
-      result.source = serializer.serializeToString(document.documentElement);
-    }
-    catch (e)
-    {
-      reportException(e);
-      result.errors.push("Cannot gather page info");

[Wladimir Palant, 2016/09/14 16:11:47]

The original code had the canvas in a separate try block - this code is known to
fail, it shouldn't affect the other data we are capturing.

[sergei, 2016/09/29 09:58:13]

Done.

-    }
+    result.errors.push("No document.documentElement");
+    return result;
   }
+
+  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(wnd, 0, 0, canvas.width, canvas.height, "rgb(255, 255, 255)");
+    result.screenshot = canvas.toDataURL("image/jpeg", 0.8);
+  }
+  catch (e)
+  {
+    reportException(e);
+    result.errors.push("Cannot make page screenshot");
+  }
+
+  try
+  {
+    // TODO: Capture frames as well?
+    let serializer = new wnd.XMLSerializer();
+    result.source = serializer.serializeToString(document.documentElement);
+  }
+  catch(e)
+  {
+    reportException(e);
+    result.errors.push("Cannot obtain page source code");
+  }
+
   return result;
 }