1015396 - Prepare Addon-SDK for Object Xrays

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

XrayToJS is now green aside from Jetpack tests:

https://tbpl.mozilla.org/?tree=Try&rev=75c2360851e4

I did some initial work to fix some failures I encountered earlier, but I don't really understand enough about the architecture to do this properly. I'm hoping that gabor can figure out what the right thing to do here is.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Fix Addon SDK. WIP

I did some initial work here, but I'm not sure if this is the right approach
to solve the issues in comment 0. Gabor, what do you think?

Comment 2

[Gabor Krizsanits (INACTIVE)]

I honestly have no idea... Did you locate what fails exactly in that injection magic? That part of the SDK is unreadable for my eyes. I will try to get back here once I have a good grasp of the object xray patches. Maybe you should consult with Irakli about this, he should have a better understanding what is exactly going on there. But I guess first we have to figure out what fails exactly, why modelFor(...).emitToContent is not a function.

Comment 3

[Tomislav Jovanovic :zombie]

is there some more formal description/documentation of changes introduced by these xray changes? something that could be easier to follow than bugzilla comments and diff patches?

Comment 4

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #2)
> But I guess first we have to
> figure out what fails exactly, why modelFor(...).emitToContent is not a
> function.

Presumably because modelFor(...) returns an untrusted (and therefore Xrayed) object, and we disallow resolving callable properties on such objects. I could fix this by just sprinkling a bunch of Cu.waiveXray calls everywhere, but I don't understand this setup enough to know whether that would be safe.

(In reply to Tomislav Jovanovic [:zombie] from comment #3)
> is there some more formal description/documentation of changes introduced by
> these xray changes? something that could be easier to follow than bugzilla
> comments and diff patches?

In general I followed the model in bug 987111 comment 0, with a couple of adjustments. To see the precise implementation, see parts 8, 9, and 10. To see what this looks like from the JS side, see the tests.

If anyone wants to write documentation about this, I'm happy to go through the changes in gory detail.

Comment 5

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #4)
> Presumably because modelFor(...) returns an untrusted (and therefore Xrayed)
> object, and we disallow resolving callable properties on such objects. I

This will break add-ons, so we need some strategy for landing this. Currently Cu
is not available for content-script / GM sandboxes. Is there a wrappedJSObject
property on these xrays or should we expose un/waiveXray methods for these sandboxes?

> could fix this by just sprinkling a bunch of Cu.waiveXray calls everywhere,
> but I don't understand this setup enough to know whether that would be safe.

So this code is about creating sandboxes for content-script from addon scope, and
injecting a few helpers into them (pipes). These sandboxes are not just trusted code,
but at this point in time no code has run in them, so it's absolutely safe I'd say.
Just add waiveXray calls until it's working and make sure you add some short explanation.

> If anyone wants to write documentation about this, I'm happy to go through
> the changes in gory detail.

Since this is (likely) going to break some addons, and they will have to use a new API
(waiveXray) we need a short blog about it as well.

Comment 6

[Gabor Krizsanits (INACTIVE)]

> Since this is (likely) going to break some addons, and they will have to use
> a new API
> (waiveXray) we need a short blog about it as well.

Ok, so wrappedJSObject is available so no new API is needed, we just have to give some heads-up about this change before breaking add-ons.

Comment 7

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #6)
> Ok, so wrappedJSObject is available so no new API is needed, we just have to
> give some heads-up about this change before breaking add-ons.

Yeah. XPCNativeWrapper.unwrap works as well. I just try to use the Cu API wherever possible because it's clearer.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #5)
> So this code is about creating sandboxes for content-script from addon
> scope, and
> injecting a few helpers into them (pipes). These sandboxes are not just
> trusted code,
> but at this point in time no code has run in them, so it's absolutely safe
> I'd say.
> Just add waiveXray calls until it's working and make sure you add some short
> explanation.

Ok, I'll give that a try.

Comment 8

[Gabor Krizsanits (INACTIVE)]

Hey Jeff, I just wanted to get this bug under your radar. Bobby is about to land a special xray for vanilla JSObjects. It will break addons, so we will have to work out a landing strategy.

example that will break:
unsafeWindow.contentDefinedFunction(); <-- will throw a contentDefinedFunction is not defined

workaround:
unsafeWindow.wrappedJSObject.contentDefinedFunction() <-- will waive xray and work as before

there are some other cases too which will fail, like setting/getting some properties on an object from the page script, the workaround will be the same in each scenario. I think if we write a short blog about the different cases, with the workaround and post it before sending the patches to mc then it should be fine...

Comment 9

[Gabor Krizsanits (INACTIVE)]

I'm even considering opting out object xrays for add-ons. I can see how this is incredibly helpful for webidl and in general in the chrome vs content scenario. But frankly, for add-on content-script or grase monkey equivalent it seems to be just another source of frustration. From an add-on developers perspective this is just yet another magic layer that does tricky stuff and have to be waive... I don't really see a scenario where this gives an extra security for add-ons. Bobby, what do you think about opting this out for add-on sandboxes? Haven't thought about the possible consequences, just a wild idea, but I think we should consider it.

Comment 10

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> example that will break:
> unsafeWindow.contentDefinedFunction(); <-- will throw a
> contentDefinedFunction is not defined
> 
> workaround:
> unsafeWindow.wrappedJSObject.contentDefinedFunction() <-- will waive xray
> and work as before

Wait, what? Doesn't unsafeWindow _already_ waive Xrays? That's the only reason you could ever see content-defined expandos.
 
> there are some other cases too which will fail, like setting/getting some
> properties on an object from the page script

But how will the content script even get a reference to the object in the first place? It seems like that would only happen in cases where Xrays have already been waived.

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #9)
> I'm even considering opting out object xrays for add-ons.

I am open to this if object Xrays really would introduce a lot of pain. But so far I'm not convinced. Can you convince me?

Comment 11

[Jonathan Griffin (:jgriffin)]

> Hey Jeff, I just wanted to get this bug under your radar. Bobby is about to land a special xray for 
> vanilla JSObjects.

I'm Jonathan, did you mean someone else instead?

Comment 12

[Gabor Krizsanits (INACTIVE)]

I'm sorry, I accidentally miss-clicked on the wrong name from the list it seems... I meant Jeff Griffith

Comment 13

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #10)
> Wait, what? Doesn't unsafeWindow _already_ waive Xrays? That's the only
> reason you could ever see content-defined expandos.
>  

OK, I have not tested it and I might be completely wrong here, wrappedJSObject used to be a shallow waiver but that might be completely gone by now. So if you're saying that unsafeWindow does deep waiving, then probably this is just a false alarm, and add-ons should not experience much from this change.

Comment 14

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #13)
> OK, I have not tested it and I might be completely wrong here,
> wrappedJSObject used to be a shallow waiver but that might be completely
> gone by now. So if you're saying that unsafeWindow does deep waiving, then
> probably this is just a false alarm, and add-ons should not experience much
> from this change.

I think we fixed the shallow/deep waiver thing. It used to be an odd effect that came from mixing access to Xrayable and non-Xrayable objects. We only did waivers to Xrayable objects. So if you did someXrayableObject.wrappedJSObject.someXrayableObject, you'd get a waiver. But if you did someXrayableObject.wrappedJSObject.someNonXrayableObject.someXrayableObject, you wouldn't get a waiver.

But now we maintain waivers even over non-Xrayable objects, so this should be a non-issue.

Comment 15

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

Uh, so how exactly will it work?

I'm OoO this week but finally have access to the AMO database again and can look to see what the usage of these patterns are ( I think they're fairly low )

