Issue 6595 - Make sure frame structure gets always updated

ext/background.js

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-present eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
@@ skipping 241 matching lines @@
           return;
       }
     }
 
     updatePageFrameStructure(details.frameId, details.tabId, details.url,
                              details.parentFrameId);
   },
   {types: ["main_frame", "sub_frame"], urls: ["http://*/*", "https://*/*"]},
   ["responseHeaders"]);
 
-  browser.webNavigation.onBeforeNavigate.addListener(details =>
+  browser.webNavigation.onCommitted.addListener(details =>
   {
-    // Since we can only listen for HTTP(S) responses using
-    // webRequest.onHeadersReceived we must update the page structure here for
-    // other navigations.
-    let {url} = details;
-    if (!(url.startsWith("http:") ||
-          url.startsWith("https:") &&

[Sebastian Noack, 2018/04/18 11:25:36]

The assumption that every document with an HTTP(S) URL is handled by the
onHeadersReceived logic above was incorrect (latest now with Service Workers).

Instead, I'm now updating the frame structure on navigation, regardless of the
protocol, but only if the frame is either unknown or has a different URL than
what we recorded before.

[kzar, 2018/05/02 12:06:52]

Initially I worried you removed the whole onHeadersReceived workaround,
replacing it with onCommitted again but I see you're not doing that now. I feel
a lot happier about your change now.

I still worry about requests made by the document (not caught by
onBeforeNavigate) which were made before the navigation completed, but I guess
that's not as important since we are still using onHeadersReceived for the
common case. But can I ask, why did you switch from onBeforeNavigate to
onCommitted? Wouldn't this approach still work using onBeforeNavigate?

[Sebastian Noack, 2018/05/02 14:32:15]

During onBeforeNavigate we don't know yet whether a navigation will actually
take place (e.g. if the response is treated as a download rather than providing
content to replace the current document), which is the reason we do that
ridiculous dance in the onHeadersReceived listener.

Before my change, we only handled navigation to non-HTTP(S) URLs here, and
therefore assumed that those always result into navigation (which might not even
have been true). But latest now where we handle requests with any kind of URL
here, we have to be sure that navigation actually happened.

[kzar, 2018/05/02 15:26:52]

OK, yea that makes sense. I can't think of a better idea either.

I wonder if it matters that blob "requests" will also be caught by this listener
now? IIRC the Chrome APIs won't give much information on a tab for a blob
"request" and the parentFrameId won't be given.

[Sebastian Noack, 2018/05/02 15:34:49]

Well, if blob: URLs are opaque to the webRequest API anyway, it shouldn't matter
whether we update the frame hierarchy for frames loaded from a blob: URL here or
not. Anyway, do you have a website at hand that shows frames with content loaded
from a blob: URL? Then I can quickly test what happens.

[kzar, 2018/05/02 15:46:35]

Well if you paste this snippet from my Chromium issue into the console for a
page (which doesn't have a CSP that clashes) you'll be able to see what happens:

let frame = document.createElement("iframe");
frame.src = URL.createObjectURL(new Blob([""]));
document.body.appendChild(frame)

[Sebastian Noack, 2018/05/02 16:27:55]

I tested it, and the onCommitted listener is emitted for the frame with blob:
URL causing the frame to be recorded (as expected), which has no visible effect.

I will push the changes now to a "next" bookmark.

-          // Chrome doesn't dispatch webRequest.onHeadersReceived
-          // for Web Store URLs.
-          // https://crrev.com/76882bf/extensions/common/extension_urls.cc#33
-          !url.startsWith("https://chrome.google.com/webstore/")))

[Sebastian Noack, 2018/04/18 11:25:36]

This special case is redundant now, since onCommitted doesn't fire for these
URLs, in the first place.

[kzar, 2018/05/02 12:06:52]

Acknowledged.

+    // We have to update the frame structure for documents that weren't
+    // loaded over HTTP (including documents cached by Service Workers),
+    // when the navigation occurs. However, we must be careful to not
+    // update the state of the same document twice, otherewise the number
+    // of any ads blocked already and any recorded sitekey could get lost.
+    let frame = ext.getFrame(details.tabId, details.frameId);
+    if (!frame || frame.url.href != details.url)
     {
-      updatePageFrameStructure(details.frameId, details.tabId, url,
+      updatePageFrameStructure(details.frameId, details.tabId, details.url,
                                details.parentFrameId);
     }
   });
 
   function forgetTab(tabId)
   {
     ext.pages.onRemoved._dispatch(tabId);
 
     removeFromAllPageMaps(tabId);
     framesOfTabs.delete(tabId);
@@ skipping 319 matching lines @@
   ext.windows = {
     create(createData, callback)
     {
       browser.windows.create(createData, createdWindow =>
       {
         afterTabLoaded(callback)(createdWindow.tabs[0]);
       });
     }
   };
 }