Bug 170008 - [Readable Streams API] Implement cloneArrayBuffer in WebCore
Summary: [Readable Streams API] Implement cloneArrayBuffer in WebCore
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Romain Bellessort
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-23 07:52 PDT by Romain Bellessort
Modified: 2017-04-13 09:40 PDT (History)
6 users (show)

See Also:


Attachments
Patch (6.83 KB, patch)
2017-03-23 08:09 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff
Patch (6.90 KB, patch)
2017-03-23 09:46 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff
Patch (24.72 KB, patch)
2017-03-27 07:04 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff
memory corruption (12.40 KB, text/plain)
2017-04-01 00:43 PDT, Alexey Proskuryakov
no flags Details
Patch (25.14 KB, patch)
2017-04-10 09:50 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff
Patch (24.15 KB, patch)
2017-04-11 01:29 PDT, Romain Bellessort
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Romain Bellessort 2017-03-23 07:52:04 PDT
Readable Streams API requires implementing cloneArrayBuffer operation. structuredCloneArrayBuffer, which is already implemented in WebCore, already provides support for cloning a full ArrayBuffer, while cloneArrayBuffer allows cloning a part of a given buffer.
Comment 1 Romain Bellessort 2017-03-23 08:09:29 PDT
Created attachment 305195 [details]
Patch
Comment 2 Romain Bellessort 2017-03-23 09:46:35 PDT
Created attachment 305201 [details]
Patch
Comment 3 youenn fablet 2017-03-24 08:39:53 PDT
Comment on attachment 305201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305201&action=review

> Source/WebCore/ChangeLog:15
> +        once ReadableStreamBYOBReader is implemented.

You might be able to test it by adding an Internals routine that allows calling cloneArrayBuffer.
This has been done for isReadableStreamDisturbed for instance.

> Source/WebCore/bindings/js/StructuredClone.cpp:40
> +EncodedJSValue JSC_HOST_CALL cloneArrayBufferImpl(ExecState* state, bool isPartialClone)

