Issue 4580 - Removed ext.webRequest.onBeforeRequest

lib/requestBlocker.js

Index: lib/requestBlocker.js
===================================================================
--- a/lib/requestBlocker.js
+++ b/lib/requestBlocker.js
@@ -90,19 +90,53 @@
   }
 }
 
-ext.webRequest.onBeforeRequest.addListener((url, type, page, frame) =>
+browser.webRequest.onBeforeRequest.addListener(details =>
 {
+  // Never block top-level documents.
+  if (details.type == "main_frame")
+    return;
+
+  // Filter out requests from non web protocols. Ideally, we'd explicitly
+  // specify the protocols we are interested in (i.e. http://, https://,
+  // ws:// and wss://) with the url patterns, given below, when adding this
+  // listener. But unfortunately, Chrome <=57 doesn't support the WebSocket
+  // protocol and is causing an error if it is given.
+  let url = new URL(details.url);
+  if (url.protocol != "http:" && url.protocol != "https:" &&
+      url.protocol != "ws:" && url.protocol != "wss:")
+    return;

[kzar, 2018/04/05 11:26:26]

Please could you add the braces here, to be consistent with the below condition
(once my feedback is addressed).

[Sebastian Noack, 2018/04/05 16:04:09]

See below.

+
+  // Firefox (only) allows to intercept requests sent by the browser
+  // and other extensions. We don't want to block these.
+  if (details.originUrl)
+  {
+    let originUrl = new URL(details.originUrl);
+    if (originUrl.protocol == "chrome:" ||
+        originUrl.protocol == "moz-extension:")
+      return;

[kzar, 2018/04/05 11:26:26]

Please can you add the braces back, since the rule is if the condition or the
body spans multiple lines you have to add them.

[Sebastian Noack, 2018/04/05 16:04:09]

Since when is this the rule? This is the first time hear that we insist on
braces if the block is only one line but the condition has multiple lines. It's
not in our coding style guide, nor does our ESLint config complain.

This rule doesn't make any sense to me. Braces group the block, and if it has
more than one line (in particular with nested logic) braces avoid
misinterpretation. I don't see what this has to do with the condition.

[Manish Jethani, 2018/04/06 14:12:48]

As far back as I can remember (which is not too far since I've been around for
only a year), Dave has always insisted on this, and I have made it a point to do
it. To be fair a lot of the rules don't make sense, our style guide needs an
upgrade at some point.

For what it's worth, when there's an else block, I also add the braces around
the else block now to "balance it out".

e.g.

  if (foo ||
      bar)
  {
    doSomething();
  }
  else
  {
    doSomethingElse();
  }

If the condition fit on one line then I would drop the braces from both blocks.

Anyway, my two cents. I'm not a big fan of these rules myself.

+  }
+
+  let frame = ext.getFrame(

[kzar, 2018/04/05 11:26:26]

Seems like previously we didn't call getFrame if details.tabId was -1 but now we
do. I wonder if that matters?

[Sebastian Noack, 2018/04/05 16:04:09]

getFrame() just returns undefined if there is not tab or frame with the given
ID. If the tabId is -1 it will effectively always return undefined.

+    details.tabId,
+    // We are looking for the frame that contains the element which
+    // has triggered this request. For most requests (e.g. images) we
+    // can just use the request's frame ID, but for subdocument requests
+    // (e.g. iframes) we must instead use the request's parent frame ID.
+    details.type == "sub_frame" ? details.parentFrameId : details.frameId
+  );
+
+  let page = null;
   let docDomain = null;
   let sitekey = null;
+  let thirdParty = false;
   let specificOnly = false;
-  let thirdParty = false;
-  let urlString = stringifyURL(url);
 
+  if (frame)
+  {
+    page = new ext.Page({id: details.tabId});
 
-  if (frame && page)
-  {
     if (checkWhitelisted(page, frame))
-      return true;
+      return;
 
     docDomain = extractHostFromFrame(frame);
     sitekey = getKey(page, frame);
@@ -111,20 +145,21 @@
                                       RegExpFilter.typeMap.GENERICBLOCK);
   }
 
-  let mappedType = resourceTypes.get(type) || "OTHER";
-
+  let urlString = stringifyURL(url);
+  let type = resourceTypes.get(details.type) || "OTHER";
   let filter = defaultMatcher.matchesAny(
-    urlString, RegExpFilter.typeMap[mappedType],
+    urlString, RegExpFilter.typeMap[type],
     docDomain, thirdParty, sitekey, specificOnly
   );
 
   setTimeout(onBeforeRequestAsync, 0, page, urlString,
-                                      mappedType, docDomain,
+                                      type, docDomain,
                                       thirdParty, sitekey,
                                       specificOnly, filter);
 
-  return !(filter instanceof BlockingFilter);
-});
+  if (filter instanceof BlockingFilter)
+    return {cancel: true};
+}, {urls: ["<all_urls>"]}, ["blocking"]);
 
 port.on("filters.collapse", (message, sender) =>
 {