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

Status: VERIFIED FIXED

Description

[Gary Kwong [:gkw] [:nth10sd]]

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

[Jesse Ruderman]

Might be a regression from bug 695238.

Comment 2

[Gary Kwong [:gkw] [:nth10sd]]

(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

Comment 3

[Dave Herman [:dherman]]

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

Comment 4

[Dave Herman [:dherman]]

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

Comment 5

[Jason Orendorff [: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.

Comment 6

[Dave Herman [:dherman]]

> 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

Comment 7

[Jason Orendorff [:jorendorff]]

(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.

Comment 8

[Dave Herman [:dherman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d5cc34a9351d

Comment 9

[Ed Morley [:emorley]]

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.

Comment 10

[Dave Herman [:dherman]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f944c4c67b78

Comment 11

[Marco Bonardo [::mak]]

https://hg.mozilla.org/mozilla-central/rev/f944c4c67b78

Comment 12

[Christian Holler (:decoder)]

A testcase for this bug was automatically identified at js/src/tests/js1_8_5/extensions/regress-696109.js.

Comment 13

[Gary Kwong [:gkw] [:nth10sd]]

Testcases have been landed by virtue of being marked in-testsuite+ -> VERIFIED as well.