1355120 - Get rid of top-level window ID tracking

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement)

Comment 1

[Kris Maglione [:kmag]]

Attachment: Bug 1355120: Get rid of top-level window ID tracking.

Review commit: https://reviewboard.mozilla.org/r/128516/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/128516/

Comment 2

[Andrew Swan [:aswan]]

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.

Comment 3

[Kris Maglione [:kmag]]

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 4

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddd77f9cf071dc1d40debb288ff632c0d7c64410
Bug 1355120: Get rid of top-level window ID tracking. r=aswan

Comment 5

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0b45ade824ac4e8ce701468eb6b7bc77b0b7f273
Bug 1355120: Follow-up: Disable old WebNavigation.jsm tests. r=bustage

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ddd77f9cf071
https://hg.mozilla.org/mozilla-central/rev/0b45ade824ac

Comment 7

[Ed Morley [:emorley]]

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!

Comment 8

[April King [:April]]

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!

Comment 9

[Florian Quèze [:florian]]

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

[Ed Morley [:emorley]]

The needinfo was resolved by the fix in bug 1367138.