crash using array.splice on an array with some non-set elements [@ js_GetGCThingTraceKind ]

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
JavaScript Engine
--
critical
VERIFIED FIXED
9 years ago
5 years ago

People

(Reporter: Timothee Groleau, Assigned: mrbkap)

Tracking

(6 keywords)

Trunk
mozilla1.9.2a1
crash, regression, testcase, topcrash, verified1.9.0.7, verified1.9.1
Points:
---
Bug Flags:
blocking1.9.2 -
blocking1.9.1 +
wanted1.9.0.x +
wanted1.8.1.x -
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] fixed-in-tracemonkey, post 1.8=branch, crash signature)

Attachments

(4 attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111419 Gentoo Firefox/3.0.4
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.4) Gecko/2008111419 Gentoo Firefox/3.0.4

using this simple javascript lines crash FF3.0.4:

var a = new Array(999);
a[6] = "toto";
a.splice(6, 1);

Reproducible: Always

Steps to Reproduce:
1) set a simple html document with the following code (intentionally left bare for brevity, bug occurs with proper doctype set, etc.)
======
<html>
<body>
<script>
var a = new Array(999);
a[6] = "toto";
a.splice(6, 1);
</script>
</body>
</html>
======

2) open the page in FF and wait

3) browser will crash within a few seconds
Actual Results:  
crash, sometimes with some delay of 5-10 seconds or more (!)

Expected Results:  
no crash
array should contain no element but have length 998

It seems the script does work before the crash, so I'm not sure what is going wrong. Adding an alert shows that the length has been updated, as expected:

var a = new Array(999);
a[6] = "toto";
a.splice(6, 1);
alert(a.length); // <- shows 998, as expected


Crash happens on FF windows as well
Confirmed with trunk, Shiretoko, Firefox 3.0.4 on Windows XP. 
It is not that easy to reproduce. (Many) reloads and opening/closing error console seems to trigger it.
Status: UNCONFIRMED → NEW
Component: General → General
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
QA Contact: general → general
Hardware: PC → All
Version: unspecified → Trunk
Regression range: http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2008-02-18+02%3A00&maxdate=2008-02-19+01%3A00
Bug 322889 or bug 418239? 
Crash ID: http://crash-stats.mozilla.com/report/index/58f54169-bcfa-4845-a867-f84ca2081202
Assignee: nobody → general
Component: General → JavaScript Engine
QA Contact: general → general
Created attachment 350976 [details]
reporter's testcase

STR:
- load page
- click 10x reload
- click tools 
- repeat the last two steps
Severity: normal → critical
Keywords: crash, regression
Summary: crash using array.splice on an array with some non-set elements → crash using array.splice on an array with some non-set elements [@ js_GetGCThingTraceKind ]
Flags: blocking1.9.2?
Flags: blocking1.9.1?
Flags: blocking1.9.0.6?
Doesn't sound like a branch blocker if it's as flaky as comment 1 and 3 say, but when this is fixed on the trunk we'll happily look at branch approval requests.
Flags: blocking1.9.0.6? → wanted1.9.0.x+

Updated

9 years ago
Flags: blocking1.9.2?
Flags: blocking1.9.2-
Flags: blocking1.9.1?
Flags: blocking1.9.1-
This bug is on #64 in the top crasher list for 1.9.1 and 1.9.2a1pre.

Don't we really wanna have this fixed for 1.9.2 and even 1.9.1?
Keywords: topcrash

Comment 6

9 years ago
looks similar to a bug we're seeing at deviantART.com which is generating a lot of reports from our users ... I'm trying to make another test case.
Re-nominating in light of frequency. This also should be considered on memory safety grounds -- without more analysis it has to be presumed exploitable. Sayrer can you help get it assigned? Thanks,

/be
Flags: blocking1.9.1- → blocking1.9.1?

Comment 8

9 years ago
I have not been able to reproduce this in 3.1 beta 2 but it's definitely happening in 3.0.5

