Bug 43401 - REGRESSION: calling unshift passing more than 1 argument causes array corruption
Summary: REGRESSION: calling unshift passing more than 1 argument causes array corruption
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh Intel OS X 10.5
: P1 Major
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-08-02 20:23 PDT by Heather
Modified: 2010-08-05 12:34 PDT (History)
5 users (show)

See Also:


Attachments
array unshift test case (290 bytes, text/html)
2010-08-02 20:23 PDT, Heather
no flags Details
Patch to fix initialization of array contents as part of unshift. (1.43 KB, patch)
2010-08-03 19:50 PDT, Michael Saboff
barraclough: review-
Details | Formatted Diff | Diff
Updated patch with fix for <tabs> in source and inclusion of new regression tests. (5.60 KB, patch)
2010-08-04 11:07 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff
Updated patch with changes suggested in review of attachment 63467 (5.51 KB, patch)
2010-08-04 11:34 PDT, Michael Saboff
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.