Bug 43401

Summary: REGRESSION: calling unshift passing more than 1 argument causes array corruption
Product: WebKit Reporter: Heather <heather162>
Component: JavaScriptCoreAssignee: Michael Saboff <msaboff>
Status: RESOLVED FIXED    
Severity: Major CC: ap, commit-queue, ggaren, msaboff, oliver
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.5   
Attachments:
Description Flags
array unshift test case
none
Patch to fix initialization of array contents as part of unshift.
barraclough: review-
Updated patch with fix for <tabs> in source and inclusion of new regression tests.
none
Updated patch with changes suggested in review of attachment 63467 none

Description Heather 2010-08-02 20:23:17 PDT
Created attachment 63295 [details]
array unshift test case

In the latest nightly calling unshift on an array and passing more than one argument causes the array to become corrupted after the 3rd call to unshift.
Comment 1 Michael Saboff 2010-08-03 19:50:27 PDT
Created attachment 63406 [details]
Patch to fix initialization of array contents as part of unshift.
Comment 2 Gavin Barraclough 2010-08-03 20:04:19 PDT
Comment on attachment 63406 [details]
Patch to fix initialization of array contents as part of unshift.

Looks fine but please remove tabs & add layout test as discussed.
cheers, G.
Comment 3 Michael Saboff 2010-08-04 11:07:26 PDT
Created attachment 63467 [details]
Updated patch with fix for <tabs> in source and inclusion of new regression tests.
Comment 4 Darin Adler 2010-08-04 11:13:10 PDT
Comment on attachment 63467 [details]
Updated patch with fix for <tabs> in source and inclusion of new regression tests.

Looks good. Nice test too.

> +    for (unsigned i = 0; i < (unsigned)count; i++)

We normally don't use C-style casts, so this should be a static_cast, or even better you could just use the same type for "i" as for "count" and avoid the cast entirely.

I’ll leave this on commit-queue? so you can decide whether to fix this. If you choose to upload a new patch, then I'll set commit-queue+ on that, or I can set commit-queue+ on this if you want to land it without changing the typecast above.

Not sure why you are using shouldEvaluateTo instead of shouldBe in the test. What's the difference?

The patch was uploaded with the wrong MIME type and without the "is a patch" check box set; I had to fix those before I could do this review.
Comment 5 Michael Saboff 2010-08-04 11:34:39 PDT
Created attachment 63470 [details]
Updated patch with changes suggested in review of attachment 63467 [details]
Comment 6 Eric Seidel (no email) 2010-08-05 08:53:05 PDT
Comment on attachment 63467 [details]
Updated patch with fix for <tabs> in source and inclusion of new regression tests.

Cleared Darin Adler's review+ from obsolete attachment 63467 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 7 WebKit Commit Bot 2010-08-05 12:34:39 PDT
Comment on attachment 63470 [details]
Updated patch with changes suggested in review of attachment 63467 [details]

Clearing flags on attachment: 63470

Committed r64773: <http://trac.webkit.org/changeset/64773>
Comment 8 WebKit Commit Bot 2010-08-05 12:34:44 PDT
All reviewed patches have been landed.  Closing bug.