April King [:April] Reporter
	Description • 7 years 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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 6 • 7 years ago

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

	Wladimir Palant
	Comment 7 • 7 years 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.

	Andrew Swan [:aswan]
	Comment 8 • 7 years 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.

	Andrew Swan [:aswan]
	Comment 10 • 7 years 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.

	Shane Caraveo (:mixedpuppy) Assignee
	Comment 11 • 7 years 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.

	Andrew Swan [:aswan]
	Comment 13 • 7 years 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.

	Pulsebot
	Comment 15 • 7 years ago

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

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 16 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/71d8ec8f89d4

	Ed Morley [:emorley]
	Comment 17 • 7 years ago

Many thanks!