Issue 29755575: Issue 6595 - Make sure frame structure gets always updated (Closed)
|
Created:
April 18, 2018, 11:16 a.m. by Sebastian Noack Modified:
May 2, 2018, 4:29 p.m. Reviewers:
kzar CC:
Manish Jethani Visibility:
Public. |
DescriptionIssue 6595 - Make sure frame structure gets always updated
Patch Set 1 #
Total comments: 9
MessagesTotal messages: 7
https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:269: url.startsWith("https:") && 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. https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:273: !url.startsWith("https://chrome.google.com/webstore/"))) This special case is redundant now, since onCommitted doesn't fire for these URLs, in the first place.
https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:269: url.startsWith("https:") && On 2018/04/18 11:25:36, Sebastian Noack wrote: > 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. 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? https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:273: !url.startsWith("https://chrome.google.com/webstore/"))) On 2018/04/18 11:25:36, Sebastian Noack wrote: > This special case is redundant now, since onCommitted doesn't fire for these > URLs, in the first place. Acknowledged.
https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:269: url.startsWith("https:") && On 2018/05/02 12:06:52, kzar wrote: > But can I ask, why did you switch from onBeforeNavigate to > onCommitted? Wouldn't this approach still work using onBeforeNavigate? 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.
Assuming you tested that things don't explode in Chrome when the onCommitted listener fires for blob "requests" then this LGTM. (If it helps to test I filed a Chromium issue a while back that has some snippets: https://bugs.chromium.org/p/chromium/issues/detail?id=704065 ) https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:269: url.startsWith("https:") && On 2018/05/02 14:32:15, Sebastian Noack wrote: > On 2018/05/02 12:06:52, kzar wrote: > > But can I ask, why did you switch from onBeforeNavigate to > > onCommitted? Wouldn't this approach still work using onBeforeNavigate? > > 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. 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.
https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:269: url.startsWith("https:") && On 2018/05/02 15:26:52, kzar wrote: > 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. 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.
https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:269: url.startsWith("https:") && On 2018/05/02 15:34:49, Sebastian Noack wrote: > 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. 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)
https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js File ext/background.js (left): https://codereview.adblockplus.org/29755575/diff/29755576/ext/background.js#o... ext/background.js:269: url.startsWith("https:") && On 2018/05/02 15:46:35, kzar wrote: > On 2018/05/02 15:34:49, Sebastian Noack wrote: > > 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. > > 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) 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. 
|
||||||||||||||||||||||||||||||||||||||||||||||
