1021580 - getting rid of the window getter hack

Status: RESOLVED INCOMPLETE

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Gabor Krizsanits (INACTIVE)]

As it turns out the SDK does this:

140     // We have to ensure that window.top and window.parent are the exact same
141     // object than window object, i.e. the sandbox global object. But not
142     // always, in case of iframes, top and parent are another window object.
143     let top = window.top === window ? content : content.top;
144     let parent = window.parent === window ? content : content.parent;
145     merge(content, {
146       // We need 'this === window === top' to be true in toplevel scope:
147       get window() content,
148       get top() top,
149       get parent() parent,
150       // Use the Greasemonkey naming convention to provide access to the
151       // unwrapped window object so the content script can access document
152       // JavaScript values.
153       // NOTE: this functionality is experimental and may change or go away
154       // at any time!
155       get unsafeWindow() window.wrappedJSObject
156     });

So essentially we are actively faking that within the content-script sandbox scope, window === this, which is a lie. I asked around and it seems like we are doing it for letting some libs do things like:

var foo = 42;

and expecting window.foo to be defined.

I think if an add-on wants that to work, they can just do the same hack explicitly, but doing it everywhere feels very wrong. Conceptually as well (explaining anything about compartments or our security layer with this hack around is pretty horrible...) and also practically, since if I leave this in, that means I have to tell people to use unsafeWindow for the new API (since that at least really is the window object unlike the one you get by this custom window getter, which is not even from the scope of the window...)

Comment 1

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #0)
> As it turns out the SDK does this:
[cut]
> So essentially we are actively faking that within the content-script sandbox
> scope, window === this, which is a lie. I asked around and it seems like we
> are doing it for letting some libs do things like:
> 
> var foo = 42;

> and expecting window.foo to be defined.

Not only for that. We're doing it in order to be sure that every code that assumes `this` is the `window` object, as you said, because in web environment the global object is the `window` object: so, because in content scripts the developers should be able to load library created for browsers, we had to fake it.

Code like that, should works:

  onload = function() {alert('Loaded!')}; 

And so on.

Said that, I think we agreed in a past weekly meeting, that if we're sure we're not breaking any of the major libraries used in web (e.g. jquery), we *could* probably remove the hack, and saying to the add-on developers that they've to use explicitly `window`, listing maybe the case where the code could differ.

This hack was introduced years ago exactly because otherwise they couldn't include such libraries, I don't remember honestly which one specifically – maybe it was jquery itself.

> and also practically, since if I leave this
> in, that means I have to tell people to use unsafeWindow for the new API
> (since that at least really is the window object unlike the one you get by
> this custom window getter, which is not even from the scope of the window...)

I usually tell to people to use `document.defaultView` instead of `unsafeWindow`. I said the same to Will to update his example about the new API.

Comment 2

[Gabor Krizsanits (INACTIVE)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #1) 
> I usually tell to people to use `document.defaultView` instead of
> `unsafeWindow`. I said the same to Will to update his example about the new
> API.

I find it hard to sell that defaultView works but window does not with the exporting API. By the way this hack breaks the defaultView === window assumption as well indeed. We should really try to remove this hack, and see what does it really break, and from there on we can make a decisions about what to do. Can we find a volunteer for that? Telling people to use defaultView by the way is a good idea, I have nothing against that! Putting all this story in a doc does not seem like such a good idea, I agree with Will that the current version is cleaner, especially since this part will change hopefully.

Comment 3

[Jeff Griffiths (:canuckistani) (:⚡︎)]

The problem this solves is: content script code can be written like regular web-content code. I know it has rough edges and involves subterfuge, but the usability of content scripts is a major factor in making the SDK friendly (er) to web developers.

Another benefit is that Chrome's content scripts also have this === window, so often when porting a chrome extension to Firefox the content script can be used unmodified.

You explained the hack. What is the real problem? This isn't a secure bug so I assume this isn't a security problem.

Comment 4

[Bobby Holley (:bholley)]

We are experimenting with something for e10s that uses an addProperty hook to mirror property writes between the sandbox global and the sandboxPrototype. It's still research-y at this point, but if it works, we could consider using it to solve this problem.

Comment 5

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Bobby Holley (:bholley) from comment #4)
> We are experimenting with something for e10s that uses an addProperty hook
> to mirror property writes between the sandbox global and the
> sandboxPrototype. It's still research-y at this point, but if it works, we
> could consider using it to solve this problem.

That sounds promising. My key requirement is that the developer can assume this === window.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Jeff Griffiths (:canuckistani) from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #4)
> My key requirement is that the developer can assume
> this === window.

This would not be the case with the suggestion from comment 4. They would be different objects, but writes to |this| would be mirrored to |window|.

What are the reason for this === requirement? I'm guessing that developers aren't actually testing it, but rather that there's a set of derived observables that we care about.

Comment 7

[Jeff Griffiths (:canuckistani) (:⚡︎)]

(In reply to Bobby Holley (:bholley) from comment #6)

> What are the reason for this === requirement? I'm guessing that developers
> aren't actually testing it, but rather that there's a set of derived
> observables that we care about.

Sorry, that was short-hand for 'the web developer can assume the same globals in the current scope that the would normally expect'. Some framework/library code might *actually compare* this to window though, but I don't think it is a blocker.

Comment 8

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1399562