Workaround for Chrome not specifying which request is main_frame under some circumstances

Workaround for Chrome not specifying which request is main_frame under some circumstances

[Thomas Greiner, 2013-08-28 13:07:06]

It's safer to overwrite the type value.

[Felix Dahlke, 2013-08-28 13:14:50]

LGTM

[Wladimir Palant, 2013-08-29 05:57:40]

Browser bugs are a PITA, just working around the issue you are seeing is
definitely not sufficient. The usual approach is:

1) Try to understand the issue. Ideally you can find it in the browser's code,
more often you will merely create a reduced testcase however (one that allows
reproducing the issue reliably) and experiment with it to figure out what kind
of bug we are dealing with. Finding all the conditions leading to the bug is
important - there might be more to it than what's obvious from the initial
report, you want to consider that in your workaround.

2) File a browser bug if you cannot find one that matches. This is the only way
to make sure you can remove the workaround at some point - and to learn when it
can be removed. Also, you can point to that bug from the code as well as send
users complaining to the right place.

From what I can tell, neither has been done so far.

http://codereview.adblockplus.org/11579001/diff/3001/webrequest.js
File webrequest.js (right):

http://codereview.adblockplus.org/11579001/diff/3001/webrequest.js#newcode64
webrequest.js:64: type = "main_frame";
There are two assumption here which both seem to be wrong:

1) This issue will always manifest itself as an unknown tab ID. I doubt that
this is the case - if the user navigates to that YouTube URL in an existing tab
then the tab will be known.

2) A request initiated from an unknown tab is always this issue. However, we
will only get the top request if it comes from HTTP or HTTPS. A local file
making requests to remote resources will show up as an unknown tab. Also, some
requests (like requests made by extensions) aren't associated with tabs - tabId
will be -1 then.

[Felix Dahlke, 2013-08-29 07:29:15]

On 2013/08/29 05:57:40, Wladimir Palant wrote:
> From what I can tell, neither has been done so far.

No, it's a workaround, I didn't think we'd want to do such an in-depth analysis
upfront. Reporting a bug definitely makes sense, but doesn't seem pressing -
we'll need a workaround anyway.

> http://codereview.adblockplus.org/11579001/diff/3001/webrequest.js
> File webrequest.js (right):
> 
> http://codereview.adblockplus.org/11579001/diff/3001/webrequest.js#newcode64
> webrequest.js:64: type = "main_frame";
> There are two assumption here which both seem to be wrong:
> 
> 1) This issue will always manifest itself as an unknown tab ID. I doubt that
> this is the case - if the user navigates to that YouTube URL in an existing
tab
> then the tab will be known.

That's true, we discussed this a bit. It seems like this issue in particular is
caused by YouTube videos opened in a new tab (using the YouTube button on an
embedded video), so this handles the issue at hand completely.

> 2) A request initiated from an unknown tab is always this issue. However, we
> will only get the top request if it comes from HTTP or HTTPS. A local file
> making requests to remote resources will show up as an unknown tab. Also, some
> requests (like requests made by extensions) aren't associated with tabs -
tabId
> will be -1 then.

Hm, I wasn't aware of this. Does that mean every request from a local file or
extensions will have type "main_frame" now?

[Wladimir Palant, 2013-08-29 08:15:42]

On 2013/08/29 07:29:15, Felix H. Dahlke wrote:
> That's true, we discussed this a bit. It seems like this issue in particular
is
> caused by YouTube videos opened in a new tab (using the YouTube button on an
> embedded video), so this handles the issue at hand completely.

I don't like the idea of a YouTube-specific workaround. Flash can open links in
the current tab as well, that scenario is likely also problematic.

> Hm, I wasn't aware of this. Does that mean every request from a local file or
> extensions will have type "main_frame" now?

Only the first one. All the following requests will be assumed to have been
requested by a bogus main URL.

[Wladimir Palant, 2013-08-29 08:22:04]

On http://www.fotoplayer.com/v4/index.html I could verify that Flash links
opening in current tab also get type "object" (select some photo and click the
download button).

[Wladimir Palant, 2013-08-29 08:26:15]

Actually, http://demo.testfire.net/vulnerable.swf is a better test case. Both
getURL(blank) and getURL(parent) produce request type "object" instead of
"main_frame". However, tabId is known for the former.

[Thomas Greiner, 2013-08-30 08:31:20]

Made changes according to feedback from intraforum and added reference to the
related issue report.

[Wladimir Palant, 2013-08-30 12:16:25]

http://codereview.adblockplus.org/11579001/diff/4/webrequest.js
File webrequest.js (right):

http://codereview.adblockplus.org/11579001/diff/4/webrequest.js#newcode63
webrequest.js:63: if (details.frameId == 0 && !(details.tabId in frames) && type
== "object")
Please ensure details.tabId >= 0, tabId -1 means a request that isn't associated
with any tab (e.g. extension).

[Thomas Greiner, 2013-08-30 12:18:18]

On 2013/08/30 12:16:25, Wladimir Palant wrote:
> http://codereview.adblockplus.org/11579001/diff/4/webrequest.js
> File webrequest.js (right):
> 
> http://codereview.adblockplus.org/11579001/diff/4/webrequest.js#newcode63
> webrequest.js:63: if (details.frameId == 0 && !(details.tabId in frames) &&
type
> == "object")
> Please ensure details.tabId >= 0, tabId -1 means a request that isn't
associated
> with any tab (e.g. extension).

This check already exists on line 55. Do you want me to add it there as well?

[Wladimir Palant, 2013-08-30 12:28:47]

> This check already exists on line 55.

You are right, missed that. LGTM then.

[Felix Dahlke, 2013-08-30 12:40:57]

LGTM