Issue 3651 - Fix popup blocking with dynamically created openers and invisible redirects

lib/popupBlocker.js

Index: lib/popupBlocker.js
===================================================================
--- a/lib/popupBlocker.js
+++ b/lib/popupBlocker.js
@@ -25,14 +25,69 @@
 let {checkWhitelisted} = require("whitelisting");
 let {logRequest} = require("devtools");
 
-let tabsLoading = {};
+let loadingPopups = Object.create(null);
 
-function checkPotentialPopup(tabId, url, sourcePage, documentHost, specificOnly)
+function hasLoadingPopups()
 {
-  let urlObj = new URL(url || "about:blank");
+  return Object.keys(loadingPopups).length > 0;

[Wladimir Palant, 2016/02/12 15:43:43]

With this being called from onCreatedNavigationTarget I wonder whether
performance is important. Did you consider not retrieving all keys?

  for (var tabId in loadingPopups)
    return true;
  return false;

[Sebastian Noack, 2016/02/12 15:52:13]

V8 is optimized for usage of Object.keys() like here. Looping over an object in
hash-table mode using for..in, on the other hand, causes an expensive
deoptimization with V8.

+}
+
+function forgetPopup(tabId)
+{
+  delete loadingPopups[tabId];
+
+  if (!hasLoadingPopups())
+  {
+    chrome.webRequest.onBeforeRequest.removeListener(onBeforeRequest);
+    chrome.webNavigation.onCompleted.removeListener(onCompleted);
+    chrome.tabs.onRemoved.removeListener(forgetPopup);
+  }
+}
+
+function getFrame(tabId, processId, frameId, callback)
+{
+  chrome.webNavigation.getFrame(
+    {
+      tabId: tabId,
+      processId: processId,
+      frameId: frameId
+    },
+    details =>
+    {
+      let {url, parentFrameId} = details;
+      let frame = {url: new URL(url), parent: null};
+
+      if (parentFrameId != -1)
+      {
+        getFrame(
+          tabId, processId, parentFrameId,
+          parentFrame =>
+          {
+            frame.parent = parentFrame;
+            callback(frame);
+          }
+        );
+      }
+      else
+      {
+        callback(frame);
+      }
+    }
+  );
+}
+
+function checkPotentialPopup(tabId, popup)
+{
+  let urlObj = new URL(popup.url || "about:blank");
   let urlString = stringifyURL(urlObj);
+  let documentHost = extractHostFromFrame(popup.sourceFrame);
   let thirdParty = isThirdParty(urlObj, documentHost);
 
+  let specificOnly = !!checkWhitelisted(
+    popup.sourcePage, popup.sourceFrame,
+    RegExpFilter.typeMap.GENERICBLOCK
+  );
+
   let filter = defaultMatcher.matchesAny(
     urlString, RegExpFilter.typeMap.POPUP,
     documentHost, thirdParty, null, specificOnly
@@ -42,48 +97,72 @@
     chrome.tabs.remove(tabId);
 
   logRequest(
-    sourcePage, urlString, "POPUP", documentHost,
-    thirdParty, null, specificOnly, filter
+    popup.sourcePage, urlString, "POPUP",
+    documentHost, thirdParty, null,
+    specificOnly, filter
   );
 }
 
+function onBeforeRequest(details)
+{
+  let popup = loadingPopups[details.tabId];
+  if (popup)
+  {
+    popup.url = details.url;
+    if (popup.sourceFrame)
+      checkPotentialPopup(details.tabId, popup);
+  }
+}
+
+function onCompleted(details)
+{
+  if (details.frameId == 0 && details.url != "about:blank")
+    forgetPopup(details.tabId);

[Wladimir Palant, 2016/02/12 15:43:43]

Will this cause a memory leak if a website opens an about:blank popup and fills
it via document.write() or similar?

[Sebastian Noack, 2016/02/12 15:52:13]

Yes, just as with the previous implementation. But now we at least cleanup when
the popup is closed.

+}
+
+function onSourceFrameRetrieved(tabId, frame)
+{
+  let popup = loadingPopups[tabId];
+  if (popup)
+  {
+    if (checkWhitelisted(popup.sourcePage, frame))
+    {
+      forgetPopup(tabId);
+    }
+    else
+    {
+      popup.sourceFrame = frame;
+      checkPotentialPopup(tabId, popup);
+    }
+  }
+}
+
 chrome.webNavigation.onCreatedNavigationTarget.addListener(details =>
 {
-  let sourcePage = new ext.Page({id: details.sourceTabId});
-  let sourceFrame = ext.getFrame(details.sourceTabId, details.sourceFrameId);
+  if (!hasLoadingPopups())
+  {
+    chrome.webRequest.onBeforeRequest.addListener(
+      onBeforeRequest,
+      {
+        urls:  ["<all_urls>"],
+        types: ["main_frame"]
+      }
+    );
 
-  if (checkWhitelisted(sourcePage, sourceFrame))
-    return;
-
-  let documentHost = extractHostFromFrame(sourceFrame);
-  if (!documentHost)
-    return;
-
-  let specificOnly = !!checkWhitelisted(sourcePage, sourceFrame,
-                                        RegExpFilter.typeMap.GENERICBLOCK);
-
-  tabsLoading[details.tabId] = {
-    page: sourcePage,
-    documentHost: documentHost,
-    specificOnly: specificOnly
-  };
-  checkPotentialPopup(details.tabId, details.url, sourcePage,
-                      documentHost, specificOnly);
-});
-
-chrome.tabs.onUpdated.addListener((tabId, changeInfo, tab) =>
-{
-  if (!(tabId in tabsLoading))
-    return;
-
-  if ("url" in changeInfo)
-  {
-    let source = tabsLoading[tabId];
-    checkPotentialPopup(tabId, tab.url, source.page,
-                                        source.documentHost,
-                                        source.specificOnly);
+    chrome.webNavigation.onCompleted.addListener(onCompleted);
+    chrome.tabs.onRemoved.addListener(forgetPopup);
   }
 
-  if ("status" in changeInfo && changeInfo.status == "complete" && tab.url != "about:blank")
-    delete tabsLoading[tabId];
+  loadingPopups[details.tabId] = {
+    url: details.url,
+    sourcePage: new ext.Page({id: details.sourceTabId}),
+    sourceFrame: null
+  };
+
+  getFrame(
+    details.sourceTabId,
+    details.sourceProcessId,
+    details.sourceFrameId,
+    onSourceFrameRetrieved.bind(null, details.tabId)
+  );
 });