699682 - "Assertion failure: length != 0,"

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: 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]]

Attachment: 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]]

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]]

Thanks for the quick review.

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

Comment 4

[Ed Morley [:emorley]]

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

Comment 6

[Bob Clary [:bc] (inactive)]

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)]

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] (NOT official MoCo now)]

This should also land on esr10.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

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]]

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]]

gkw says it doesn't go in cleanly. I'm cloning esr10 now to reassess.

Comment 12

[Lukas Blakk [:lsblakk] use ?needinfo]

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] (NOT official MoCo now)]

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

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]]

It does not apply cleanly. I'll re-assess this morning.

Comment 16

[Jason Orendorff [:jorendorff]]

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]]

Landed and stuck.
https://hg.mozilla.org/releases/mozilla-esr10/rev/c8ab4b92efd0

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

(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

[u279076]

Verified fixed in Firefox 10.0.5esrpre 2012-05-31 js-shell.

Comment 20

[Christian Holler (:decoder)]

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