Bug 43401

Summary:

REGRESSION: calling unshift passing more than 1 argument causes array corruption

Product:

WebKit

Reporter:

Heather <heather162>

Component:

JavaScriptCore

Assignee:

Michael Saboff <msaboff>

Status:

RESOLVED FIXED

Severity:

Major

CC:

ap, commit-queue, ggaren, msaboff, oliver

Priority:

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

Heather

Reported 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.

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
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Michael Saboff

Comment 1 2010-08-03 19:50:27 PDT

Created attachment 63406 [details] Patch to fix initialization of array contents as part of unshift.

Gavin Barraclough

Comment 2 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.

Michael Saboff

Comment 3 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.

Darin Adler

Comment 4 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.

Michael Saboff

Comment 5 2010-08-04 11:34:39 PDT

Created attachment 63470 [details] Updated patch with changes suggested in review of attachment 63467 [details]

Eric Seidel (no email)

Comment 6 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.

WebKit Commit Bot

Comment 7 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>

WebKit Commit Bot

Comment 8 2010-08-05 12:34:44 PDT

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.