Closed Bug 696109 Opened 10 years ago Closed 10 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla10

People

(Reporter: gkw, Unassigned)

References

Details

(Keywords: assertion, regression, testcase, Whiteboard: js-triage-needed)

Attachments

(2 files)

Attached file 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.
Might be a regression from bug 695238.
(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
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
(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.
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/mozilla-central/rev/f944c4c67b78
Status: NEW → RESOLVED
Closed: 10 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+
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.