690825 - "Assertion failure: !wrapper->unwrap(NULL)->isProxy()"

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jesse Ruderman]

Attachment: testcase (asserts fatally when loaded)

This browser testcase triggers:

Assertion failure: !wrapper->unwrap(NULL)->isProxy(), at js/src/jswrapper.cpp:749

Comment 1

[Jesse Ruderman]

Attachment: stack trace

Comment 2

[Jesse Ruderman]

My guess at a JS shell testcase did NOT reproduce the assertion:

n = newGlobal('new-compartment')
(new WeakMap).has.call(n);

Instead, it said "function has() {[native code]} is not a function". (O rly?)

Comment 3

[Luke Wagner [:luke]]

The key piece of this test-cast is that 'this' is a wrapped wrapper (the first wrapper is cross-compartment, the second is the outer window, which is implemented as a wrapper, but not a cross-compartment wrapper) which the assert didn't think was possible.  So the simple fix is to change the assert to !isCrossCompartmentWrapper().

But then it hits a different assert (and null-deref) when HandleNonGenericMethodClassMismatch assumes that the callee is a function (in fact it is a wrapped function).  This is also trivially fixed by having callers explicitly pass the native.

Stepping through this I got the chills to see us enter the compartment of a different-domain (to do nativeCall).  This is safe because (by the NonGenericMethodGuard contract), this will immediately return an error.  However, I'd like to back off this ledge and just have the security wrappers override nativeCall to fail.  But that is gross and leads me back to a previous design I pursued, then backed off of, which was to make a new TransparentCrossCompartmentWrapper that was not derived by security wrappers.  That way, the *default* would be for nativeCall to give an errror.

Comment 4

[Luke Wagner [:luke]]

Jesse: here's the shell test-case:

var wrappedProxy = newGlobal('new-compartment').eval('Proxy.create({}, {}.prototype)');
String.prototype.toString.call(wrapedProxy);

Comment 5

[Luke Wagner [:luke]]

Attachment: fix HandleNonGenericMethodClassMismatch and nativeCall assert

Arg, I was so close to having had this caught by testCrossCompartmentTransparency.js.  Waldo, I blame you ;-)

Comment 6

[Luke Wagner [:luke]]

Attachment: add js::SecurityWrapper

I started to add a TransparentCrossCompartmentWrapper (and make CrossCompartmentWrapper opaque by default), but this was going against the grain.  The natural solution I think is to add a js::SecurityWrapper that derives CrossCompartmentWrapper which has the meaning: "a cross compartment wrapper which does not share by default" (unlike Wrapper and CCW which are transparent by default).

Now, in this patch, I only added overrides for nativeCall and objectClassIs (and thus the testcase in comment 0 never enters the target compartment to call WeakMap_has) because I was scared of breaking things.  But I think, with SecurityWrapper in place, we may want to add more default-to-no overrides in the future.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 564227 [details] [diff] [review]
fix HandleNonGenericMethodClassMismatch and nativeCall assert

Review of attachment 564227 [details] [diff] [review]:
-----------------------------------------------------------------

A little kludgy, but meh.

Comment 8

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6029755897c3

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/6029755897c3

Comment 10

[Luke Wagner [:luke]]

Still waiting on review from mrbkap and/or gal on the SecurityWrapper patch.

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 564274 [details] [diff] [review]
add js::SecurityWrapper

I talked with Luke on IRC. There are a couple of same compartment wrappers that are security wrappers which necessitates making SecurityWrapper a template in its own right.

Comment 12

[Luke Wagner [:luke]]

Attachment: add CrossCompartmentSecurityWrapper and SameCompartmentSecurityWrapper

The SecurityWrapper<> template was verbose so I added two slightly less but still verbose typedefs.

Comment 13

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 565976 [details] [diff] [review]
add CrossCompartmentSecurityWrapper and SameCompartmentSecurityWrapper

r=me with the fix to WrapperFactory::WrapSOWObject.

Thanks.

Comment 14

[Luke Wagner [:luke]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/949c2cf4c772

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/949c2cf4c772