Closed Bug 1329299 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Request Handling, defect)

50 Branch
defect
Not set

Tracking

(firefox50 wontfix, firefox51 wontfix, firefox52 fixed, firefox53 fixed)

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- wontfix
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: alexeiatyahoodotcom+mzllbgzll, Assigned: mixedpuppy)

Details

Attachments

(1 file)

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.
Component: Untriaged → WebExtensions: Untriaged
Product: Firefox → Toolkit
This is a breaking bug for privacy badger which is affecting a large number of our users.
Assignee: nobody → mixedpuppy
Component: WebExtensions: Untriaged → WebExtensions: Request Handling
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?
see comment 2
Flags: needinfo?(alexeiatyahoodotcom+mozilla)
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
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: chrome.webRequest.onBeforeRequest doesn't fire for frame 0 → chrome.webRequest.onBeforeRequest frameId can be non-zero with type==main_frame
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 on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

https://reviewboard.mozilla.org/r/103674/#review104338
Attachment #8825552 - Flags: review?(kmaglione+bmo) → review+
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/b446f3bf75e4
verify frameId in webrequest tests, r=kmag
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
Attachment #8825552 - Flags: approval-mozilla-aurora?
Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

Test-only changes are auto-approved for uplift.
Attachment #8825552 - Flags: approval-mozilla-aurora?
Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

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

r=me
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/f15661d44c76
verify frameId in webrequest tests, r=kmag
https://hg.mozilla.org/mozilla-central/rev/f15661d44c76
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Flags: needinfo?(alexeiatyahoodotcom+mzllbgzll)
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
Flags: needinfo?(mixedpuppy)
Attachment #8825552 - Flags: approval-mozilla-aurora?
Comment on attachment 8825552 [details]
Bug 1329299 verify frameId in webrequest tests,

webextensions fix for aurora52
Attachment #8825552 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.