Get rid of top-level window ID tracking

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
WebExtensions: General
RESOLVED FIXED
a year ago
4 months ago

People

(Reporter: kmag, Assigned: kmag)

Tracking

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

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

Attachments

(1 attachment)

Comment hidden (empty)
Comment hidden (mozreview-request)
Attachment #8856553 - Flags: review?(mixedpuppy) → review?(aswan)

Updated

a year ago
Whiteboard: triaged

Comment 2

a year ago
mozreview-review
Comment on attachment 8856553 [details]
Bug 1355120: Get rid of top-level window ID tracking.

https://reviewboard.mozilla.org/r/128516/#review131656

Okay, this makes sense.  Why was the code originally written with the tracking in the parent process?  It certainly does seem unnecessarily awkward and potentially racy.
Attachment #8856553 - Flags: review?(aswan) → review+
(Assignee)

Comment 3

a year ago
mozreview-review-reply
Comment on attachment 8856553 [details]
Bug 1355120: Get rid of top-level window ID tracking.

https://reviewboard.mozilla.org/r/128516/#review131656

It's mostly just outdated at this point. I'm not sure why it was originally written this way, but this code is really old, and we've already moved most of the frame ID mapping into the content process.

Comment 6

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/ddd77f9cf071
https://hg.mozilla.org/mozilla-central/rev/0b45ade824ac
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Hi!

April's CSP-generator addon broke with this change, since `frameId` as reported by browser.webRequest.onResponseStarted() used to be zero for eg xmlhttprequest made by the main frame, whereas now it's non-zero, like so:

{
  "requestId": "6166",
  "url": "https://brasstacks.mozilla.com/orangefactor/api/bybug?startday=2017-05-16&endday=2017-05-23&tree=trunk",
  "originUrl": "https://brasstacks.mozilla.com/orangefactor/",
  "documentUrl": "https://brasstacks.mozilla.com/orangefactor/",
  "method": "GET",
  "tabId": 49,
  "type": "xmlhttprequest",
  "timeStamp": 1495493952888,
  "frameId": 6442451022,
  "parentFrameId": 6442451022,
  "fromCache": false,
  "ip": "63.245.215.33",
  "statusCode": 200,
  "statusLine": "HTTP/2.0 200 OK"
}

Was the landing here meant to cause this change to `frameId`?

If so, how should an addon be checking whether an xmlhttprequest (or other) belongs to the main frame or an iframe?

For more info see:
https://github.com/april/laboratory/issues/6

Many thanks!
Flags: needinfo?(kmaglione+bmo)
I imagine there are workarounds, either involving documentUrl or connecting back to the tab, but connecting back to the tab will likely be significantly slower for something that might be processing hundreds or thousands of requests.

I'm looking at MDN's documentation on onResponseStarted and here is what the documentation on frameId states:

> 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.

Chrome's documentation also seems to state that frameId should be 0 for requests made from the main frame. Perhaps I'm misunderstanding what this means?

Also, I'm not sure what details.documentUrl is -- the documentation on MDN doesn't seem to acknowledge that it exists at all.

Thanks!

Updated

a year ago
Depends on: 1367138
Comment on attachment 8856553 [details]
Bug 1355120: Get rid of top-level window ID tracking.

https://reviewboard.mozilla.org/r/128516/#review164790

::: browser/base/content/browser.js:83
(Diff revision 1)
>    ["TelemetryStopwatch", "resource://gre/modules/TelemetryStopwatch.jsm"],
>    ["Translation", "resource:///modules/translation/Translation.jsm"],
>    ["UITour", "resource:///modules/UITour.jsm"],
>    ["UpdateUtils", "resource://gre/modules/UpdateUtils.jsm"],
>    ["Weave", "resource://services-sync/main.js"],
> +  ["WebNavigationFrames", "resource://gre/modules/WebNavigationFrames.js"],

There's a typo on this line, .js instead of .jsm; it never worked.

This bug was hidden by the getter from nsContextMenu.js (same scope) which was overwritting this broken getter.

I'll fix it in bug 1381853 where loading nsContextMenu.js lazily causes tests to fail due to this typo.

Comment 10

8 months ago
The needinfo was resolved by the fix in bug 1367138.
Flags: needinfo?(kmaglione+bmo)
You need to log in before you can comment on or make changes to this bug.