Requests made from the top frame don't have frameId 0

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: Request Handling
P1
normal
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: April, Assigned: mixedpuppy)

Tracking

(Blocks: 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged, URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

3 months ago
A change in Firefox 55 seems to have broken my WebExtension. It seems that XMLHttpRequests made from the main frame no longer have frameID set to 0, unlike what the MDN and Chrome documentation seems to indicate:

> frameId (integer): Zero if the request happens in the main frame; a positive value is the ID of a subframe in which the request happens. If the document of a (sub-)frame is loaded (type is main_frame or sub_frame), frameId indicates the ID of this frame, not the ID of the outer frame. Frame IDs are unique within a tab.

I use frameId === 0 in Laboratory:
https://addons.mozilla.org/en-US/firefox/addon/laboratory-by-mozilla/

...to determine if an XMLHttpRequest was made by the main frame (and should be logged) or made by a sub frame (such as an iframe) and should be ignored.
(Reporter)

Updated

3 months ago

Updated

3 months ago
Component: WebExtensions: General → WebExtensions: Request Handling
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Assignee: nobody → mixedpuppy
Priority: -- → P1
Whiteboard: triaged
(Assignee)

Updated

3 months ago
Duplicate of this bug: 1369284
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
another push for try, added r?aswan in case he can get to it before kmag.

Comment 7

3 months ago
Changing summary to indicate that this affects all requests - I saw this bug before filing mine but didn't realize that both were referring to the same issue.
Blocks: 1226547
Summary: XMLHttpRequests made from the top frame don't have frameId 0 → Requests made from the top frame don't have frameId 0
Version: unspecified → Trunk

Comment 8

3 months ago
mozreview-review
Comment on attachment 8871928 [details]
Bug 1367138 fix webrequest frameId and parentFrameId,

https://reviewboard.mozilla.org/r/143438/#review148826

::: toolkit/components/extensions/test/mochitest/test_ext_webrequest_frameId.html:96
(Diff revision 4)
> +  for (let i = 0; i < 6; i++) {
> +    checkDetails(await extension.awaitMessage("onBeforeRequest"));
> +  }

This is clunky.  A small improvement would be to change 6 to `Object.keys(expected).length)`, or even better would be to just pass all of `expected` into `checkDetails()` and have it do the iteration itself.
Attachment #8871928 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)

Comment 10

3 months ago
mozreview-review
Comment on attachment 8871928 [details]
Bug 1367138 fix webrequest frameId and parentFrameId,

https://reviewboard.mozilla.org/r/143438/#review148950

::: toolkit/modules/addons/WebRequest.jsm:763
(Diff revisions 4 - 5)
> -      // A mainframe and requests within a mainframe will have:
> +      // A main_frame and requests within a main_frame will have:
>        //   frameOuterWindowID == 0 && outerWindowID == parentOuterWindowID
> -      //   In which case wa want frameId = 0 and parentFrameId = -1
> -      // A subframe has:
> +      //   In which case we want frameId = 0 and parentFrameId = -1
> +      // A sub_frame has:
>        //   frameOuterWindowID != 0 && outerWindowID == parentOuterWindowID
> -      // Requests in a subframe have:
> +      // Nested sub_frames have:
> +      //   frameOuterWindowID != 0 && outerWindowID != parentOuterWindowID
> +      // Requests within a sub_frame have:
>        //   frameOuterWindowID == 0 && outerWindowID != parentOuterWindowID
> -      if (loadInfo.frameOuterWindowID != 0 ||
> -          loadInfo.outerWindowID != loadInfo.parentOuterWindowID) {
> +      //   Further, a request in a sub_frame whos parent is main_frame will
> +      //   have parentOuterWindowID == browser.outerWindowID, in which case
> +      //   set parentWindowId to zero.

I if I understand correctly, I think this could all be more clearly stated as something like:

The main frame will have: ...
A sub frame whose parent is the main frame or a request within such a sub frame will have: ...
A sub frame of a sub frame will have: ...

::: toolkit/modules/addons/WebRequest.jsm:772
(Diff revisions 4 - 5)
>        //   frameOuterWindowID != 0 && outerWindowID == parentOuterWindowID
> -      // Requests in a subframe have:
> +      // Nested sub_frames have:
> +      //   frameOuterWindowID != 0 && outerWindowID != parentOuterWindowID
> +      // Requests within a sub_frame have:
>        //   frameOuterWindowID == 0 && outerWindowID != parentOuterWindowID
> -      if (loadInfo.frameOuterWindowID != 0 ||
> +      //   Further, a request in a sub_frame whos parent is main_frame will

nit: whos -> whose

::: toolkit/modules/addons/WebRequest.jsm:777
(Diff revisions 4 - 5)
> +      let windowId = 0;
> +      let parentWindowId = -1;

I think this would be easier to read if the actual values here were just in an else clause below.

::: toolkit/modules/addons/WebRequest.jsm:781
(Diff revisions 4 - 5)
> +      // Defaults for top level loads (ie. main_frame)
> +      let windowId = 0;
> +      let parentWindowId = -1;
> +
> +      if (loadInfo.frameOuterWindowID != 0) {
> +        windowId = loadInfo.frameOuterWindowID;

No reason not to be verbose here, add a comment that this could be either a sub frame of the main frame (in which case parentWindowId needs to be 0) or something originating inside a sub frame (either a nested frame or some other sort of request) in which case parentWindowId is the id of that sub frame.

::: toolkit/modules/addons/WebRequest.jsm:784
(Diff revisions 4 - 5)
> +
> +      if (loadInfo.frameOuterWindowID != 0) {
> +        windowId = loadInfo.frameOuterWindowID;
> +        parentWindowId = loadInfo.outerWindowID == loadInfo.parentOuterWindowID ? 0 : loadInfo.outerWindowID;
> +      } else if (loadInfo.outerWindowID != loadInfo.parentOuterWindowID) {
> +        let parentMainFrame = data.browser && data.browser.outerWindowID == loadInfo.parentOuterWindowID;

And again, a comment to clarify that this is a request coming from within a sub frame, and we need to check browser.outerWindowId to figure out if the parent frame is the main frame or not.
(Assignee)

Comment 11

3 months ago
mozreview-review-reply
Comment on attachment 8871928 [details]
Bug 1367138 fix webrequest frameId and parentFrameId,

https://reviewboard.mozilla.org/r/143438/#review148950

> I if I understand correctly, I think this could all be more clearly stated as something like:
> 
> The main frame will have: ...
> A sub frame whose parent is the main frame or a request within such a sub frame will have: ...
> A sub frame of a sub frame will have: ...

Some of your later comments illustrated that this was not clear, so I'm reworking the comments and only having them in the blocks.

> I think this would be easier to read if the actual values here were just in an else clause below.

Actually, this was wrong anyway, I accidentally made it possible to not have values for this in details.  I need to move them back up to where they were in the last iteration so defaults are always in details.
Comment hidden (mozreview-request)

Comment 13

3 months ago
mozreview-review
Comment on attachment 8871928 [details]
Bug 1367138 fix webrequest frameId and parentFrameId,

https://reviewboard.mozilla.org/r/143438/#review149278

::: toolkit/modules/addons/WebRequest.jsm:781
(Diff revisions 5 - 6)
> +          parentWindowId: loadInfo.outerWindowID == loadInfo.parentOuterWindowID ? 0 : loadInfo.outerWindowID,
> +        });
>        } else if (loadInfo.outerWindowID != loadInfo.parentOuterWindowID) {
> +        // This is a non-frame (e.g. script, image, etc) request within a
> +        // sub_frame.  We have to check parentOuterWindowID against the browser
> +        // to see if it is the main_frame (thus zero).

If I undestand this correctly, "thus zero" is confusing, it should be something like "in which case the parenteWindowId available to the caller must be set to zero".  The existing comment sounds like you're referring to the value inside loadInfo.
Comment hidden (mozreview-request)

Comment 15

3 months ago
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/71d8ec8f89d4
fix webrequest frameId and parentFrameId, r=aswan

Comment 16

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71d8ec8f89d4
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

Comment 17

3 months ago
Many thanks!
You need to log in before you can comment on or make changes to this bug.