Closed Bug 467499 Opened 16 years ago Closed 16 years ago

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

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.9.2a1

People

(Reporter: timothee, Assigned: mrbkap)

Details

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

Crash Data

Attachments

(4 files)

reporter's testcase
113 bytes, text/html
Details
modified testcase
410 bytes, text/html
Details
Fix
968 bytes, patch
crowderbt
: review+
shaver
: review+
shaver
: approval1.9.1+
Details | Diff | Splinter Review
Patch for the 1.9.0 branch
977 bytes, patch
mrbkap
: review+
dveditz
: approval1.9.0.7+
Details | Diff | Splinter Review 
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
Ever confirmed: true
OS: Linux → All
Product: Firefox → Core
QA Contact: general → general
Hardware: PC → All
Version: unspecified → Trunk
Attached file 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+
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
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?
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?
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
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.
Attached file modified testcase — 
my modified testcase, calls the original testcase code in a loop...
Assignee: general → mrbkap
Attached patch Fix — — Splinter Review 
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 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+
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+
This is the patch with the filename fixed up.
Attachment #358965 - Flags: review+
Attachment #358965 - Flags: approval1.9.0.7?
http://hg.mozilla.org/mozilla-central/rev/081bfaae5839
Status: NEW → RESOLVED
Closed: 16 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?
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+
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
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.

Attachment

General

Creator:
Created:
Updated:
Size: