621845 - Assertion failure: compartment mismatched with pending exception

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bob Clary [:bc] (inactive)]

Attachment: testcase

see also bug 609508 for a possible dupe

1. http://qqbaby.qq.com/QzoneJump.html?uin=1073574132%2525252526keyname=368%2525252526params=%2525252526qz_style=33%2525252526canvastype=home

Reason: KERN_PROTECTION_FAILURE at address: 0x00000000
0x0664c3af in JS_Assert (s=0x6b266c0 "compartment mismatched", file=0x6b26088 "/work/mozilla/builds/2.0.0/mozilla/js/src/jscntxtinlines.h", ln=541) at /work/mozilla/builds/2.0.0/mozilla/js/src/jsutil.cpp:80
80	    *((int *) NULL) = 0;  /* To continue from here in GDB: "return" then "continue". */
(gdb) bt
#0  0x0664c3af in JS_Assert (s=0x6b266c0 "compartment mismatched", file=0x6b26088 "/work/mozilla/builds/2.0.0/mozilla/js/src/jscntxtinlines.h", ln=541) at /work/mozilla/builds/2.0.0/mozilla/js/src/jsutil.cpp:80
#1  0x064dd0a1 in js::CompartmentChecker::fail (c1=0x258ccc00, c2=0xf0e600) at jscntxtinlines.h:541
#2  0x064dd0f8 in js::CompartmentChecker::check (this=0xbfffc238, c=0xf0e600) at jscntxtinlines.h:549
#3  0x064dd120 in js::CompartmentChecker::check (this=0xbfffc238, obj=0x251296c8) at jscntxtinlines.h:557
#4  0x064dd151 in js::CompartmentChecker::check (this=0xbfffc238, v=@0xbfffc208) at jscntxtinlines.h:562
#5  0x064dd17d in js::CompartmentChecker::check (this=0xbfffc238, v={asBits = 18446462629419587272, s = {payload = {i32 = 621975240, u32 = 621975240, boo = 621975240, str = 0x251296c8, obj = 0x251296c8, ptr = 0x251296c8, why = 621975240, word = 621975240}, tag = JSVAL_TAG_OBJECT}, asDouble = -nan(0xf0007251296c8), asPtr = 0x251296c8}) at jscntxtinlines.h:566
#6  0x064e0f20 in js::assertSameCompartment<jsval_layout> (cx=0x234c94d0, t1={asBits = 18446462629419587272, s = {payload = {i32 = 621975240, u32 = 621975240, boo = 621975240, str = 0x251296c8, obj = 0x251296c8, ptr = 0x251296c8, why = 621975240, word = 621975240}, tag = JSVAL_TAG_OBJECT}, asDouble = -nan(0xf0007251296c8), asPtr = 0x251296c8}) at jscntxtinlines.h:624
#7  0x064bc9f8 in JS_SetPendingException (cx=0x234c94d0, v={asBits = 18446462629419587272, s = {payload = {i32 = 621975240, u32 = 621975240, boo = 621975240, str = 0x251296c8, obj = 0x251296c8, ptr = 0x251296c8, why = 621975240, word = 621975240}, tag = JSVAL_TAG_OBJECT}, asDouble = -nan(0xf0007251296c8), asPtr = 0x251296c8}) at /work/mozilla/builds/2.0.0/mozilla/js/src/jsapi.cpp:5794
#8  0x05a457b4 in AutoExceptionRestorer::~AutoExceptionRestorer (this=0xbfffc2f8) at /work/mozilla/builds/2.0.0/mozilla/js/src/xpconnect/src/xpcconvert.cpp:1603
#9  0x05a3ebf6 in XPCConvert::JSValToXPCException (ccx=@0xbfffc70c, s={asBits = 18446462629419587272, s = {payload = {i32 = 621975240, u32 = 621975240, boo = 621975240, str = 0x251296c8, obj = 0x251296c8, ptr = 0x251296c8, why = 621975240, word = 621975240}, tag = JSVAL_TAG_OBJECT}, asDouble = -nan(0xf0007251296c8), asPtr = 0x251296c8}, ifaceName=0x25275f80 "nsIDOMEventListener", methodName=0x88be38 "handleEvent", exceptn=0xbfffc4e4) at /work/mozilla/builds/2.0.0/mozilla/js/src/xpconnect/src/xpcconvert.cpp:1791
#10 0x05a6190d in nsXPCWrappedJSClass::CheckForException (ccx=@0xbfffc70c, aPropertyName=0x88be38 "handleEvent", anInterfaceName=0x25275f80 "nsIDOMEventListener", aForceReport=1) at /work/mozilla/builds/2.0.0/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1078
#11 0x05a64b8d in nsXPCWrappedJSClass::CallMethod (this=0x22d8c550, wrapper=0x25209f20, methodIndex=3, info=0x88be28, nativeParams=0xbfffcafc) at /work/mozilla/builds/2.0.0/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1735
#12 0x05a5b79f in nsXPCWrappedJS::CallMethod (this=0x25209f20, methodIndex=3, info=0x88be28, params=0xbfffcafc) at /work/mozilla/builds/2.0.0/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp:588

Comment 1

[Andreas Gal :gal]

Can you capture the test case?

Comment 2

[Bob Clary [:bc] (inactive)]

see the attachment. note linux 32bit also.

Comment 3

[David Mandelin [:dmandelin]]

Marking this blocking because compartment assertions are scary.

Comment 4

[Andreas Gal :gal]

Reproduced.

Comment 5

[Andreas Gal :gal]

Attachment: patch

