Last Comment Bug 699682 - "Assertion failure: length != 0,"
: "Assertion failure: length != 0,"
Status: VERIFIED FIXED
js-triage-needed [qa+] [testday-20120...
: assertion, regression, testcase, verified-beta
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Mac OS X
: -- critical (vote)
: mozilla11
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
: 701768 (view as bug list)
Depends on:
Blocks: jsfunfuzz 532972 694306 701768
  Show dependency treegraph
 
Reported: 2011-11-03 18:25 PDT by Gary Kwong [:gkw] [:nth10sd]
Modified: 2013-01-04 13:11 PST (History)
10 users (show)
choller: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
affected
verified
13+
verified


Attachments
stack (5.98 KB, text/plain)
2011-11-03 18:25 PDT, Gary Kwong [:gkw] [:nth10sd]
no flags Details
v1 (4.30 KB, patch)
2011-11-17 11:54 PST, Jason Orendorff [:jorendorff]
jwalden+bmo: review+
akeybl: approval‑mozilla‑esr10+
Details | Diff | Splinter Review

Description Gary Kwong [:gkw] [:nth10sd] 2011-11-03 18:25:40 PDT
Created attachment 571864 [details]
stack

"\\"/{''}

asserts js debug shell on m-c changeset 6cbeabc07c59 without any CLI flags at Assertion failure: length != 0,

autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   79633:50f566a3b449
user:        Jason Orendorff
date:        Wed Nov 02 14:30:58 2011 -0500
summary:     Bug 694306 - Keywords should not be permitted in destructuring shorthand. r=Waldo.
Comment 1 Jason Orendorff [:jorendorff] 2011-11-17 11:54:45 PST
Created attachment 575255 [details] [diff] [review]
v1

This isn't the shortest fix but I think it's the sanest. Only call checkForKeyword on an atom produced by a NAME token.

Waldo, IIRC you have a Parser::variables() rewriting in midflight; you might need to update that to include this fix in variables() as well as primaryExpr().
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-11-17 14:47:13 PST
Comment on attachment 575255 [details] [diff] [review]
v1

Review of attachment 575255 [details] [diff] [review]:
-----------------------------------------------------------------

My work is going to hopelessly conflict with just about anything in the penumbras here.  The first rebase of it, once I have everything passing tests, should be quite interesting.
Comment 3 Jason Orendorff [:jorendorff] 2011-11-18 13:55:34 PST
Thanks for the quick review.

https://hg.mozilla.org/integration/mozilla-inbound/rev/dbc8a19dd0dd
Comment 4 Ed Morley [:emorley] 2011-11-19 05:12:19 PST
https://hg.mozilla.org/mozilla-central/rev/dbc8a19dd0dd
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-12-16 15:47:05 PST
*** Bug 701768 has been marked as a duplicate of this bug. ***
Comment 6 Bob Clary [:bc:] 2011-12-25 17:06:39 PST
Reproducible on Beta/10 all platforms. I checked in a nightly Beta build on Mac and didn't crash.

http://www.ria.ru/society/20111224/525177803.html
http://www.ria.ru/culture/20111224/525166390.html
Comment 7 Mihaela Velimiroviciu (:mihaelav) 2012-02-03 05:17:49 PST
Mac OS X 10.6

I have built the js shell for the latest beta an run the test case from comment #0 (./js testcase, where testcase contains only "\\"/{''} ). The assertion fail is not reproducible anymore. 
Marking as VERIFIED.
Comment 8 Gary Kwong [:gkw] [:nth10sd] 2012-04-24 17:00:41 PDT
This should also land on esr10.
Comment 9 Gary Kwong [:gkw] [:nth10sd] 2012-04-24 17:04:34 PDT
Comment on attachment 575255 [details] [diff] [review]
v1

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: jsfunfuzz finds this really easily, in a short period of time after being run.
User impact if declined: Open sourcing jsfunfuzz might lead to more bug duplicates of this one.
Fix Landed on Version: 11

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

jorendorff, perhaps you'd like to answer these portions?

Risk to taking this patch (and alternatives if risky): 
String changes made by this patch:
Comment 10 Jason Orendorff [:jorendorff] 2012-04-25 12:26:14 PDT
If this goes into esr10 cleanly, the risk is practically nil. If not I'll be happy to re-assess.

No string changes.
Comment 11 Jason Orendorff [:jorendorff] 2012-04-25 12:37:11 PDT
gkw says it doesn't go in cleanly. I'm cloning esr10 now to reassess.
Comment 12 Lukas Blakk [:lsblakk] use ?needinfo 2012-04-25 13:00:32 PDT
same as with bug 686296, I need to understand why this would be needed on ESR. it's not a security bug or a user pain issue that i can see - so please explain reasoning here.
Comment 13 Gary Kwong [:gkw] [:nth10sd] 2012-04-25 13:04:45 PDT
(In reply to Lukas Blakk [:lsblakk] from comment #12)
> same as with bug 686296, I need to understand why this would be needed on
> ESR. it's not a security bug or a user pain issue that i can see - so please
> explain reasoning here.

This assert occurs somewhat frequently enough similar to bug 697279 that it masks other potential problems due to the testcase simplicity.

It's just:

"\\"/{''}

Again, I can put this assert on suppression, but the value of jsfunfuzz'ing ESR 10 is diminished since this assert will just be easily triggered.

Or I could remove it from my source tree, but i'd like not to have to patch my local source.
Comment 14 Alex Keybl [:akeybl] 2012-04-25 13:24:20 PDT
Comment on attachment 575255 [details] [diff] [review]
v1

[Triage Comment]
With near-zero risk to the ESR and benefit internally, approved for the ESR.
Comment 15 Jason Orendorff [:jorendorff] 2012-05-01 08:08:01 PDT
It does not apply cleanly. I'll re-assess this morning.
Comment 16 Jason Orendorff [:jorendorff] 2012-05-01 17:06:34 PDT
Applies and builds cleanly after changing "PNK_" back to "TOK_". So my "near-zero risk" assessment from comment 10 stands.

I've never landed anything on mozilla-esr10, but looking at
  https://wiki.mozilla.org/Release_Management/ESR_Landing_Process
I don't see anything this patch lacks. Unless someone objects, I will land it on mozilla-esr10 tomorrow morning.
Comment 17 Jason Orendorff [:jorendorff] 2012-05-02 15:27:37 PDT
Landed and stuck.
https://hg.mozilla.org/releases/mozilla-esr10/rev/c8ab4b92efd0
Comment 18 Gary Kwong [:gkw] [:nth10sd] 2012-05-02 18:35:55 PDT
(In reply to Jason Orendorff [:jorendorff] from comment #17)
> Landed and stuck.
> https://hg.mozilla.org/releases/mozilla-esr10/rev/c8ab4b92efd0

Thank you!
Comment 19 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-06-03 10:42:25 PDT
Verified fixed in Firefox 10.0.5esrpre 2012-05-31 js-shell.
Comment 20 Christian Holler (:decoder) 2013-01-04 13:11:01 PST
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-699682.js.

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