Issue 6953 - Update frame structure for data URI frames

Issue 6953 - Update frame structure for data URI frames

[Manish Jethani, 2018-09-16 14:11:32]

Patch Set 1

[Manish Jethani, 2018-09-16 14:14:14]

On 2018/09/16 14:11:32, Manish Jethani wrote:
> Patch Set 1

Note that it is in fact debatable whether ads should be whitelisted in data URI
frames. Chrome doesn't execute content scripts in data URI frames, for example,
but does in about frames. Nevertheless, the frame structure should be updated
here regardless and the question of how whitelisting should work in data URI
frames should be addressed elsewhere (mostly likely in checkWhitelisted).

[Sebastian Noack, 2018-09-17 16:02:31]

Assuming onBeforeNavigate is triggered for data: URLs, and that you reproduced a
scenario in which onCommitted being triggered for a data: URL after sub-requests
already occurred, I agree to the approach. But IIRC, we had slightly different
assumptions when we touched this code in the past. So we might want to double
check carefully.

https://codereview.adblockplus.org/29882555/diff/29882556/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29882555/diff/29882556/ext/background.js#n...
ext/background.js:257: if (/^(about|data):/.test(details.url))
Nit: I'm not sure if a regular expression is justified here.

[Manish Jethani, 2018-09-21 12:48:18]

On 2018/09/17 16:02:31, Sebastian Noack wrote:
> Assuming onBeforeNavigate is triggered for data: URLs, and that you reproduced
a
> scenario in which onCommitted being triggered for a data: URL after
sub-requests
> already occurred, I agree to the approach. But IIRC, we had slightly different
> assumptions when we touched this code in the past. So we might want to double
> check carefully.

OK, so it seems that the real issue is that onCommitted is not supposed to get a
parentFrameId, it's always undefined.

https://developer.chrome.com/extensions/webNavigation#event-onCommitted
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/we...

This means it's probably not working for HTTP URIs either, but I'll check.

onCommitted does get triggered for data URI frames before sub-requests so we
should be OK over there.

[Manish Jethani, 2018-09-21 20:29:05]

On 2018/09/21 12:48:18, Manish Jethani wrote:
> On 2018/09/17 16:02:31, Sebastian Noack wrote:
> > Assuming onBeforeNavigate is triggered for data: URLs, and that you
reproduced
> a
> > scenario in which onCommitted being triggered for a data: URL after
> sub-requests
> > already occurred, I agree to the approach. But IIRC, we had slightly
different
> > assumptions when we touched this code in the past. So we might want to
double
> > check carefully.
> 
> OK, so it seems that the real issue is that onCommitted is not supposed to get
a
> parentFrameId, it's always undefined.
> 
> https://developer.chrome.com/extensions/webNavigation#event-onCommitted
>
https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/we...
> 
> This means it's probably not working for HTTP URIs either, but I'll check.
> 
> onCommitted does get triggered for data URI frames before sub-requests so we
> should be OK over there.

Alright, it's confirmed now. onCommitted doesn't have parentFrameId even for
HTTP URIs. The only reason it works is that onHeadersReceived does have
parentFrameId.

Since we won't be receiving headers for data: URIs, it makes sense to handle
this in onBeforeNavigate (similar to about: URIs).

[Manish Jethani, 2018-09-21 20:32:24]

Patch Set 2: Use String.startsWith

https://codereview.adblockplus.org/29882555/diff/29882556/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29882555/diff/29882556/ext/background.js#n...
ext/background.js:257: if (/^(about|data):/.test(details.url))
On 2018/09/17 16:02:31, Sebastian Noack wrote:
> Nit: I'm not sure if a regular expression is justified here.

Changed to String.startsWith.

I have a hunch that the regular expression is better choice though, but I'll
leave it like it is for now.

[Manish Jethani, 2018-09-22 15:56:51]

https://codereview.adblockplus.org/29882555/diff/29887602/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29882555/diff/29887602/ext/background.js#n...
ext/background.js:254: // Requests can be made by about: and data: frames before
the frame's
Oh, I think I should update this comment. The real issue, as I mentioned
earlier, is that both about: and data: frames don't get onHeadersReceived (of
course), and onCommitted doesn't have parentFrameId, so the frame is never set
up.

[Manish Jethani, 2018-09-25 16:07:19]

Patch Set 3: Update comment

https://codereview.adblockplus.org/29882555/diff/29887602/ext/background.js
File ext/background.js (right):

https://codereview.adblockplus.org/29882555/diff/29887602/ext/background.js#n...
ext/background.js:254: // Requests can be made by about: and data: frames before
the frame's
On 2018/09/22 15:56:51, Manish Jethani wrote:
> Oh, I think I should update this comment. The real issue, as I mentioned
> earlier, is that both about: and data: frames don't get onHeadersReceived (of
> course), and onCommitted doesn't have parentFrameId, so the frame is never set
> up.

Done.

[Manish Jethani, 2018-09-25 16:09:03]

Patch Set 4: Remove mistakenly uploaded dependencies file

[Sebastian Noack, 2018-09-25 16:47:56]

LGTM

[Manish Jethani, 2018-09-26 12:44:33]

On 2018/09/25 16:47:56, Sebastian Noack wrote:
> LGTM

Should I land this in next?

[Sebastian Noack, 2018-09-26 13:09:11]

On 2018/09/26 12:44:33, Manish Jethani wrote:
> Should I land this in next?

Yes, please. Thanks.