Last Comment Bug 696109 - "Assertion failure: !cx->isExceptionPending()," with Reflect.parse
: "Assertion failure: !cx->isExceptionPending()," with Reflect.parse
Status: VERIFIED FIXED
js-triage-needed
: assertion, regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- critical (vote)
: mozilla10
Assigned To: general
:
Mentors:
Depends on:
Blocks: jsfunfuzz 695238
  Show dependency treegraph
 
Reported: 2011-10-20 09:00 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-10 16:38 PST (History)
8 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
stack (2.88 KB, text/plain)
2011-10-20 09:00 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
C++ precedence bug (2.14 KB, patch)
2011-10-20 14:41 PDT, Dave Herman [:dherman]
jorendorff: review+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-10-20 09:00:08 PDT
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 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 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 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.

Note You need to log in before you can comment on or make changes to this bug.