Issue 29350357: Issue 4386 - Fixed determining document domain, particularly after being redirected (Closed)
|
Created:
Aug. 31, 2016, 3:11 p.m. by Wladimir Palant Modified:
Sept. 5, 2016, 5:10 p.m. Visibility:
Public. |
DescriptionIssue 4386 - Fixed determining document domain, particularly after being redirected
Repository: hg.adblockplus.org/adblockpluschrome
Patch Set 1 #
Total comments: 11
Patch Set 2 : Fixed variable names #Patch Set 3 : Fixed another regression from last-minute changes #MessagesTotal messages: 10
Your code Looks good to Dave apart from a couple of nits. As for if this change is doing the correct thing and using the correct events, I'm not sure yet. Problem is I need to stop for the evening now, I've run out of time. I can try to figure that out tomorrow, or if you guys want to go ahead without me I don't mind either. https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:121: frame = frames[details.frameId] = {}; Nit: Seems inconsistent to do `Object.create(null)` above and `{}` here. https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; 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.)
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:121: frame = frames[details.frameId] = {}; On 2016/08/31 16:06:39, kzar wrote: > Nit: Seems inconsistent to do `Object.create(null)` above and `{}` here. Yes, seems inconsistent but other than the frames object this one isn't a lookup table, so it can be a regular object. https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; On 2016/08/31 16:06:39, kzar wrote: > 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? Frankly, I think that this would specialize the function too much and also make it less obvious that we are manipulating object properties here.
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:115: var frames = framesOfTabs[details.tabId]; It should be `tabId` and `frameId` rather than `details.tabId` or `details.frameId` in this function right? https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:121: frame = frames[details.frameId] = {}; On 2016/08/31 19:13:44, Wladimir Palant wrote: > On 2016/08/31 16:06:39, kzar wrote: > > Nit: Seems inconsistent to do `Object.create(null)` above and `{}` here. > > Yes, seems inconsistent but other than the frames object this one isn't a lookup > table, so it can be a regular object. Acknowledged. https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; On 2016/08/31 19:13:44, Wladimir Palant wrote: > On 2016/08/31 16:06:39, kzar wrote: > > 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? > > Frankly, I think that this would specialize the function too much and also make > it less obvious that we are manipulating object properties here. Acknowledged.
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; 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.
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; On 2016/09/01 12:46:15, kzar wrote: > 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. 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...
https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:130: frame.parent = frames[details.parentFrameId] || null; On 2016/09/01 13:22:57, kzar wrote: > On 2016/09/01 12:46:15, kzar wrote: > > 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. > > 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... 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/
I only fixed the one issue below for now. The fix isn't entirely working for some reason, I see this as well - still investigating. https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... File chrome/ext/background.js (right): https://codereview.adblockplus.org/29350357/diff/29350358/chrome/ext/backgrou... chrome/ext/background.js:115: var frames = framesOfTabs[details.tabId]; On 2016/09/01 11:06:48, kzar wrote: > It should be `tabId` and `frameId` rather than `details.tabId` or > `details.frameId` in this function right? Ouch, not sure how I missed that in the testing. Fixed.
It seems that I tested my changes, then cleaned up the code a bit and somehow failed to test it again - probably didn't update the development environment when testing. Either way, another stupid mistake and now this seems to work correctly.
LGTM 
|
