1692217 - SessionStateAggregator.js imports Timer.jsm into frame script 'this' scope

Status: RESOLVED FIXED

(GeckoView :: General, task, P3)

Description

[Mark Banner (:standard8)]

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.

Comment 1

[Kris Maglione [:kmag]]

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

Comment 2

[Mathew Hodson]

Attachment: Bug 1692217 - Remove `ChromeUtils.import` with `this` in SessionStateAggregator.js. r?agi

Comment 3

[Mark Banner (:standard8)]

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.

Comment 4

[Mark Banner (:standard8)]

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 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c201d7243e0
Remove `ChromeUtils.import` with `this` in SessionStateAggregator.js. r=agi

Comment 6

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/0c201d7243e0

Comment 7

[Chris Peterson [:cpeterson]]

No need to uplift to Beta 101