MessagePortArray cloning code needs to verify source before copying

• Component: WebCore JavaScript
• Status: RESOLVED FIXED

[Andrew Wilson]

The V8 and JSC implementation of MessagePortArray cloning code needs to verify the source array before allocating a copy.

For example, the following code will cause an OOM when the bindings code naively tries to allocate a 1234567890 element array:

var channel4 = new MessageChannel();
var channel = new MessageChannel();


var largePortArray = [];
largePortArray[1234567890] = channel4.port1;
channel.port1.postMessage("largeSequence", largePortArray);

The correct behavior is to throw an exception because there are undefined elements in the array.

[Andrew Wilson]

Created attachment 94086 [details]
Patch to change the MessagePortArray copying code to not pre-allocate the destination.

Note that this new code (not pre-allocating the destination) is just as fast as the old code for any arrays containing up to 16 MessagePorts (which covers all expected use cases in practice).

[WebKit Review Bot]

Attachment 94086 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1

LayoutTests/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:10:  Line contains tab character.  [whitespace/tab] [5]
Source/WebCore/ChangeLog:13:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 3 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Darin Adler]

Comment on attachment 94086 [details]
Patch to change the MessagePortArray copying code to not pre-allocate the destination.

Change looks fine, but tabs in change log will cause the commit queue to fail.

[Andrew Wilson]

Created attachment 94097 [details]
Patch without tabs in ChangeLog

[Andrew Wilson]

Comment on attachment 94097 [details]
Patch without tabs in ChangeLog

I removed those tabs, but the patch is otherwise identical. Anyone want to re-r+ this so the commit-queue can take it?

[Darin Adler]

Comment on attachment 94097 [details]
Patch without tabs in ChangeLog

Next time I think you should set commit-queue to ? rather than to +. Unless I am ignorant about how commit-queue+ works.

[Andrew Wilson]

You're probably right - it's been a while since I've landed anything but test_expectations changes so I'm a bit rusty on the process :( I should probably also wait on the EWS bots before R? it too.

Thanks for the quick turnaround on the reviews!

[WebKit Commit Bot]

Comment on attachment 94097 [details]
Patch without tabs in ChangeLog

Clearing flags on attachment: 94097

Committed r86899: <http://trac.webkit.org/changeset/86899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ademar Reis]

Revision r86899 cherry-picked into qtwebkit-2.2 with commit 63adcb6 <http://gitorious.org/webkit/qtwebkit/commit/63adcb6>