In general, we are trying to avoid bool parameters like this. It is not always easy to understand the meaning of it.
Here, it is probably ok since all functions are grouped together.
Comment 4 Romain Bellessort 2017-03-27 07:04:52 PDT
Created attachment 305465 [details]
Patch
Comment 5 Romain Bellessort 2017-03-27 08:28:21 PDT
(In reply to youenn fablet from comment #3)
> Comment on attachment 305201 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305201&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        once ReadableStreamBYOBReader is implemented.
> 
> You might be able to test it by adding an Internals routine that allows
> calling cloneArrayBuffer.
> This has been done for isReadableStreamDisturbed for instance.

Thanks, I was not aware this was feasible. The new patch implements such testing.
Comment 6 youenn fablet 2017-03-29 10:08:43 PDT
Comment on attachment 305465 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=305465&action=review

> Source/WebCore/bindings/js/StructuredClone.cpp:64
> +    return cloneArrayBufferImpl(state, true);

It is not always clear/readable what boolean parameters mean. Here I guess this is ok as is, although you could also have used a class enum .

> LayoutTests/streams/readable-stream-byob-request.js:246
> +    const CloneArrayBuffer = internals.cloneArrayBuffer.bind(internals);

Why not testing it in Worker since @cloneArrayBuffer is available in worker global object as well.
Comment 7 Romain Bellessort 2017-03-30 08:16:07 PDT
(In reply to youenn fablet from comment #6)
> Comment on attachment 305465 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=305465&action=review
> 
> > Source/WebCore/bindings/js/StructuredClone.cpp:64
> > +    return cloneArrayBufferImpl(state, true);
> 
> It is not always clear/readable what boolean parameters mean. Here I guess
> this is ok as is, although you could also have used a class enum .
> 
> > LayoutTests/streams/readable-stream-byob-request.js:246
> > +    const CloneArrayBuffer = internals.cloneArrayBuffer.bind(internals);
> 
> Why not testing it in Worker since @cloneArrayBuffer is available in worker
> global object as well.

I disabled the test when running a worker as the reference to "internals" throws a ReferenceError (Can't find variable: internals) in this case. Have I missed something here that enables avoiding this issue?
Comment 8 youenn fablet 2017-03-30 09:51:32 PDT
> I disabled the test when running a worker as the reference to "internals"
> throws a ReferenceError (Can't find variable: internals) in this case. Have
> I missed something here that enables avoiding this issue?

Ah right, I guess this is good enough although it would be good to add infra support for worker internals.
Comment 9 WebKit Commit Bot 2017-03-31 03:37:09 PDT
Comment on attachment 305465 [details]
Patch

Clearing flags on attachment: 305465

Committed r214663: <http://trac.webkit.org/changeset/214663>
Comment 10 WebKit Commit Bot 2017-03-31 03:37:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Alexey Proskuryakov 2017-04-01 00:43:12 PDT
Created attachment 306043 [details]
memory corruption

This introduced heap-use-after-free memory corruption, see attached ASan report.
Comment 12 Alexey Proskuryakov 2017-04-01 00:47:54 PDT
Rolled back in r214713.
Comment 13 Romain Bellessort 2017-04-10 09:50:15 PDT
Created attachment 306695 [details]
Patch
Comment 14 Romain Bellessort 2017-04-10 10:29:08 PDT
(In reply to Alexey Proskuryakov from comment #11)
> Created attachment 306043 [details]
> memory corruption
> 
> This introduced heap-use-after-free memory corruption, see attached ASan
> report.

I've fixed the memory corruption by making two different calls depending on whether a partial or a full clone is requested (previously I had factorized the code, but this was the reason for the memory issue). Also, as suggested in a previous comment, I replaced the boolean parameter by an enum.
Comment 15 youenn fablet 2017-04-10 11:08:30 PDT
(In reply to Romain Bellessort from comment #14)
> (In reply to Alexey Proskuryakov from comment #11)
> > Created attachment 306043 [details]
> > memory corruption
> > 
> > This introduced heap-use-after-free memory corruption, see attached ASan
> > report.
> 
> I've fixed the memory corruption by making two different calls depending on
> whether a partial or a full clone is requested (previously I had factorized
> the code, but this was the reason for the memory issue). Also, as suggested
> in a previous comment, I replaced the boolean parameter by an enum.

LGTM.
Maybe you should split the added test in its own file.
This test will remain webkit specific while the other ones might move/be superseeded to web-platform-tests
Comment 16 Romain Bellessort 2017-04-11 01:29:34 PDT
Created attachment 306786 [details]
Patch
Comment 17 Romain Bellessort 2017-04-11 02:58:35 PDT
(In reply to youenn fablet from comment #15)
> (In reply to Romain Bellessort from comment #14)
> > (In reply to Alexey Proskuryakov from comment #11)
> > > Created attachment 306043 [details]
> > > memory corruption
> > > 
> > > This introduced heap-use-after-free memory corruption, see attached ASan
> > > report.
> > 
> > I've fixed the memory corruption by making two different calls depending on
> > whether a partial or a full clone is requested (previously I had factorized
> > the code, but this was the reason for the memory issue). Also, as suggested
> > in a previous comment, I replaced the boolean parameter by an enum.
> 
> LGTM.
> Maybe you should split the added test in its own file.
> This test will remain webkit specific while the other ones might move/be
> superseeded to web-platform-tests

I just did that in the new patch, thanks.
Comment 18 WebKit Commit Bot 2017-04-13 09:40:28 PDT
Comment on attachment 306786 [details]
Patch

Clearing flags on attachment: 306786

Committed r215322: <http://trac.webkit.org/changeset/215322>
Comment 19 WebKit Commit Bot 2017-04-13 09:40:30 PDT
All reviewed patches have been landed.  Closing bug.