Issue 4386 - Fixed determining document domain, particularly after being redirected

chrome/ext/background.js

Index: chrome/ext/background.js
===================================================================
--- a/chrome/ext/background.js
+++ b/chrome/ext/background.js
@@ -105,18 +105,38 @@
   };
 
   chrome.tabs.onUpdated.addListener(function(tabId, changeInfo, tab)
   {
     if (changeInfo.status == "loading")
       ext.pages.onLoading._dispatch(new Page(tab));
   });
 
+  function createFrame(tabId, frameId)
+  {
+    var frames = framesOfTabs[details.tabId];

[kzar, 2016/09/01 11:06:48]

It should be `tabId` and `frameId` rather than `details.tabId` or
`details.frameId` in this function right?

[Wladimir Palant, 2016/09/05 15:48:25]

Ouch, not sure how I missed that in the testing. Fixed.

+    if (!frames)
+      frames = framesOfTabs[details.tabId] = Object.create(null);
+
+    var frame = frames[details.frameId];
+    if (!frame)
+      frame = frames[details.frameId] = {};

[kzar, 2016/08/31 16:06:39]

Nit: Seems inconsistent to do `Object.create(null)` above and `{}` here.

[Wladimir Palant, 2016/08/31 19:13:44]

Yes, seems inconsistent but other than the frames object this one isn't a lookup
table, so it can be a regular object.

[kzar, 2016/09/01 11:06:48]

Acknowledged.

+
+    return frame;
+  }
+
   chrome.webNavigation.onBeforeNavigate.addListener(function(details)
   {
+    // Capture parent frame here because onCommitted doesn't get this info.
+    var frame = createFrame(details.tabId, details.frameId);
+    frame.parent = frames[details.parentFrameId] || null;

[kzar, 2016/08/31 16:06:39]

Since both times we use createFrame we just want to assign a property to the
frame, perhaps we could make the function do that work for us?

createFrame(details.tabId, details.frameId, "parent", 
            frames[details.parentFrameId] || null);

(Of course then perhaps the function should have a different name too, but I
don't have any better ideas.)

[Wladimir Palant, 2016/08/31 19:13:44]

Frankly, I think that this would specialize the function too much and also make
it less obvious that we are manipulating object properties here.

[kzar, 2016/09/01 11:06:48]

Acknowledged.

[kzar, 2016/09/01 12:46:15]

We don't assign the frame's URL here in case the navigation isn't completed.
That makes sense, we certainly don't want to overwrite an existing frame URL in
case the navigation never actually happens. (Like a pre-rendered frame for a URL
which the user starts typing but then stops.)

On the other hand I worry that we might try to access the frame's URL in between
when the onBeforeNavigate event fires and the onCommitted one does. Especially a
problem if the navigation never finished and so the onCommitted event never
fired!

So how about we assign the frame's URL here in the onBeforeNavigate listener,
but only if the frame doesn't already have an URL assigned? That way we would
know the frame would always have an URL, but we wouldn't risk overwriting a URL
too soon. The onCommitted listener would always set the frame URL too, so if the
page is redirected or the URL changed we'd still end up with the correct URL,
but only at the point where we know the navigation is really happening.

[kzar, 2016/09/01 13:22:57]

OK it turns out both onCommitted and onBeforeNavigate can fire for pre-rendered
pages before the user ever completes the navigation... so my assumptions were
wrong.

Also it turns out the fix doesn't seem to work, I still see ads being blocked
for a whitelisted domain when the page is redirected there.

Continuing to investigate...

[kzar, 2016/09/01 13:39:13]

I have something working now, I hope you don't mind I opened a new review since
I can't update yours: https://codereview.adblockplus.org/29350379/

+  });
+
+  chrome.webNavigation.onCommitted.addListener(function(details)
+  {
     if (details.frameId == 0)
     {
       ext._removeFromAllPageMaps(details.tabId);
 
       chrome.tabs.get(details.tabId, function()
       {
         // If the tab is prerendered, chrome.tabs.get() sets
         // chrome.runtime.lastError and we have to dispatch the onLoading event,
@@ -131,25 +151,19 @@
               id: details.tabId,
               url: details.url
             })
           );
         }
       });
     }
 
-    // Add or update frame in frame structure
-    var frames = framesOfTabs[details.tabId];
-    if (!frames)
-      frames = framesOfTabs[details.tabId] = Object.create(null);
-
-    frames[details.frameId] = {
-      parent: frames[details.parentFrameId] || null,
-      url: new URL(details.url)
-    };
+    // Update frame URL in frame structure
+    var frame = createFrame(details.tabId, details.frameId);
+    frame.url = new URL(details.url);
   });
 
   function forgetTab(tabId)
   {
     ext.pages.onRemoved._dispatch(tabId);
 
     ext._removeFromAllPageMaps(tabId);
     delete framesOfTabs[tabId];