1329299 - chrome.webRequest.onBeforeRequest frameId can be non-zero with type==main_frame

Status: RESOLVED FIXED

(WebExtensions :: Request Handling, defect)

Description

[Alexei]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/54.0.2840.100 Safari/537.36

Steps to reproduce:

Privacy Badger, a popular Firefox addon built newly converted to Web Extensions, is experiencing problems where it appears that chrome.webRequest.onBeforeRequest never fires for frame 0 on pages opened into a new tab (such as by ctrl-clicking links) on Firefox 50.1, and for all pages on Firefox Developer Edition. Since Privacy Badger expects frame 0 (top-level) data to be present, missing frame 0 data breaks the addon.

Please see https://github.com/EFForg/privacybadger/issues/1101 for the original Privacy Badger issue.

Comment 1

[Cooper Quintin]

This is a breaking bug for privacy badger which is affecting a large number of our users.

Comment 2

[Shane Caraveo (:mixedpuppy)]

I want to clarify terminology here.  When you say frame 0, do you mean the top level document, or do you mean window.frames[0]?  I believe you mean the top level document, but I want to be certain.  As well, the last comment in the github issue states it is working in 51, so this would only be a fx50 issue?

Comment 3

[Shane Caraveo (:mixedpuppy)]

see comment 2

Comment 4

[Alexei]

When I say frame 0, I mean the "main frame"/top-level document: https://developer.mozilla.org/en-US/Add-ons/WebExtensions/API/webRequest/onBeforeRequest#details

Comment 5

[Shane Caraveo (:mixedpuppy)]

ok, using a simple background script with:

browser.webRequest.onBeforeRequest.addListener((details) => {
  if (details.type == "main_frame")
  dump(`request ${details.type} ${details.frameId} ${details.url}\n`);
}, {urls: ["<all_urls>"]} , ["blocking"]);

I am able to reproduce a non-zero frameId when type is main_frame.  Per documentation frameId should be zero with main_frame.  

Given that 51 is in final stages for release, and I cannot reproduce on 52/53, I'm inclined to wontfix this.  Addons would need to ignore frameId when type is main_frame for fx50/51.

Comment 6

[Shane Caraveo (:mixedpuppy)]

A touch more background for this bug.  The webrequest listener is indeed called.  The actual problem in the addon is that they *correctly* expect frameId is zero when type is main_frame.  Changes we've worked on in WebRequest code probably fixed this as a side affect.

Comment 7

[Shane Caraveo (:mixedpuppy)]

Attachment: Bug 1329299 verify frameId in webrequest tests,

Review commit: https://reviewboard.mozilla.org/r/103674/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/103674/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

https://reviewboard.mozilla.org/r/103674/#review104338

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b446f3bf75e4
verify frameId in webrequest tests, r=kmag

Comment 10

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

Approval Request Comment
[Feature/Bug causing the regression]: webrequest
[User impact if declined]: possible bug affecting addons, this is a test to check for that.  bug affects 50/51 but not manually reproducible in 52/53.  This test is to make sure it stays that way.
[Is this code covered by automated tests?]: yes, new test
[Has the fix been verified in Nightly?]: just landed
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: test only, no
[Why is the change risky/not risky?]: test only change
[String changes made/needed]: none

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I had to back this out for test bustage like https://treeherder.mozilla.org/logviewer.html#?job_id=67789870&repo=autoland

https://hg.mozilla.org/integration/autoland/rev/639010128e5e

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

Test-only changes are auto-approved for uplift.

Comment 13

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103674/diff/1-2/

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

https://reviewboard.mozilla.org/r/103674/#review104394

r=me

Comment 15

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103674/diff/2-3/

Comment 16

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/103674/diff/3-4/

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f15661d44c76
verify frameId in webrequest tests, r=kmag

Comment 18

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/f15661d44c76

Comment 19

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

Approval Request Comment
[Feature/Bug causing the regression]: webrequest
[User impact if declined]: affects some addons that rely on frameId as documented
[Is this code covered by automated tests?]: yes, new test
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: very small change with test coverage
[String changes made/needed]: none

Comment 20

[Julien Cristau [:jcristau] (back April 22)]

Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

webextensions fix for aurora52

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d49507cffc8a