"Assertion failure: length != 0,"

VERIFIED FIXED in Firefox 11

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
6 years ago
4 years ago

People

(Reporter: gkw, Assigned: jorendorff)

Tracking

(Blocks: 2 bugs, 4 keywords)

Trunk
mozilla11
x86
Mac OS X
assertion, regression, testcase, verified-beta
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox10 affected, firefox11 verified, firefox-esr1013+ verified)

Details

(Whiteboard: js-triage-needed [qa+] [testday-20120203])

Attachments

(2 attachments)

(Reporter)

Description

6 years ago
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.
Blocks: 701768
(Assignee)

Updated

6 years ago
Assignee: general → jorendorff
(Assignee)

Comment 1

6 years ago
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().
Attachment #575255 - Flags: review?(jwalden+bmo)
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.
Attachment #575255 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 3

6 years ago
Thanks for the quick review.

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

Comment 4

6 years ago
https://hg.mozilla.org/mozilla-central/rev/dbc8a19dd0dd
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
Duplicate of this bug: 701768

Comment 6

5 years ago
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
Blocks: 532972
status-firefox10: --- → affected
status-firefox11: --- → fixed
Whiteboard: js-triage-needed → js-triage-needed [qa+]
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.
Status: RESOLVED → VERIFIED
Keywords: verified-beta
Whiteboard: js-triage-needed [qa+] → js-triage-needed [qa!]
Whiteboard: js-triage-needed [qa!] → js-triage-needed [qa!] [testday-20120203]
(Reporter)

Comment 8

5 years ago
This should also land on esr10.
status-firefox-esr10: --- → affected
tracking-firefox-esr10: --- → ?
(Reporter)

Comment 9

5 years ago
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:
Attachment #575255 - Flags: approval-mozilla-esr10?
(Assignee)

Comment 10

5 years ago
If this goes into esr10 cleanly, the risk is practically nil. If not I'll be happy to re-assess.

No string changes.
(Assignee)

Comment 11

5 years ago
gkw says it doesn't go in cleanly. I'm cloning esr10 now to reassess.
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.
(Reporter)

Comment 13

5 years ago
(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.

Updated

5 years ago
tracking-firefox-esr10: ? → 13+
Comment on attachment 575255 [details] [diff] [review]
v1

[Triage Comment]
With near-zero risk to the ESR and benefit internally, approved for the ESR.
Attachment #575255 - Flags: approval-mozilla-esr10? → approval-mozilla-esr10+
(Assignee)

Comment 15

5 years ago
It does not apply cleanly. I'll re-assess this morning.
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Landed and stuck.
https://hg.mozilla.org/releases/mozilla-esr10/rev/c8ab4b92efd0
(Assignee)

Updated

5 years ago
status-firefox-esr10: affected → fixed
(Reporter)

Comment 18

5 years ago
(In reply to Jason Orendorff [:jorendorff] from comment #17)
> Landed and stuck.
> https://hg.mozilla.org/releases/mozilla-esr10/rev/c8ab4b92efd0

Thank you!
status-firefox11: fixed → verified
Whiteboard: js-triage-needed [qa!] [testday-20120203] → js-triage-needed [qa+] [testday-20120203]
Verified fixed in Firefox 10.0.5esrpre 2012-05-31 js-shell.
status-firefox-esr10: fixed → verified
A testcase for this bug was automatically identified at js/src/tests/js1_8_5/regress/regress-699682.js.
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.