"Assertion failure: !cx->isExceptionPending()," with Reflect.parse

VERIFIED FIXED in mozilla10

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: gkw, Unassigned)

Tracking

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

Trunk
mozilla10
x86
Linux
assertion, regression, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: js-triage-needed)

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
Created attachment 568410 [details]
stack

Reflect.parse("with({c})p")

asserts js debug shell on m-c changeset 311fdb9b38b7 without any CLI flags at Assertion failure: !cx->isExceptionPending(),

autoBisect is now running.

Comment 1

6 years ago
Might be a regression from bug 695238.
(Reporter)

Comment 2

6 years ago
(In reply to Jesse Ruderman from comment #1)
> Might be a regression from bug 695238.

That's correct.

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   78876:feeee0906588
user:        Dave Herman
date:        Mon Oct 17 21:09:56 2011 -0700
summary:     Bug 695238 - Reflect.parse should throw when an object literal is missing a property RHS. r=jorendorff
Found it. Turns out that patch surfaced a bug that had been sitting around for a while. ASTSerializer::statement has a precedence bug in the TOK_WITH | TOK_WHILE case (missing parentheses around the ? : expression).

Patch forthcoming.

Thanks,
Dave
Created attachment 568523 [details] [diff] [review]
C++ precedence bug

Updated

6 years ago
Attachment #568523 - Flags: review?(jorendorff)
Comment on attachment 568523 [details] [diff] [review]
C++ precedence bug

Please make a separate 6-line test rather than adding to reflect-parse.js.

reflect-parse.js is awesome, and I only wish our whole test suite were that thorough -- but because it's so long, once it fails I still don't know what I broke. I have to make a copy of it and manually reduce. So let's not make it longer.

r=me with that.
Attachment #568523 - Flags: review?(jorendorff) → review+
> Please make a separate 6-line test rather than adding to reflect-parse.js.

Good call.

> reflect-parse.js is awesome, and I only wish our whole test suite were that
> thorough -- but because it's so long, once it fails I still don't know what
> I broke. I have to make a copy of it and manually reduce. So let's not make
> it longer.

Is it worth separating out chunks of reflect-parse.js? Separate directory for it, divided into a few files by theme? Am I really asking you to ask me to do more work?

Dave
(Reporter)

Updated

6 years ago
Blocks: 695238
(In reply to Dave Herman [:dherman] from comment #6)
> Is it worth separating out chunks of reflect-parse.js? Separate directory
> for it, divided into a few files by theme?

If you want to, rs=me. There's no need to hold this up over that, though.
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5cc34a9351d

Comment 9

6 years ago
Backed out for reftest failure:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=d5cc34a9351d
{
REFTEST TEST-UNEXPECTED-FAIL | file:///home/cltbld/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=js1_8_5/extensions/regress-696109.js | Unknown file:///home/cltbld/talos-slave/test/build/jsreftest/tests/js1_8_5/extensions/regress-696109.js:10: Reflect is not defined item 1
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/5b8e29b39c7f

philor/mak says likely fix is to add |skip-if(!xulRuntime.shell)| to the test in jstests.list.
https://hg.mozilla.org/integration/mozilla-inbound/rev/f944c4c67b78
https://hg.mozilla.org/mozilla-central/rev/f944c4c67b78
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla10
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-696109.js.
Flags: in-testsuite+
(Reporter)

Comment 13

5 years ago
Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.