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/29 11:44:58]

Double quotation marks please.

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

Done.

+"use strict";
 
 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");
 }
 
 const {XPCOMUtils} = Cu.import("resource://gre/modules/XPCOMUtils.jsm", {});
 
 /**
  * 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
  */
-const webProgressListener =
+let webProgressListener =
 {
   onStateChange: function(webProgress, request, flags, status)
   {
     if (webProgress.DOMWindow == content &&
-        (flags & Ci.nsIWebProgressListener.STATE_STOP) &&
-        (flags & Ci.nsIWebProgressListener.STATE_IS_WINDOW))

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

You don't have to check STATE_IS_WINDOW any more.

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

Done. I left it just in case.

+        (flags & Ci.nsIWebProgressListener.STATE_STOP))
     {
-      if (content.location.protocol == 'about:')
+      // 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;

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

This needs an explanation. Is that check necessary because you load about:blank
into unused tabs and you don't want to report that load? At the very least, this
needs a comment saying so.

Still, what issue does this check solve? My guess is that without this check the
code will collect data from about:blank, send a message and this message will be
ignored. Wasting a few CPU cycles, so what? Do we really need this special case?
Or maybe we should rather work correctly even if we get something like
about:newtab on our URL list?

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

Answer is in
https://codereview.adblockplus.org/29338242/diff/29340782/lib/child/frameScri...

-      const pageInfo = {channelStatus: status};

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

Please don't declare regular variables as constants. Just because you don't
assign to pageInfo itself but only its properties this doesn't make it a
constant.

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

const in javascript does not mean immutable, it's useful to find unexpected
assignments. Do we use it as indicator of immutability?

I have changed to "let" most of the declarations.

[Wladimir Palant, 2016/09/30 07:43:12]

Yes, that's how const is normally used - to indicate values that are inherently
immutable, not merely unchanged due to current implementation details.

+      let pageInfo = {channelStatus: status};
       if (request instanceof Ci.nsIHttpChannel)
       {
         try
         {
           pageInfo.headers = [];
           pageInfo.headers.push("HTTP/x.x " + request.responseStatus + " " + request.responseStatusText);
           request.visitResponseHeaders((header, value) => pageInfo.headers.push(header + ": " + value));
         }
         catch (e)
         {
@@ skipping 11 matching lines @@
 
   QueryInterface: XPCOMUtils.generateQI([Ci.nsIWebProgressListener, Ci.nsISupportsWeakReference])
 };
 
 function onPageLoaded(pageInfo)
 {
   Object.assign(pageInfo, gatherPageInfo(content));
   sendAsyncMessage("abpcrawler:pageInfoGathered", pageInfo);
 };
 
-const webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebProgress);

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

As above, please don't mark any variable that you happen to only assign once (in
this revision of the code at least, but it might change later) as a constant.
Please get rid of the pseudo-constants below as well.

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

Done.

+let webProgress = docShell.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIWebProgress);
 webProgress.addProgressListener(webProgressListener, Ci.nsIWebProgress.NOTIFY_STATE_WINDOW);
 
 /**
  * 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)
 {
-  const document = wnd.document;
-  const result = {errors:[]};
+  let document = wnd.document;
+  let result = {errors:[]};
   if (!document.documentElement)
   {
-    result.errors.push('No document.documentElement');

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

Double quotation marks please.

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

Done.

+    result.errors.push("No document.documentElement");
     return result;
   }
 
   try
   {
-    const canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
+    let canvas = document.createElementNS("http://www.w3.org/1999/xhtml", "canvas");
     canvas.width = document.documentElement.scrollWidth;
     canvas.height = document.documentElement.scrollHeight;
-    const context = canvas.getContext("2d");
+    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?
-    const serializer = new wnd.XMLSerializer();
+    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;
 }