Closed
Bug 1367138
Opened 7 years ago
Closed 7 years ago
Requests made from the top frame don't have frameId 0
Categories
(WebExtensions :: Request Handling, defect, P1)
WebExtensions
Request Handling
Tracking
(firefox55 fixed)
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: April, Assigned: mixedpuppy)
References
()
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.
Reporter | ||
Updated•7 years ago
|
Updated•7 years ago
|
Component: WebExtensions: General → WebExtensions: Request Handling
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mixedpuppy
Priority: -- → P1
Whiteboard: triaged
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
another push for try, added r?aswan in case he can get to it before kmag.
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.
Blocks: webext-port-abp
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•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.
Attachment #8871928 -
Flags: review?(aswan) → review+
Comment hidden (mozreview-request) |
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.
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.
Comment hidden (mozreview-request) |
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.
Comment hidden (mozreview-request) |
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
Comment 16•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/71d8ec8f89d4
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 17•7 years ago
|
||
Many thanks!
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•