Assertion failure: compartment mismatched with pending exception

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: bc, Assigned: gal)

Tracking

(Blocks: 1 bug, {assertion, reproducible, testcase})

Trunk
x86
All
assertion, reproducible, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [compartments][hardblocker], fixed-in-tracemonkey, URL)

Attachments

(2 attachments)

(Reporter)

Description

7 years ago
Created attachment 500167 [details]
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
(Assignee)

Comment 1

7 years ago
Can you capture the test case?
(Reporter)

Comment 2

7 years ago
see the attachment. note linux 32bit also.
OS: Mac OS X → All
blocking2.0: --- → ?
Marking this blocking because compartment assertions are scary.
blocking2.0: ? → final+
(Assignee)

Comment 4

7 years ago
Reproduced.
Whiteboard: [compartments] → [compartments][hardblocker]
Assignee: general → gal
Blocks: 605662

Updated

7 years ago
No longer blocks: 605662
Blocks: 605662
(Assignee)

Comment 5

7 years ago
Created attachment 501409 [details] [diff] [review]
patch
(Assignee)

Updated

7 years ago
Attachment #501409 - Flags: review?(jorendorff)
(Assignee)

Updated

7 years ago
Attachment #501409 - Flags: review?(lw)
(Assignee)

Comment 6

7 years ago
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.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 609508
(Assignee)

Updated

7 years ago
blocking2.0: final+ → betaN+

Comment 8

7 years ago
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
Attachment #501409 - Flags: review?(lw) → review+
(Assignee)

Comment 9

7 years ago
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

7 years ago
I mistyped: the return value of wrapException is *not* a success/failure bool: it just says "was an exception *already* thrown".
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."
Attachment #501409 - Flags: review?(jorendorff) → review+
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!
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.
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

7 years ago
(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?)
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
(Assignee)

Comment 17

7 years ago
comment 14: the return is necessary, there is an error goto label below it
(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
(Assignee)

Comment 19

7 years ago
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
(Assignee)

Comment 20

7 years ago
http://hg.mozilla.org/tracemonkey/rev/d58e45442c87
Whiteboard: [compartments][hardblocker] → [compartments][hardblocker], fixed-in-tracemonkey
(Assignee)

Comment 21

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

Comment 22

7 years ago
I backed this out because of tinderbox failures.
Whiteboard: [compartments][hardblocker], fixed-in-tracemonkey → [compartments][hardblocker]
(Assignee)

Comment 23

7 years ago
New clean re-landing: http://hg.mozilla.org/tracemonkey/rev/e051f5f4c46a
(Assignee)

Updated

7 years ago
Whiteboard: [compartments][hardblocker] → [compartments][hardblocker], fixed-in-tracemonkey
http://hg.mozilla.org/mozilla-central/rev/d58e45442c87
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
http://hg.mozilla.org/mozilla-central/rev/3d7dc9c88873
http://hg.mozilla.org/mozilla-central/rev/d56f9c2057ed
Filter on qa-project-auto-change:

Bug in removed tracer code, setting in-testsuite- flag.
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.