Closed Bug 633741 Opened 9 years ago Closed 9 years ago

Assertion failure: !cx->isExceptionPending(), at ../jscntxtinlines.h:704


(Core :: JavaScript Engine, defect, critical)

Not set





(Reporter: jandem, Assigned: jandem)



(Keywords: assertion, testcase, Whiteboard: fixed-in-tracemonkey)


(1 file, 2 obsolete files)

delete Function;
This asserts without any jit flags:

$ ./js test.js
Assertion failure: !cx->isExceptionPending(), at ../jscntxtinlines.h:704
Attached patch Check defineProperty result (obsolete) — Splinter Review
With this patch we get:

test.js:3: TypeError: Object.getOwnPropertyNames(this) is not extensible

I don't know if this makes sense, but at least the patch shows where the exception occurs. global_enumerate calls JS_EnumerateStandardClasses, which calls js_InitFunctionAndObjectClasses.
Attached patch Check defineProperty result (obsolete) — Splinter Review
Attachment #511968 - Attachment is obsolete: true
Attachment #511972 - Flags: review?(jwalden+bmo)
Old regression (from bug 352604).

Jan, thanks for patching -- just need some braces around the "then" clause since the "if" condition is multiline. The assertion is pointing to a bug where someone forgot to check and propagate failure, so "this makes sense" (comment 1).

Blocks: 352604
Comment on attachment 511972 [details] [diff] [review]
Check defineProperty result

>diff --git a/js/src/tests/js1_8_5/regress/jstests.list b/js/src/tests/js1_8_5/regress/jstests.list
>+script regress-633741.js
>diff --git a/js/src/tests/js1_8_5/regress/regress-633741.js b/js/src/tests/js1_8_5/regress/regress-633741.js
>+delete Function;
>+try {
>+    /* Don't assert. */
>+    Object.getOwnPropertyNames(this);
>+} catch(e) {
>+reportCompare(0, 0, "ok");

The test matches what ES5 requires, but it's kind of a bit goofy how it does it.  |Object.getOwnPropertyNames(this)| shouldn't throw at all per ES5!  So how about if you add a |reportCompare(true, false, "this shouldn't have thrown");| inside the catch-block, then mark the test as failing?  It's still going to test for not asserting.  (I think -- please verify that a "fails" test doesn't treat a fatal assertion as an acceptable failure condition.  Intuition from the crash-whole-browser scenario suggests it doesn't, but best to be certain.)  It'll just do one better and check for per-spec behavior as well.

The patch doesn't really implement ES5's semantics, but that's a bit more involved certainly than this spot-fix.  In the meantime we definitely shouldn't assert, or drop an exception on the floor, or whatever, so this is an improvement even if it's not fully correct.

Anyway, if you could post a patch with these changes, plus the bracing, I'd appreciate it.  For a trivial spot-fix like this there's no reason we can't get it landed before release if we have the time.
Attachment #511972 - Flags: review?(jwalden+bmo) → review+
Attached patch PatchSplinter Review
Thanks for the reviews. This does not throw in the browser so I marked it as fails-if( I also confirmed that reverting the fix still makes the test fail; this is much nicer indeed.
Assignee: general → jandemooij
Attachment #511972 - Attachment is obsolete: true
Attachment #512141 - Flags: review?(jwalden+bmo)
Comment on attachment 512141 [details] [diff] [review]

Excellent.  I poked jorendorff about landing this, doubt I have time.
Attachment #512141 - Flags: review?(jwalden+bmo)
Attachment #512141 - Flags: review+
Attachment #512141 - Flags: approval2.0?
Attachment #512141 - Flags: approval2.0? → approval2.0+
Keywords: checkin-needed
OS: Mac OS X → All
Whiteboard: fixed-in-tracemonkey
Target Milestone: --- → mozilla2.0b12
Keywords: checkin-needed
(In reply to comment #7)

-  assertEq(e instanceof TypeError, true, "expected TypeError, got: " + e);
+  reportCompare(e instanceof TypeError, true, "expected TypeError, got: " + e);

I learned from jorendorff that reportCompare takes (expect, actual) -- opposite order from assertEq. rs=me on fix.

Brendan, I think that's bug 621432, right?
(In reply to comment #9)
> Brendan, I think that's bug 621432, right?

I saw it here:

which is not this bug's cset, nor the one for bug 621432. Waldo, which bug number is the right one for 902e844c46bd ?

Oops, my bad -- that was a followup to bug 621432.  Sorry about that!
Closed: 9 years ago
Resolution: --- → FIXED
FYI, bug 642772 fixed regress-633741.js.
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-633741.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.