Bug 154849 - Remove DeferredWrapper::resolve<Vector<unsigned char>>
Summary: Remove DeferredWrapper::resolve<Vector<unsigned char>>
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: youenn fablet
URL:
Keywords:
Depends on: 150497
Blocks:
  Show dependency treegraph
 
Reported: 2016-03-01 01:20 PST by youenn fablet
Modified: 2016-03-24 03:36 PDT (History)
2 users (show)

See Also:


Attachments
Patch (11.73 KB, patch)
2016-03-01 05:24 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (8.76 KB, patch)
2016-03-22 08:53 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (8.85 KB, patch)
2016-03-24 00:37 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2016-03-01 01:20:34 PST
Following on bug 154820, it may be better to pass an ArrayBuffer in lieu of a Vector<char>, Vector<uint8_t> to promises.
Comment 1 youenn fablet 2016-03-01 05:24:16 PST
Created attachment 272560 [details]
Patch
Comment 2 Darin Adler 2016-03-01 09:22:43 PST
Comment on attachment 272560 [details]
Patch

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

A step in the right direction, overlaps another patch in process, and has some problems handling memory exhaustion.

> Source/JavaScriptCore/runtime/ArrayBuffer.h:156
> -PassRefPtr<ArrayBuffer> ArrayBuffer::create(const void* source, unsigned byteLength)
> +RefPtr<ArrayBuffer> ArrayBuffer::create(const void* source, unsigned byteLength)

This overlaps a patch that someone is currently working on to fix all the ArrayBuffer constructors to not use PassRefPtr any more. As I noticed when reviewing that patch, there’s something particularly bad about a function named create that can actually return nullptr given our usual idiom. I think this function should be renamed tryCreate.

If we want to use a function for normal cases where we need the buffer to be allocated and it’s not optional to succeed, then we should make one that crashes if it can’t allocate, like most memory allocations in WebKit. And that function can be named create, and it should return a Ref<ArrayBuffer>, not a RefPtr<ArrayBuffer>.

> Source/WebCore/Modules/fetch/FetchBody.cpp:108
> -        RefPtr<ArrayBuffer> buffer = ArrayBuffer::create(data.data(), data.size());
> -        promise.resolve(buffer);
> +        promise.resolve(ArrayBuffer::create(data.data(), data.size()));

This calls the “tryCreate” function, but does not check for null.

> Source/WebCore/Modules/mediastream/RTCDataChannel.cpp:284
> +        scheduleDispatchEvent(MessageEvent::create(ArrayBuffer::create(data, dataLength)));

This calls the “tryCreate” function, but does not check for null.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:170
> +        wrapper.resolve(ArrayBuffer::create(result.data(), result.size()));

This calls the “tryCreate” function, but does not check for null.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:222
> +        wrapper.resolve(ArrayBuffer::create(result.data(), result.size()));

This calls the “tryCreate” function, but does not check for null.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:374
> +        wrapper.resolve(ArrayBuffer::create(result.data(), result.size()));

This calls the “tryCreate” function, but does not check for null.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:616
> +        wrapper.resolve(ArrayBuffer::create(result.data(), result.size()));

This calls the “tryCreate” function, but does not check for null.

> Source/WebCore/bindings/js/JSSubtleCryptoCustom.cpp:676
> +            wrapper.resolve(ArrayBuffer::create(encryptedData.data(), encryptedData.size()));