Comment 6

[Andreas Gal :gal]

This patch re-wraps in resetCompartment. The rest is just fluff and extra assertions. I also made resetCompartment default to NULL instead of the defaultCompartment and adjusted some assertions so we crash instead of abusing the default compartment.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 501409 [details] [diff] [review]
patch

Cool, 2 nits:

>+    /*
>+     * Its safe to ignore the return value of wrapException here
>+     * since we know an exception was pending and that can't change.
>+     */
>+    if (isExceptionPending())
>+        (void) compartment->wrapException(this);
>+    return;

This doesn't seem to need a comment; the return value of wrapException is a success/failure bool.

> BEGIN_CASE(JSOP_EXCEPTION)
>-    JS_ASSERT(cx->throwing);
>-    PUSH_COPY(cx->exception);
>-    cx->throwing = JS_FALSE;
>+    JS_ASSERT(cx->isExceptionPending());
>+    PUSH_COPY(cx->getPendingException());

Can drop the assert

Comment 9

[Andreas Gal :gal]

I think that comment is definitely necessary. We should never ever ever under any circumstances swallow exceptions silently, except with a very good explanation.

Comment 10

[Luke Wagner [:luke]]

I mistyped: the return value of wrapException is *not* a success/failure bool: it just says "was an exception *already* thrown".

Comment 11

[Jason Orendorff [:jorendorff]]

Comment on attachment 501409 [details] [diff] [review]
patch

This is the line of code you want me to review, right?

>+    compartment = scopeobj->compartment();
>+    /*
>+     * Its safe to ignore the return value of wrapException here
>+     * since we know an exception was pending and that can't change.
>+     */
>+    if (isExceptionPending())
>+        (void) compartment->wrapException(this);

Looks good.

Contrary to Luke, I think this is tricky enough to warrant a comment. Blank line before it though, and it should say "It's safe to ignore the return value of wrapException here. If it fails, it clears this->exception and reports OOM, effectively upgrading the exception to an uncatchable OOM error."

Comment 12

[Jason Orendorff [:jorendorff]]

Ooh. Luke is right in comment 10. Still want a comment here. Maybe:  "If wrapException fails, it clears this->exception and reports OOM. The upshot is that we silently turn the exception into an uncatchable OOM error. A bit surprising, but the caller is just going to return false either way."

BTW, I think the call to wrapException() in AutoCompartment::leave becomes redundant, with this change, and should be removed.

Another call to wrapException(), in JSCrossCompartmentWrapper::construct, after call.leave(), is already redundant. Kill it too!

Comment 13

[Steve Fink [:sfink] [:s:]]

I just stumbled over comment 10 too. Any chance of adding to this patch a comment on wrapException explaining the return value? The code seemed wrong when I looked at it, because I was assuming success/fail like all the other wrap()s.

Comment 14

[Brendan Eich [:brendan]]

Lack of a cx leading parameter should mean infallible predicate, not fallible and therefore not-ignorable function or method.

Yeah, need a comment.

Don't need that final return; though.

/be

Comment 15

[Luke Wagner [:luke]]

(In reply to comment #14)
> Lack of a cx leading parameter should mean infallible predicate, not fallible
> and therefore not-ignorable function or method.
> 
> Yeah, need a comment.

Maybe I'm missing some sort of idiom here, but I viewed the return value of wrapException (in the context of JSCCW::construct) as just syntactic sugar for:

  bool shouldI_theCaller_returnTrueOrFalse = !cx->throwing;
  c->wrapException(cx);
  return shouldI_theCaller_returnTrueOrFalse;

which is nice.  So, by ignoring the return value you, are just waiving the sugar.  It sounds like everyone wants the comment so that's fine; I just wanted to explain that I'm not some sort of anti-comment Nazi.  (Is this an instance of Godwin's law if I refer to myself as a Nazi?)

Comment 16

[Brendan Eich [:brendan]]

You so Godwin'ed us there :-P.

So long as we propagate the right boolean or equivalent result, then a comment is good enough. The leading cx is of course |this| here, but the void return type means this method does not change failure status (cx->throwing). But callers have to propagate, which seems a burden on them that a local comment does not address.

Or maybe I'm missing something else. resetCompartment is a sharp tool, in any event.

/be

Comment 17

[Andreas Gal :gal]

comment 14: the return is necessary, there is an error goto label below it

Comment 18

[Brendan Eich [:brendan]]

(In reply to comment #17)
> comment 14: the return is necessary, there is an error goto label below it

Yeah, I looked at the patch after writing that. For a one-line nulling step I'd argue to avoid the downward goto and let the compiler tail-fuse. Drive-by nit!

/be

Comment 19

[Andreas Gal :gal]

thats what we did before, but it ended up diverging, I commoned it to make it obvious we handle errors the same way in both cases, not for perf/code size win

Comment 20

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/d58e45442c87

Comment 21

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/d56f9c2057ed
http://hg.mozilla.org/tracemonkey/rev/3d7dc9c88873

Comment 22

[Robert Sayre]

I backed this out because of tinderbox failures.

Comment 23

[Andreas Gal :gal]

New clean re-landing: http://hg.mozilla.org/tracemonkey/rev/e051f5f4c46a

Comment 24

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/d58e45442c87

Comment 25

[Chris Leary [:cdleary] (not checking bugmail)]

http://hg.mozilla.org/mozilla-central/rev/3d7dc9c88873
http://hg.mozilla.org/mozilla-central/rev/d56f9c2057ed

Comment 26

[Christian Holler (:decoder)]

Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.