[Readable Streams API] Implement cloneArrayBuffer in WebCore

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Romain Bellessort]

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.

[Romain Bellessort]

Created attachment 305195 [details]
Patch

[Romain Bellessort]

Created attachment 305201 [details]
Patch

[youenn fablet]

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.

[Romain Bellessort]

Created attachment 305465 [details]
Patch

[Romain Bellessort]

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

[youenn fablet]

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.

[Romain Bellessort]

(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?

[youenn fablet]

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

[WebKit Commit Bot]

Comment on attachment 305465 [details]
Patch

Clearing flags on attachment: 305465

Committed r214663: <http://trac.webkit.org/changeset/214663>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Created attachment 306043 [details]
memory corruption

This introduced heap-use-after-free memory corruption, see attached ASan report.

[Alexey Proskuryakov]

Rolled back in r214713.

[Romain Bellessort]

Created attachment 306695 [details]
Patch

[Romain Bellessort]

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

[youenn fablet]

(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

[Romain Bellessort]

Created attachment 306786 [details]
Patch

[Romain Bellessort]

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

[WebKit Commit Bot]

Comment on attachment 306786 [details]
Patch

Clearing flags on attachment: 306786

Committed r215322: <http://trac.webkit.org/changeset/215322>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.