This calls the “tryCreate” function, but does not check for null.
Comment 3 Darin Adler 2016-03-01 09:23:01 PST
I like what you are doing here.
Comment 4 youenn fablet 2016-03-01 09:38:34 PST
(In reply to comment #2)
> Comment on attachment 272560 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=272560&action=review
> 
> A step in the right direction, overlaps another patch in process, and has
> some problems handling memory exhaustion.
> 
> > Source/JavaScriptCore/runtime/ArrayBuffer.h:156
> > -PassRefPtr<ArrayBuffer> ArrayBuffer::create(const void* source, unsigned byteLength)
> > +RefPtr<ArrayBuffer> ArrayBuffer::create(const void* source, unsigned byteLength)
> 
> This overlaps a patch that someone is currently working on to fix all the
> ArrayBuffer constructors to not use PassRefPtr any more. As I noticed when
> reviewing that patch, there’s something particularly bad about a function
> named create that can actually return nullptr given our usual idiom. I think
> this function should be renamed tryCreate.
> 
> If we want to use a function for normal cases where we need the buffer to be
> allocated and it’s not optional to succeed, then we should make one that
> crashes if it can’t allocate, like most memory allocations in WebKit. And
> that function can be named create, and it should return a Ref<ArrayBuffer>,
> not a RefPtr<ArrayBuffer>.
> 
> > Source/WebCore/Modules/fetch/FetchBody.cpp:108
> > -        RefPtr<ArrayBuffer> buffer = ArrayBuffer::create(data.data(), data.size());
> > -        promise.resolve(buffer);
> > +        promise.resolve(ArrayBuffer::create(data.data(), data.size()));
> 
> This calls the “tryCreate” function, but does not check for null.

The null check is done in the toJS(...ArrayBuffer*) function and will translate to a JS null value. This patch does not bring anything new here actually.

That said, I agree "create" should actually crash when allocation fails.
I will wait for the other patch to land.
Comment 5 youenn fablet 2016-03-07 23:54:20 PST
> That said, I agree "create" should actually crash when allocation fails.
> I will wait for the other patch to land.

There is some changes in the fetch spec, particularly step 5 of https://fetch.spec.whatwg.org/#body-mixin.
In some cases, crashing is fine, in some others, throwing an exception might be better.
Having "tryCreate" in addition to "create" is therefore potentially useful.
Comment 6 Darin Adler 2016-03-08 08:49:28 PST
(In reply to comment #5)
> > That said, I agree "create" should actually crash when allocation fails.
> > I will wait for the other patch to land.
> 
> There is some changes in the fetch spec, particularly step 5 of
> https://fetch.spec.whatwg.org/#body-mixin.
> In some cases, crashing is fine, in some others, throwing an exception might
> be better.
> Having "tryCreate" in addition to "create" is therefore potentially useful.

I figured that would be the case. That’s why we have the try and non-try variants of various functions like fastMalloc itself. For allocations that will always succeed we use the non-try version so we get crashes instead of other types of security vulnerabilities when we get it wrong. For allocations that can fail, the try version helps make it clear that we need to check for and handle failures.
Comment 7 Darin Adler 2016-03-08 08:51:29 PST
I think the urgent first step is to rename the functions that may fail to tryCreate. Then at our leisure we can do things like: 1) add non-try variants, 2) get rid of the try variant entirely in cases where it’s not needed, 3) fix call sites to use the appropriate version.

I think it’s dangerous to have functions with the “try” semantic for memory allocation that do not express that in their names.
Comment 8 youenn fablet 2016-03-22 08:53:30 PDT
Created attachment 274655 [details]
Patch
Comment 9 Darin Adler 2016-03-22 09:29:56 PDT
Comment on attachment 274655 [details]
Patch

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

> Source/WebCore/bindings/js/JSDOMPromise.cpp:100
> +    if (!arrayBuffer)
> +        promise.reject<ExceptionCode>(TypeError);
> +    promise.resolve(arrayBuffer);

I think this is missing a return statement. Is it really OK to both reject and resolve?

> Source/WebCore/bindings/js/JSDOMPromise.h:59
> +void fulfillPromiseWithArrayBuffer(DeferredWrapper&, const RefPtr<ArrayBuffer>&);

Type here is peculiar. It should be one of these types:

    ArrayBuffer*
    RefPtr<ArrayBuffer>&&

We almost never want to pass const RefPtr<ArrayBuffer>&.
Comment 10 youenn fablet 2016-03-24 00:37:31 PDT
Created attachment 274825 [details]
Patch for landing
Comment 11 youenn fablet 2016-03-24 00:39:37 PDT
Thanks for the review.

(In reply to comment #9)
> Comment on attachment 274655 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274655&action=review
> 
> > Source/WebCore/bindings/js/JSDOMPromise.cpp:100
> > +    if (!arrayBuffer)
> > +        promise.reject<ExceptionCode>(TypeError);
> > +    promise.resolve(arrayBuffer);
> 
> I think this is missing a return statement. Is it really OK to both reject
> and resolve?

Yes, I fixed that.
Also, I replaced the rejected value to an OutOfMemory error which is more appropriate.

> 
> > Source/WebCore/bindings/js/JSDOMPromise.h:59
> > +void fulfillPromiseWithArrayBuffer(DeferredWrapper&, const RefPtr<ArrayBuffer>&);
> 
> Type here is peculiar. It should be one of these types:
> 
>     ArrayBuffer*
>     RefPtr<ArrayBuffer>&&
> 
> We almost never want to pass const RefPtr<ArrayBuffer>&.

Switched to ArrayBuffer* then.
Comment 12 WebKit Commit Bot 2016-03-24 03:36:47 PDT
Comment on attachment 274825 [details]
Patch for landing

Clearing flags on attachment: 274825

Committed r198622: <http://trac.webkit.org/changeset/198622>
Comment 13 WebKit Commit Bot 2016-03-24 03:36:52 PDT
All reviewed patches have been landed.  Closing bug.