Closed Bug 1367138 Opened 3 years ago Closed 3 years ago

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

Categories

(WebExtensions :: Request Handling, defect, P1)

defect

Tracking

(firefox55 fixed)

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: April, Assigned: mixedpuppy)

References

(Blocks 1 open bug, )

Details

(Whiteboard: triaged)

Attachments

(1 file)

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.
Component: WebExtensions: General → WebExtensions: Request Handling
Assignee: nobody → mixedpuppy
Priority: -- → P1
Whiteboard: triaged
Duplicate of this bug: 1369284
another push for try, added r?aswan in case he can get to it before kmag.
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.
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 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 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.
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 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.
Pushed by mixedpuppy@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/71d8ec8f89d4
fix webrequest frameId and parentFrameId, r=aswan
https://hg.mozilla.org/mozilla-central/rev/71d8ec8f89d4
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Many thanks!
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.