this !== window within content_scripts

NEW
Unassigned

Status

()

Toolkit
WebExtensions: General
P3
normal
2 years ago
8 days ago

People

(Reporter: callahad, Unassigned)

Tracking

({DevAdvocacy})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: triaged[DevRel:P3])

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

2 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

2 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

2 years ago
This no longer blocks Bug 1208765, as RES has moved away from depending on this quirk.
No longer blocks: 1208765
Blocks: 1214433
Priority: -- → P1

Updated

2 years ago
Flags: blocking-webextensions+

Updated

2 years ago
Whiteboard: triaged

Updated

2 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

2 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

2 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

2 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

2 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

2 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

a year ago
Whiteboard: triaged → triaged[DevRel:P3]

Updated

11 months ago
Component: WebExtensions: Untriaged → WebExtensions: General
Flags: blocking-webextensions-

Updated

9 months ago
Duplicate of this bug: 1320262

Updated

6 months ago
Duplicate of this bug: 1344066

Comment 23

4 months 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

3 months 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

2 months ago
Duplicate of this bug: 1372649

Comment 26

2 months 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"

Updated

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