this !== window within content_scripts

NEW
Unassigned

Status

P3
normal
3 years ago
16 days ago

People

(Reporter: callahad, Unassigned)

Tracking

({DevAdvocacy})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged[DevRel:P3])

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
(If anyone can suggest a better title, I'd be grateful :))

Attempts to share variables between content_scripts by assigning to this.{variableName} fail when accessed via window.{variableName}.

This works in Chrome, but not in Firefox.

Steps to Reproduce:

1. Define two content scripts, "A" and "B".
2. In "A", set this.foo = "foo";
3. In "B", console.log("window.foo is", window.foo);

What Should Happen:

- "window.foo is foo" should appear in the console

What Actually Happens:

- "window.foo is undefined" appears in the console

Additional Notes:

In Chrome, B is able to see all of: window.foo, self.foo, this.foo, and foo.

In Firefox, B is only able to see: this.foo and foo.

This causes the Reddit Enhancement Suite to fail: https://github.com/honestbleeps/Reddit-Enhancement-Suite/blob/v4.5.4/lib/vendor/favico.js#L830-L833

Test case in the "shared_content_script_properties" folder of https://github.com/callahad/webextension-tests
I think the reason here is that we use sandboxes for content script, and for sandboxes the xray uses exclusiveGlobal. What that means is that for each content script we use a different expendo for the same window, unlike in the web content case, where the expendos are shared between same origin compartments.

Bobby, I think what we want here is to add an extra option to sandboxes, that makes them behave like web content modulo content scripts of the same extension. We might want to add originAttribute to these sandboxes and use those for this check, but I'm fine with any other solution. What do you think?
Flags: needinfo?(bobbyholley)
Er, I'm not sure. We use the same sandbox for all content scripts for a given extension. I think maybe we just need to do something like |sandbox["window"] = sandbox|. Otherwise |window| is always going to be different than |this|.
(In reply to Bill McCloskey (:billm) from comment #2)
> Er, I'm not sure. We use the same sandbox for all content scripts for a
> given extension.

Exactly, so I don't think comment 1 applies.

> I think maybe we just need to do something like
> |sandbox["window"] = sandbox|. Otherwise |window| is always going to be
> different than |this|.

That's pretty yucky - certainly incompatible with the existing body of scripts that get run in sandboxPrototype sandboxes, though we could do it just for WebExtensions if compat is really such an issue. But I think it would probably break a ton of stuff - basically any DOM API that takes a Window as an argument would stop working because |window| wouldn't point to a Window anymore. It would also make the semantics subtly different from GreaseMonkey and Jetpack, which would be unfortunate.

I think we should just fix this with occasional pull requests to addons that interchange |window| with |this|, which I'd guess is pretty rare.
Flags: needinfo?(bobbyholley)
(Reporter)

Comment 4

3 years ago
The problem is that this pattern works just great in Chrome, but fails in Firefox, and the first library I looked into porting, RES, fails because of this incongruity.

favico.js tries to detect AMD / RequireJS / CommonJS and do the right thing, falling back to "this.Favico = Favico" for the "included directly via <script> tag" case: https://github.com/ejci/favico.js/blob/master/favico.js#L838-L851

RES then attempts to call "favicon = new window.Favico();", which only fails in Firefox.

I'll send a patch upstream to RES, since omitting "window." works in both Fx and Chrome.

Perhaps the best outcome of this would be a policy statement regarding the fidelity of our parity with Chrome? It sounds like we're leaning toward "it's not a bug if there's an equally idiomatic way to accomplish the same thing in both browsers?" This would have a bearing on Bug 1209192 as well.
It looks like Jetpack does the hack that I mentioned:
http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/sandbox.js#158

However, I'm still not sure whether we should do this. It adds some hackiness to WebExtensions, which is something I'd like to avoid if possible. However, this is likely to come up a lot since |this == window| is something that web developers have really come to expect.
Sorry I misread the STR.

(In reply to Bill McCloskey (:billm) from comment #5)
> However, this is likely to come up a lot since |this == window| is
> something that web developers have really come to expect.

It did for jetpack. I tried to push the team to drop that hack, but too many people expected/wanted this to work. Including some popular libraries (or at least some versions of them).
(Reporter)

Comment 7

3 years ago
This no longer blocks Bug 1208765, as RES has moved away from depending on this quirk.
No longer blocks: 1208765

Updated

3 years ago
Flags: blocking-webextensions+

Updated

3 years ago
Whiteboard: triaged

Updated

3 years ago
Flags: blocking-webextensions+ → blocking-webextensions-
Priority: P1 → P3
Created attachment 8716490 [details]
MozReview Request: [STRAW MAN] Bug 1208775: [webext] Make `this` and `window` consistent in content script scopes.

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

Updated

3 years ago
Attachment #8716490 - Flags: feedback?(wmccloskey)
Attachment #8716490 - Flags: feedback?(bobbyholley)
So this basically calls loadSubScript and sets the target object to a cross-compartment wrapper around a Window. This works better than I would have expected. However, we more tests for things like:

window.foo = "bar"; this.foo;  // I think this should work.
foo = "bar"; window.foo;       // (No |var|.) I don't think this will work.

I don't think we should do this unless it's pretty solid. At least the current problem makes sense to developers. Having it work with |var| but not without just seems like a bug, and people will get very confused.

I think this might be fixable though. The problem happens because loadSubScript with a non-global target object creates a DynamicWithObject on the scope chain. DynamicWithObjects are not considered unqualified variables objects, so unqualified var lookups skip over them.

We also have something called a NonSyntacticVariablesObject. It *is* considered an unqualified variables object. However, it doesn't allow you to store the vars on a separate object--they have to go directly on the NSVO. Since a Window isn't an NSVO, we can't use that.

It seems like we want to modify DynamicWithObject to have a flag saying whether it should act as an unqualified variables object. However, there may be reasons why we can't do that. I'm going to needinfo shu since he is the expert here. If we could use a DynamicWithObject as an unqualified vars object, then we maybe could just eliminate NonSyntacticVariablesObject and always use DynamicWithObjects instead. However, I'm worried there's a reason why the code doesn't already work this way.
Flags: needinfo?(shu)
(In reply to Bill McCloskey (:billm) from comment #9)
> So this basically calls loadSubScript and sets the target object to a
> cross-compartment wrapper around a Window. This works better than I would
> have expected. However, we more tests for things like:
>
> window.foo = "bar"; this.foo;  // I think this should work.
> foo = "bar"; window.foo;       // (No |var|.) I don't think this will work.

You're right, the first one works, the second one doesn't. I added tests for
both.

> I don't think we should do this unless it's pretty solid. At least the
> current problem makes sense to developers. Having it work with |var| but not
> without just seems like a bug, and people will get very confused.

Yeah, that's a fair point.
Comment on attachment 8716490 [details]
MozReview Request: [STRAW MAN] Bug 1208775: [webext] Make `this` and `window` consistent in content script scopes.

I generally agree with Bill. I'm ok with some very simple property definition if it makes a big difference in terms of script compatibility, but I'm very wary about introducing yet another set of semantics.

The correct engine-side fix here, IMO, is to actually make the window wrapper the global somehow.
Attachment #8716490 - Flags: feedback?(bobbyholley)

Comment 12

3 years ago
(In reply to Bill McCloskey (:billm) from comment #9)
> It seems like we want to modify DynamicWithObject to have a flag saying
> whether it should act as an unqualified variables object. However, there may
> be reasons why we can't do that. I'm going to needinfo shu since he is the
> expert here. If we could use a DynamicWithObject as an unqualified vars
> object, then we maybe could just eliminate NonSyntacticVariablesObject and
> always use DynamicWithObjects instead. However, I'm worried there's a reason
> why the code doesn't already work this way.

The history here is that any object used to be able to be designated as a qualified varobj (holder of 'var'-introduced bindings) and an unqualified varobj (holder of bareword assignments). We made some effort to clean that up so a few distinguished scope objects can be varobjs at all.

For qualified, that list is: global scope, function scope, module scope, NSVO, and |with| scopes that were introduced by the embedding (i.e., |with| scopes introduced by |with (o)| don't trap vars).

For unqualified, that list is: global scope, NSVO.

I don't think there's an implementation reason in the engine that |with| scopes aren't unqualified varobjs. IIRC, the reason they're qualified varobjs at all was due to some pattern where chrome code would pass in some object to loadSubScript, and expect to be able to access 'var' bindings on that object.

Currently DynamicWithObjects have two modes:

1) Introduced by source text. Neither a qualified nor an unqualified varobj.
2) Introduced by embedding. Qualified varobj.

I would like to avoid adding a third mode where it's both a qualified and an unqualified varobj, but I'm open to changing 2) to all DynamicWithObjects that are introduced by the embedding to be both qualified and unqualified varobjs.

The question is, is there code that depends on scopes that are passed to NOT trap bareword assignments but do trap 'var' bindings? I sure hope not.

(In reply to Bobby Holley (busy) from comment #11)
> The correct engine-side fix here, IMO, is to actually make the window
> wrapper the global somehow.

Do all content scripts in the add-on get the same window wrapper?
Flags: needinfo?(shu)

Comment 13

3 years ago
I'm hesitant to say what I think is the correct engine-side fix without fully understanding what weird behaviors our chrome code depend on. Seems like one of those leave-your-logic-at-the-door situations.
(In reply to Bobby Holley (busy) from comment #11)
> The correct engine-side fix here, IMO, is to actually make the window
> wrapper the global somehow.

I think we could actually use something like Kris's approach to clean a lot of stuff up without a lot of reworking of our invariants. Shu's recent scope work allows us to put an object on the scope chain right before the global. If we could make that work for unqualified var objects, then we could get rid of this horrible hack:
https://dxr.mozilla.org/mozilla-central/source/js/xpconnect/src/Sandbox.cpp#571

I'm not even sure that using a window wrapper as the global is that much cleaner. And it would be a lot more work.

> The question is, is there code that depends on scopes that are passed to NOT trap bareword
> assignments but do trap 'var' bindings? I sure hope not.

I hope not too, but I'm not sure. It seems quite believable that mistakes have crept into people's code. At the same time, it's not hard to convince someone that this is a bug in their code, not ours.

I think it would be worth trying this out. Step 1 would be to land a change pretty early in the release cycle that makes DynamicWithObjects that are qualified var objs also be unqualified var objs. If it's not a huge disaster, then we can land Kris's patch.

> Do all content scripts in the add-on get the same window wrapper?

There's a 1:1 correspondence between the global (which is a Sandbox right now) and the window wrapper. But a given add-on can have more than one Sandbox if it's touching multiple windows.
Kris, if you're up for trying this, it's a pretty simple change. The code is here:
https://dxr.mozilla.org/mozilla-central/source/js/src/jsobjinlines.h#240
I *think* you just need to make isUnqualifiedVarObj check hasAllFlags(js::BaseShape::QUALIFIED_VAROBJ) instead of what it's doing now. Then you would push to try and see what breaks. And then maybe check a few big add-ons that use loadSubScript.

Comment 16

3 years ago
(In reply to Bill McCloskey (:billm) from comment #15)
> Kris, if you're up for trying this, it's a pretty simple change. The code is
> here:
> https://dxr.mozilla.org/mozilla-central/source/js/src/jsobjinlines.h#240
> I *think* you just need to make isUnqualifiedVarObj check
> hasAllFlags(js::BaseShape::QUALIFIED_VAROBJ) instead of what it's doing now.
> Then you would push to try and see what breaks. And then maybe check a few
> big add-ons that use loadSubScript.

No, that's what not I suggested and would break spec compliance badly. That would mean that all qualified varobjs are also unqualified varobjs, which means things like |eval('x = 42')| would introduce new var bindings inside a function.

What you want is add another disjunct for DynamicWithObject:

is<js::DynamicWithObject>() && !as<js::DynamicWithObject>().isSyntactic()
(In reply to Bill McCloskey (:billm) from comment #15)
> Kris, if you're up for trying this, it's a pretty simple change. The code is
> here:
> https://dxr.mozilla.org/mozilla-central/source/js/src/jsobjinlines.h#240
> Then you would push to try and see what breaks. And then maybe check a few
> big add-ons that use loadSubScript.

Yeah, I'll give it a try.


(In reply to Shu-yu Guo [:shu] from comment #16)
> What you want is add another disjunct for DynamicWithObject:
> 
> is<js::DynamicWithObject>() && !as<js::DynamicWithObject>().isSyntactic()

Thanks
I tried this yesterday. As far as the subscript loader is concerned, it worked well.

It does cause some other problems, though, mainly in terms of odd test failures:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fed4480c15d5&selectedJob=16510482

I'm not very familiar with most of this code, but he problem seems to be that change also affects other scopes, including event listener attributes and some uses of eval, so sometimes assignments that are supposed to go to the global (e.g., gContextMenu) wind up on some other scope instead.

Comment 19

3 years ago
(In reply to Kris Maglione [:kmag] from comment #18)
> I tried this yesterday. As far as the subscript loader is concerned, it
> worked well.
> 
> It does cause some other problems, though, mainly in terms of odd test
> failures:
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fed4480c15d5&selectedJob=16510482
> 
> I'm not very familiar with most of this code, but he problem seems to be
> that change also affects other scopes, including event listener attributes
> and some uses of eval, so sometimes assignments that are supposed to go to
> the global (e.g., gContextMenu) wind up on some other scope instead.

From this then it seems like the answer to the question

> is there code that depends on scopes that are passed to NOT trap bareword assignments but do trap 'var' bindings?

is yes. :(
Comment on attachment 8716490 [details]
MozReview Request: [STRAW MAN] Bug 1208775: [webext] Make `this` and `window` consistent in content script scopes.

Going to clear this for now.
Attachment #8716490 - Flags: feedback?(wmccloskey)
(Reporter)

Updated

3 years ago
Whiteboard: triaged → triaged[DevRel:P3]

Updated

2 years ago
Component: WebExtensions: Untriaged → WebExtensions: General
Flags: blocking-webextensions-

Updated

2 years ago
Duplicate of this bug: 1320262

Updated

2 years ago
Duplicate of this bug: 1344066

Comment 23

2 years ago
I am having a similar issue in my extension. 

Basically I am creating a variable on document_start and then check its existence with chrome.tabs.executeScript({code: `window.myVar`}, [r] => console.error(r)); It works fine in Chrome but not Firefox. Is there any workaround for this issue?

Comment 24

a year ago
Wouldn't window being in the scope chain *before* the sandbox global weaken security and performance by making |new Object()| equivalent to |new window.Object()| which creates them in the less privileged page compartment?

https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Language_Bindings/Components.utils.createObjectIn

Updated

a year ago
Duplicate of this bug: 1372649

Comment 26

a year ago
(In reply to andy.portmen from comment #23)
> I am having a similar issue in my extension. 
> 
> Basically I am creating a variable on document_start and then check its
> existence with chrome.tabs.executeScript({code: `window.myVar`}, [r] =>
> console.error(r)); It works fine in Chrome but not Firefox. Is there any
> workaround for this issue?

Use "this" instead of "window"
Duplicate of this bug: 1385775
Duplicate of this bug: 1395568

Comment 29

a year ago
Is there a timeline for when/if this will be addressed? Seems to be a pretty big departure from how add-on SDK worked and how chrome extensions work.

Comment 30

a year ago
This seems to be a non trivial issue. Accessing "this" context via a webpack module I believe is currently impossible. See this related issue: https://stackoverflow.com/questions/46027967/capture-this-context-in-webpack. Please prioritize this higher, as this is currently a blocker, and will require decent workarounds in order to overcome.
Duplicate of this bug: 1396482

Updated

5 months ago
Product: Toolkit → WebExtensions

Updated

4 months ago
Duplicate of this bug: 1475950

Comment 33

3 months ago
I need to access variables from the top frame content-script. Since we are on the same origin, in Chrome I use `window.top.my_var`. In FF, none of the following works

`
window.top.my_var
self.top.my_var
this.top.my_var
`

Any advice?
Is the issue in comment 33 related to the fact that we have special expando behavior for Xrays originating from sandboxes (the exclusive global stuff at [1])?

If so, I wonder if it makes sense to change it, and make it opt-out for content script sandboxes. We originally implemented that behavior just because it seemed more sane for sandboxes, so we could easily change it if it's causing trouble. That said, the comment in that code now mentions JIT behavior, so maybe we've started depending on it for performance.

Thoughts Kris?

[1] https://searchfox.org/mozilla-central/rev/71ef4447db179639be9eff4471f32a95423962d7/js/xpconnect/wrappers/XrayWrapper.cpp#1031
Flags: needinfo?(kmaglione+bmo)
(In reply to Bobby Holley (:bholley) from comment #34)
> Is the issue in comment 33 related to the fact that we have special expando
> behavior for Xrays originating from sandboxes (the exclusive global stuff at
> [1])?

It is, yes. Or, at least, that's the problem if the variables are explicitly being assigned to properties of `window` rather than to top-level `this` or as top-level `var`s.

> If so, I wonder if it makes sense to change it, and make it opt-out for
> content script sandboxes. We originally implemented that behavior just
> because it seemed more sane for sandboxes, so we could easily change it if
> it's causing trouble. That said, the comment in that code now mentions JIT
> behavior, so maybe we've started depending on it for performance.

I think it does make sense, but yes, we rely heavily on it for JIT performance now. The IC stubs for X-ray property access rely on exclusive X-ray expandos, and that's not trivial to change.

I suspect our best bet at this point is to try to put all same-origin content scripts into the same compartment, which would make this all Just Work. We could do that easily enough now with non-syntactic scope chains, but I'm worried that would hurt JIT performance a lot, so it might be better to just wait for bug 1357862.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #35)
>
> I suspect our best bet at this point is to try to put all same-origin
> content scripts into the same compartment, which would make this all Just
> Work. We could do that easily enough now with non-syntactic scope chains,
> but I'm worried that would hurt JIT performance a lot, so it might be better
> to just wait for bug 1357862.

Hm, nice idea! I hadn't considered the implications of bug 1357862 on sandboxes. That would solve the issue in comment 33, so probably makes sense to wait for that.

In the mean time, any proposed workarounds for Jeremy?
(In reply to Bobby Holley (:bholley) from comment #36)
> In the mean time, any proposed workarounds for Jeremy?

No especially sane ones. Extension messaging is probably the safest bet.

The only other real alternative I can think of is to use custom events or X-ray waivers to pass an object belonging to one content script sandbox to another. Content pages would still technically be able to intercept that event or read the property, but they wouldn't be able to access any properties of the object.

We ideally really need a Sandbox-aware window.postMessage to allow content scripts to send messages to moz-extension: origins and Sandbox listeners to receive them.

Comment 38

3 months ago
:kmag

> Extension messaging is probably the safest bet

This is not working for my case. So in my Popup Blocker (strict) extension, I need to have access to the preferences as soon as possible especially since most malicious scripts use dynamic iframe method for popup issuing. In the new release, when a subframe is created, the extension tries to get the preferences from its parent node; if this is not possible, then uses the storage API call to get the preferences. This method works pretty well.

I can probably use the custom event idea, but this is what I am trying to avoid to prevent any unprotected script from observing the user preferences.
(In reply to Jeremy from comment #38)
> I can probably use the custom event idea, but this is what I am trying to
> avoid to prevent any unprotected script from observing the user preferences.

Kris' point in comment 37 is that, while the script may be able to intercept the event, you can prevent it from reading the data by allocating the data as an object in the content script, since security checks will prevent the content from reading the higher-privileged object.

Comment 40

3 months ago
(In reply to Kris Maglione [:kmag] from comment #37)
> We ideally really need a Sandbox-aware window.postMessage to allow content
> scripts to send messages to moz-extension: origins and Sandbox listeners to
> receive them.

see bug 1305109

Updated

16 days ago
Duplicate of this bug: 1502726
You need to log in before you can comment on or make changes to this bug.