Comment 1 Jesse Ruderman 2011-10-20 09:21:16 PDT

Might be a regression from bug 695238.

Comment 2 Gary Kwong [:gkw] [:nth10sd] 2011-10-20 09:51:36 PDT

(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] 2011-10-20 11:46:01 PDT

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] 2011-10-20 14:41:51 PDT

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

Comment 5 Jason Orendorff [:jorendorff] 2011-10-20 14:50:44 PDT

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] 2011-10-20 15:03:21 PDT

> 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] 2011-10-26 16:22:26 PDT

(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] 2011-10-26 16:47:33 PDT

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

Comment 9 Ed Morley [:emorley] 2011-10-26 18:01:37 PDT

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] 2011-10-26 20:53:37 PDT

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

Comment 11 Marco Bonardo [::mak] 2011-10-27 01:58:12 PDT

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

Comment 12 Christian Holler (:decoder) 2013-01-04 13:19:31 PST

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] 2013-01-10 16:38:21 PST

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