1207302 - Jetpack content scripts need a better way than document.defaultView to access the content window

Status: RESOLVED INCOMPLETE

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

Description

[Bobby Holley (:bholley)]

I just spent a good chunk of the morning helping djvj debug the strange behavior he was seeing in his addon, which I couldn't reproduce with a mochitest. In his PageMod content script, he would do |new window.Promise(...)|, but somehow end up with a Promise in the sandbox's scope, rather than the page.

After some debugging, I discovered that |window.Promise| was actually an expando property on the XrayWrapper, and that djvj needed to delete that property to access the underlying Xrayed Promise constructor.

Gabor, any idea why we're doing this? We need to fix this for our API injection story to be credible.

Comment 1

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #0)
> Gabor, any idea why we're doing this? We need to fix this for our API
> injection story to be credible.

I have absolutely no idea why. According to blame, Jordan was working on this (Bug 881047) let's see if he has the answer for you. Hey Jordan, is there any reason why don't we just use the DOM Promise instead of the weird Promise.jsm in the SDK? Can we stop doing that, or would that be hard?

Comment 2

[Jordan Santell [:jsantell] [@jsantell]]

Background: Bug 881047 was for changing the implementation of sdk/core/promise to use Promise.jsm rather than rolling its own (not spec compliant) implementation.

Seems like there are two issues here:
* `sdk/core/promise` is used throughout the SDK because it was used before DOM Promises -- I don't think this is related to the issue in comment #1 though, as this is in the addon, not the content script.
* Primitives in content scripts are wrapped.

I think it was some sort of security/privilege issue when changes to unsafeWindow[0] or adding Xrays[1] to content scripts. Gozala would know more about the design/reasoning of this I believe.

[0] https://blog.mozilla.org/addons/2014/04/10/changes-to-unsafewindow-for-the-add-on-sdk/
[1] https://blog.mozilla.org/addons/2014/08/14/compatibility-for-firefox-32/

Comment 3

[Bobby Holley (:bholley)]

Jordan, can you point me to the code where we override the Promise constructor on |window| for content scripts? What would happen if we just removed that?

Comment 4

[Jordan Santell [:jsantell] [@jsantell]]

I don't really know the architecture for the content script portion, pinging Irakli and Mossop for that

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #0)
> window.Promise(...)|, but somehow end up with a Promise in the sandbox's
> scope, rather than the page.

Are you sure that you were not tricked by the window getter hack in content script? http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/content/sandbox.js#165

I could not find any place where we overwrite the Promise on the window or on the content sandbox. Do you get the same result if you do document.defaultView.Promise?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> (In reply to Bobby Holley (:bholley) from comment #0)
> > window.Promise(...)|, but somehow end up with a Promise in the sandbox's
> > scope, rather than the page.
> 
> Are you sure that you were not tricked by the window getter hack in content
> script?
> http://mxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/
> content/sandbox.js#165

Oh yeah, duh. That's almost certainly it. I'd forgotten that |window| lies in JP :-(.

So deleting window.Promise causes us to resolve the Promise constructor on the global's prototype, which is the one we want.

So I guess there's nothing window-specific that needs to be done here. However, what we _do_ need is a way for JP authors to properly access the actual content window when using the exportHelpers.

What about |contentWindow| or |realWindow|? The former is more webby, but the latter is more greppable.

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #6)
> So I guess there's nothing window-specific that needs to be done here.
> However, what we _do_ need is a way for JP authors to properly access the
> actual content window when using the exportHelpers.

Yeah this part always bothered me. document.defaultView is usually what we suggest when someone needs the real window object since that's the most webby alternative.

> 
> What about |contentWindow| or |realWindow|? The former is more webby, but
> the latter is more greppable.

I'm not sure. Something greppable would be nice, but realWindow sounds a bit off. I wish I had a good alternative. I tried to come up with something back then but I failed with it, so feel free to decide this if you feel strongly about either alternative. Clearing needinfo's for Dave and Irakli.

Comment 8

[Bobby Holley (:bholley)]

Ok. Any objections to contentWindow then? Who is currently maintaining JP?

Comment 9

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #8)
> Ok. Any objections to contentWindow then? Who is currently maintaining JP?

No, I'm fine with it. Good question, I think Dave but I'm not sure, so I sent out an email about this. But let's assume it's Dave.

Comment 10

[Dave Townsend [:mossop]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #8)
> > Ok. Any objections to contentWindow then? Who is currently maintaining JP?
> 
> No, I'm fine with it. Good question, I think Dave but I'm not sure, so I
> sent out an email about this. But let's assume it's Dave.

No-one really. Tag you're it!

Comment 11

[Takanori MATSUURA]

This bug lies at rest in the graveyard.