SessionStateAggregator.js imports Timer.jsm into frame script 'this' scope
Categories
(GeckoView :: General, task, P3)
Tracking
(firefox-esr91 wontfix, firefox100 wontfix, firefox101 wontfix, firefox102 fixed)
People
(Reporter: standard8, Assigned: mathew.hodson)
References
(Blocks 1 open bug)
Details
Attachments
(1 file)
(deleted),
text/x-phabricator-request
|
Details |
In removing places where we're using ChromeUtils.import
to import into the this
global scope, we found a case in SessionStateAggregator.js that is potentially unclear as to what it is doing.
The original code is:
ChromeUtils.import("resource://gre/modules/XPCOMUtils.jsm", this);
ChromeUtils.import("resource://gre/modules/Timer.jsm", this);
const { Services } = ChromeUtils.import(
"resource://gre/modules/Services.jsm",
this
);
This imports XPCOMUtils
, all items from Timer.jsm
and possibly Services
into the global this
object.
The intention in the patch was to change it to:
const { XPCOMUtils } = ChromeUtils.import(
"resource://gre/modules/XPCOMUtils.jsm"
);
const { setTimeoutWithTarget } = ChromeUtils.import(
"resource://gre/modules/Timer.jsm"
);
const { Services } = ChromeUtils.import("resource://gre/modules/Services.jsm");
As Gijs pointed out:
This file also uses clearTimeout. And I'm not sure if those are available on this global (the message manager, as this is a frame script) by default, but even if they were, their identifiers are likely different from the ones generated by Timer.jsm which is where setTimeoutWithTarget comes from, and so clearTimeout wouldn't do the right thing.
I'd also be curious whether the pre-patch state assigns the timeout stuff to this and thus impacts other framescripts loaded in this context...
As I don't have an android environment set up to try this out on, I'm disabling the rule in SessionStateAggregator.js
for now. This bug should re-enable it when fixed.
Updated•4 years ago
|
Comment 1•4 years ago
|
||
(In reply to Mark Banner (:standard8) from comment #0)
This file also uses clearTimeout. And I'm not sure if those are available on this global (the message manager, as this is a frame script) by default, but even if they were, their identifiers are likely different from the ones generated by Timer.jsm which is where setTimeoutWithTarget comes from, and so clearTimeout wouldn't do the right thing.
They aren't. The only access frame scripts have to setTimeout
/clearTimeout
is via Timer.jsm or via a content window.
I'd also be curious whether the pre-patch state assigns the timeout stuff to this and thus impacts other framescripts loaded in this context...
For what it's worth, I ran into that a lot when I was migrating frame scripts to actors. I fixed all of the code that I found that was unintentionally relying on Timer.jsm being imported by unrelated code, and we're down to a much smaller number of frame scripts than we used to have, but I wouldn't be surprised if more of those issues have surfaced in the mean time.
Updated•3 years ago
|
Reporter | ||
Updated•3 years ago
|
Assignee | ||
Comment 2•3 years ago
|
||
Updated•3 years ago
|
Reporter | ||
Comment 3•3 years ago
|
||
Mathew, please stop arbitrarily moving dependencies around. A lot of these aren't helping and are creating unnecessary noise. The reason this was linked to bug 1608272 is because that's where we found the issue in the first place, and so having it directly linked makes sense as this is a follow-up to that bug.
Reporter | ||
Comment 4•3 years ago
|
||
Updating dependencies to reflect that it is planned that bug 1648158 will fix this, and also that bug 1758481 is the one this really blocks (SessionStateAggregator.js doesn't use null).
Comment 6•2 years ago
|
||
bugherder |
Comment 7•2 years ago
|
||
No need to uplift to Beta 101
Description
•