604346 - Impossible to create a sandbox that would reuse a compartment

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wladimir Palant]

It seems that bug 584237 broke Adblock Plus 1.2.2. Reason is misuse of sandboxes to get an independent namespace. Code that works with the browser window is loaded like this:

  var sandbox = new Components.utils.Sandbox(window);
  sandbox.window = window;
  subscriptLoader.loadSubScript("...", sandbox);

However, this code seems to be in a different compartment now and has limited access to the browser window (in particular, window.gBrowser.contentWindow is undefined). I am not sure whether it is supposed to work that way or whether it is a bug - there seems to be zero documentation on how compartments are supposed to work. At least changing the parameter to Sandbox constructor doesn't seem to have any effect (system principal produces the same result, chrome URI crashes).

Comment 1

[Wladimir Palant]

Btw, I observed this issue in Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101013 Firefox/4.0b8pre. I also tried 20101013 nightly from tracemonkey branch.

Comment 2

[Wladimir Palant]

After more investigation it seems that window.gBrowser returns an XrayWrapper instance which is supposed to allow all access to native properties. However, it doesn't work for contentWindow, probably because it is an XBL property. The other issue is that expando properties set on XrayWrapper aren't visible from other compartments.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Yeah, the real issue is that we end up with an XrayWrapper here, which is the equivalent of the old XPCNativeWrapper.  We should be ending up with the equivalent of the old XPCSafeJSObjectWrapper or something, no?

Comment 4

[Peter Van der Beken [:peterv]]

Andreas, we end up in ResolveNativeProperty from XrayWrapper<Base, Policy>::enumerate. The NewResolve hook tries to define the property, in XrayWrapper<Base, Policy>::defineProperty we end up defining it on the expando (GetExpandoObject). Next thing in ResolveNativeProperty we call JS_GetPropertyDescriptorById on the holder, which of course doesn't find the property. Any ideas?

Comment 5

[Peter Van der Beken [:peterv]]

Ignore comment 4, wrong bug.

Comment 6

[IU]

This bug also affects the Stop Autoplay extension: https://addons.mozilla.org/en-US/firefox/addon/1765/

Comment 7

[Wladimir Palant]

(In reply to comment #6)
> This bug also affects the Stop Autoplay extension:
> https://addons.mozilla.org/en-US/firefox/addon/1765/

Why do you think that? Looking at its source code I see no why it would be affected - not only doesn't it use sandboxes, it generally doesn't load code dynamically.

Comment 8

[IU]

(In reply to comment #7)
> Why do you think that? Looking at its source code I see no why it would be
> affected - not only doesn't it use sandboxes, it generally doesn't load code
> dynamically.

Sorry.  Wrong bug.  Should have posted to 604420.

Comment 9

[Peter Van der Beken [:peterv]]

In order to avoid Xray wrappers you need to call the Sandbox constructor with as second argument an object that has a property named wantXrays with value false.

Comment 10

[Giorgio Maone [:ma1]]

(In reply to comment #9)
> In order to avoid Xray wrappers you need to call the Sandbox constructor with
> as second argument an object that has a property named wantXrays with value
> false.

Is this new sandbox/wrappers stuff documented anywhere?

Comment 11

[Wladimir Palant]

For reference: the change in question is http://hg.mozilla.org/mozilla-central/rev/f68f9d741b29 (in bug 604957).

Comment 12

[Wladimir Palant]

Peter, this bug is marked as blocking Beta 7. It doesn't look like your patch landed on Beta 7 branch however. So this bug isn't resolved after all?

Comment 13

[Peter Van der Beken [:peterv]]

(In reply to comment #10)
> Is this new sandbox/wrappers stuff documented anywhere?

Not yet, it will be though. It took a while to figure out the right API.

(In reply to comment #12)
> Peter, this bug is marked as blocking Beta 7. It doesn't look like your patch
> landed on Beta 7 branch however. So this bug isn't resolved after all?

None of the compartments patches have landed on the branch and it is fixed on trunk.

Comment 14

[Eric Shepherd [:sheppy]]

Is there documentation needed related specifically to this bug? I already documented the changes to the Components.util.Sandbox constructor, which seems to cover the stuff I read here.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

Yeah, the sandbox constructor documentation covers this. Wladimir, the { wantXrays: false } flag is the one that affects this bug.

Comment 16

[Wladimir Palant]

Adblock Plus 1.3 has been released a week ago, it no longer needs to create sandboxes. So Adblock Plus is no longer affected.