It also seems to be more easily reproducible with firebug enabled :(
Flags: blocking1.9.1?

Comment 9

9 years ago
I was wrong, it does happen in 3.1 beta - http://crash-stats.mozilla.com/report/index/34cc7139-14d0-4e40-91c1-3c9112090125?p=1
Please don't remove the blocking flag. It's highly reproducible here with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090124 Shiretoko/3.1b3pre Ubiquity/0.1.5 ID:20090124020525
Flags: blocking1.9.1?
Opening the testcase with the above mentioned build gives a bit different stack:

0  	libmozjs.dylib  JS_CallTracer  	js/src/jsgc.cpp:1085
1 	libmozjs.dylib 	array_trace 	js/src/jsarray.cpp:1069
2 	libmozjs.dylib 	JS_CallTracer 	js/src/jsgc.cpp:2692
3 	libmozjs.dylib 	js_TraceObject 	js/src/jsobj.cpp:5322
4 	libmozjs.dylib 	JS_CallTracer 	js/src/jsgc.cpp:2692
5 	libmozjs.dylib 	js_TraceObject 	js/src/jsobj.cpp:5322

Comment 12

9 years ago
sorry didn't mean to remove the flag... not sure how that happened.

The quickest way I've found to reproduce it is setting a breakpoint in the testcase code and calling that repeatedly from a loop, then just stepping through the code in firebug and boom, crashes right away.

Comment 13

9 years ago
Created attachment 358770 [details]
modified testcase

my modified testcase, calls the original testcase code in a loop...
(Assignee)

Updated

9 years ago
Assignee: general → mrbkap
(Assignee)

Comment 14

9 years ago
Created attachment 358806 [details] [diff] [review]
Fix

The actual bug fix here is passing |oldsize| to ResizeSlots, which was failing to initialize the newly-allocated spaces. I've also avoided calling ResizeSlots in the case that we're not going to be shrinking the slots anyway.

I'm not sure who's doing reviews in this code nowadays, so the first of crowder and shaver to get to this should take it!
Attachment #358806 - Flags: review?(shaver)
Attachment #358806 - Flags: review?(crowder)

Comment 15

9 years ago
Comment on attachment 358806 [details] [diff] [review]
Fix

Hm.  I approve of the additional length read, if your debugging shows it to be necessary, but is it actually necessary that we don't shrink the array here?  If we can, I think we should.
Attachment #358806 - Flags: review?(crowder) → review+
Comment on attachment 358806 [details] [diff] [review]
Fix

Did I not make ResizeSlots early-out if it didn't really need to resize? Bad shaver. :(
Attachment #358806 - Flags: review?(shaver) → review+
(Assignee)

Comment 17

9 years ago
http://hg.mozilla.org/tracemonkey/rev/081bfaae5839
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 18

9 years ago
Comment on attachment 358806 [details] [diff] [review]
Fix

I'm not sure if this is going to get blocking+ status, but IMO the severity and frequency warrant a 1.9.1 landing.
Attachment #358806 - Flags: approval1.9.1?
Comment on attachment 358806 [details] [diff] [review]
Fix

yeah, and we don't want to fix it on 1.9.0.x without having fixed in 1.9.1. a=shaver
Attachment #358806 - Flags: approval1.9.1? → approval1.9.1+
(Assignee)

Comment 20

9 years ago
Created attachment 358965 [details] [diff] [review]
Patch for the 1.9.0 branch

This is the patch with the filename fixed up.
Attachment #358965 - Flags: review+
Attachment #358965 - Flags: approval1.9.0.7?

Comment 21

9 years ago
http://hg.mozilla.org/mozilla-central/rev/081bfaae5839
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Flags: blocking1.9.1? → blocking1.9.1+
Resolution: --- → FIXED
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey [needs 1.9.1 landing before 1.9.0 approval]
No crash anymore with the attached testcases on trunk. Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090128 Minefield/3.2a1pre

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090129 Minefield/3.2a1pre
Status: RESOLVED → VERIFIED
Target Milestone: --- → mozilla1.9.2a1
When is this landing on 1.9.1?
(Assignee)

Comment 24

9 years ago
Now: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d320b89f6f92
Keywords: fixed1.9.1
Whiteboard: fixed-in-tracemonkey [needs 1.9.1 landing before 1.9.0 approval] → fixed-in-tracemonkey
Comment on attachment 358965 [details] [diff] [review]
Patch for the 1.9.0 branch

Approved for 1.9.0.7, a=dveditz for release-drivers.
Attachment #358965 - Flags: approval1.9.0.7? → approval1.9.0.7+
(Assignee)

Comment 26

9 years ago
Fixed on the 1.9.0 branch.
Keywords: fixed1.9.0.7
Verified fixed on the 1.9.1 and 1.9.0 branch with:

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20090204 Shiretoko/3.1b3pre ID:20090204020327

Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.0.7pre) Gecko/2009020405 GranParadiso/3.0.7pre

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.7pre) Gecko/2009020404 GranParadiso/3.0.7pre

Mukunda, can you please cross-verify with one of the latest nightly builds? Thanks! You can download here: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2009/02/
Flags: in-testsuite?
Keywords: fixed1.9.0.7, fixed1.9.1 → verified1.9.0.7, verified1.9.1
Flags: wanted1.8.1.x-
Whiteboard: fixed-in-tracemonkey → fixed-in-tracemonkey, post 1.8=branch
Whiteboard: fixed-in-tracemonkey, post 1.8=branch → [sg:critical?] fixed-in-tracemonkey, post 1.8=branch

Updated

9 years ago
Keywords: testcase
Crash Signature: [@ js_GetGCThingTraceKind ]
Flags: in-testsuite? → in-testsuite-
You need to log in before you can comment on or make changes to this bug.