435151 - XPCSafeJSObjectWrapper breaks evalInSandbox

Status: RESOLVED FIXED

(Core :: XPConnect, defect, P2)

Description

[Mike Perry]

I'm trying to get the Torbutton 1.1.x-alpha extension working with Firefox 3,
but I seem to be running into a bug involving evalInSandbox. 

The extension needs to run some javascript in the context of the content 
window to install wrappers around the screen and Date objects to prevent anonymity set reduction by resolution and timezone.

The script is injected via evalInSanbox with a sandbox of:
s = Cu.Sandbox(contentWindow.wrappedJSObject).
s.window = contentWindow.wrappedJSObject;

The problem is that in Firefox 3.0, I've started getting the
following error from an eval that I also call inside the sandboxed
code:

"EvalError: function eval must be called directly, and not by way of a
function of another name"

The line in question just does a window.eval(string). I suspect
something is going on with the XPCSafeJSObjectWrapper now wrapping my
sandboxed window (and thus the eval call).

In addition, I seem to be unable to call constructors from the wrapped
window object in general. Doing something as simple as:

new window.Object();

I've created a test extension that reproduces these errors for FF3.0rc1 here:
http://fscked.org/transient/firefox/evalInSandbox.xpi

It does not require Tor to run, and will pop up alerts with the above errors on every page loaded while it is enabled.

Comment 1

[Mike Perry]

Err, missed a line from that description (was a cut and paste). Doing:

new window.Object();

throws a NS_ERROR_XPC_NOT_ENOUGH_ARGS exception. For constructors with arguments, such as a String copy:

new window.String("Copy me")

throws an NS_ERROR_ILLEGAL_VALUE error. So it appears as if constructor calls inside of a sandbox are off by an arg (perhaps some kind of this pointer is not being properly passed and everything is shifted over?)

Comment 2

[Mike Perry]

I should also note that in case this bug is being neglected because the window.eval issue is a hard one to fix: that issue could be worked around if I was able to do something like 'new window.Function(some_script)'. Of course that would be a performance hit, but at least it would work.. If only constructors were handled properly, that is.

Comment 3

[Samuel Sidler (old account; do not CC)]

Pretty sure this is known/expected. Blake, can you comment?

Comment 4

[Eric Promislow]

We're seeing this in Komodo with Moz 1.9, when a secondary dialog window tries
to run code in the main window's context via

parent.opener.eval(stringToEval)

The code is run, but with the warning.  Both windows are chrome URIs.

Comment 5

[Samuel Sidler (old account; do not CC)]

Assigning to Blake for now so he can comment on this.

Comment 6

[Johnny Stenback (:jst)]

Sounds bad. Blake, if you don't think this is a blocker, please say so.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Fix for this bug

So, this fixes the issue with e.g. (new XPCSafeJSObjectWrapper(window).Number)(5), but not eval. I'll file a new bug on smoothing out the relationship between XPCSafeJSObjectWrapper and evalInSandbox.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

(In reply to comment #7)
> I'll file a new bug on smoothing out the relationship between
> XPCSafeJSObjectWrapper and evalInSandbox.

That blocks bug 460882.

Comment 9

[Damon Sicore (:damons)]

What's left to do here?  Is this ready to land?

Comment 10

[Daniel Veditz [:dveditz]]

Was a new bug filed as mentioned in comment 8? If so would that be a better bug for bug 460882 to depend on? Or (seeing as 460882 is marked fixed) did that aspect just get wrapped up into 460882?

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/mozilla-central/rev/95747b72826b

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/92a2f4d59609

Comment 13

[Al Billings [:abillings - ex-MoCo]]

Hey, Mike, can you verify this with a nightly Firefox 3 build from ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/?

Comment 14

[Mike Perry]

Hrmm, the test xpi is still having issues with the window.String() constructor:

String Constructor failed: TypeError: invalid new expression result new String() success!

I'll have to go back and check the real Torbutton code I had, but I'm thinking that the fix is only partial, since you can't build strings using window.String() inside a sandbox still. 

This may mean that any constructor off of window with more than 0 arguments will fail.

Comment 15

[Mike Perry]

Yeah, the behaviour is the same as the test XPI. This is only a partial fix that seems to cover only zero argument constructors.

Once again, the test xpi to demonstrate the issue is at:
http://fscked.org/transient/firefox/evalInSandbox.xpi

Comment 16

[Blake Kaplan (:mrbkap) (inactive)]

bug 386635 might have fixed this.

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

I'm going to call this fixed by compartments.