Comment 16

[Bobby Holley (:bholley)]

Irakli says we can just git rid of the double sandboxes in the Addon SDK, which seems to fix most of the problems I'd run into. Pushing to try to see if there are any others:

https://tbpl.mozilla.org/?tree=Try&rev=37cd32a438d9

Comment 17

[Bobby Holley (:bholley)]

Attachment: Get rid of apiSandbox. v1

Currently, Jetpack uses two sandboxes for each content script - one for the
content script itself, and another for SDK machinery. And we end up getting Xrays
between the sandboxes, even though they're same-origin (generally with an nsEP)
because Jetpack sandboxes use |wantXrays: true| to get same-origin Xrays.

Object Xrays cause all sorts of communication problems between the sandboxes.
Irakli says we don't actually need the separate sandbox anymore since we've
removed the proxy layer, so let's just get rid of it.

Comment 18

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #16)
> https://tbpl.mozilla.org/?tree=Try&rev=37cd32a438d9

(This was green modulo the fact that I needed to duplicate the changes in sandbox.js to deprecated/traits-worker.js)

Comment 19

[Bobby Holley (:bholley)]

Green try push: https://tbpl.mozilla.org/?tree=Try&rev=bbd8afc93c21

Comment 20

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #17)
> Created attachment 8431016 [details] [diff] [review]
> Get rid of apiSandbox. v1
> 
> Currently, Jetpack uses two sandboxes for each content script

> Irakli says we don't actually need the separate sandbox anymore since we've
> removed the proxy layer, so let's just get rid of it.

Actually the proxy layer was not the reason for the two sandboxes. The reasoning behind it was (if I'm not mistaken) that when a content-script modifies its base prototype like Object/Function.prototype etc., that might alter or break the behavior of these basic jetpack API (accidentally). So it's mostly for guarding add-on developers against shooting them-self in the foot. I don't feel very strongly about keeping them, just wanted to make it clear before we ditch it.

Comment 21

[Gabor Krizsanits (INACTIVE)]

(In reply to Jeff Griffiths (:canuckistani) from comment #15)
> Uh, so how exactly will it work?

I think we don't have to worry about this. It's extremely hard from add-on code to get a reference to an xrayed JSObject. Because we have to waive xrays (like unsafeWindow does) to get a reference to a JSObject from the page scope, and then one would have to explicitly unwaive the xray on that object to get the altered behavior. I don't think any add-on developer ever did something like that, or even aware of the fact that such thing is possible for that matter. TL;DR: it was a false alarm :)

Comment 22

[Bobby Holley (:bholley)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/bf91ee3a9a21

Comment 23

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/bf91ee3a9a21

Comment 24

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/c6e0346eb3da51a78f2c7ed1135792f80b6f0283
Bug 1015396 - Get rid of apiSandbox. r=irakli

Currently, Jetpack uses two sandboxes for each content script - one for the
content script itself, and another for SDK machinery. And we end up getting Xrays
between the sandboxes, even though they're same-origin (generally with an nsEP)
because Jetpack sandboxes use |wantXrays: true| to get same-origin Xrays.

Object Xrays cause all sorts of communication problems between the sandboxes.
Irakli says we don't actually need the separate sandbox anymore since we've
removed the proxy layer, so let's just get rid of it.