Bug 155328 – [JSC] Add ArrayBuffer::tryCreate and change the callsites where it is needed

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Joonghun Park]

Add ArrayBuffer::tryCreate in which it can return null, and it ensures that ArrayBuffer::create doesn't return null.
And change the callsites to use ArrayBuffer::tryCreate where it cares about null.

[Joonghun Park]

Created attachment 274260 [details]
<WIP> Change callsites going to be proceeded

[Joonghun Park]

Created attachment 274352 [details]
Patch

[Joonghun Park]

Created attachment 274375 [details]
Fix build break of Win port

[Darin Adler]

Comment on attachment 274375 [details]
Fix build break of Win port

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

Not sure why the DataCue changes are in here. They look OK, but not sure you intended to mix them in with ArrayBuffer.

Looks generally good but a lot of suggestions for improvement. In some of the tryCreate call sites, I am unclear where the code to handle null is and whether the error policy is what we want.

> Source/JavaScriptCore/runtime/ArrayBuffer.h:163
>      return create(numElements, elementByteSize, ArrayBufferContents::ZeroInitialize);

I suggest this instead:

    auto buffer  = tryCreate(numElements, elementByteSize);
    if (!buffer)
        CRASH();
    return buffer.releaseNonNull();

> Source/JavaScriptCore/runtime/ArrayBuffer.h:178
> -RefPtr<ArrayBuffer> ArrayBuffer::create(const void* source, unsigned byteLength)
> +Ref<ArrayBuffer> ArrayBuffer::create(const void* source, unsigned byteLength)
>  {
>      ArrayBufferContents contents;
>      ArrayBufferContents::tryAllocate(byteLength, 1, ArrayBufferContents::ZeroInitialize, contents);
>      if (!contents.m_data)
> -        return nullptr;
> -    auto buffer = adoptRef(*new ArrayBuffer(contents));
> -    ASSERT(!byteLength || source);
> -    memcpy(buffer->data(), source, byteLength);
> -    return WTFMove(buffer);
> +        CRASH();
> +    return ArrayBuffer::createInternal(contents, source, byteLength);
>  }

I suggest this instead:

    auto buffer = tryCreate(source, byteLength);
    if (!buffer)
        CRASH();
    return buffer.releaseNonNull();

> Source/JavaScriptCore/runtime/ArrayBuffer.h:212
> +    return ArrayBuffer::createInternal(contents, source, byteLength);

Just "createInternal", not "ArrayBuffer::createInternal".

> Source/JavaScriptCore/runtime/ArrayBuffer.h:232
> +Ref<ArrayBuffer> ArrayBuffer::create(unsigned numElements, unsigned elementByteSize, ArrayBufferContents::InitializationPolicy policy)
> +{
> +    ArrayBufferContents contents;
> +    ArrayBufferContents::tryAllocate(numElements, elementByteSize, policy, contents);
> +    if (!contents.m_data)
> +        CRASH();
> +    return adoptRef(*new ArrayBuffer(contents));
> +}

I suggest not adding this function, or making it a cover that calls through to tryCreate as above.

> Source/JavaScriptCore/runtime/ArrayBuffer.h:237
> +    ASSERT(!byteLength || source);

I’d put the assertion on the first line of the function. Why not validate the arguments *before* doing anything else? Cleaner.

> Source/WebCore/Modules/fetch/FetchBody.cpp:100
> +            promise.resolve(ArrayBuffer::tryCreate(nullptr, 0));

Is passing nullptr to resolve() the behavior we want when out of memory?

> Source/WebCore/Modules/fetch/FetchBody.cpp:177
> +        promise.resolve<RefPtr<ArrayBuffer>>(ArrayBuffer::tryCreate(data.data(), data.size()));

Same question again.

> Source/WebCore/Modules/fetch/FetchLoader.cpp:103
> +        m_client.didFinishLoadingAsArrayBuffer(m_data ? m_data->createArrayBuffer() : ArrayBuffer::tryCreate(nullptr, 0));

Seems clear this should be create, not tryCreate. Is it really OK to pass a nullptr for the array buffer?

> Source/WebCore/bindings/js/JSDOMPromise.h:176
> +    auto buffer = ArrayBuffer::tryCreate(result.data(), result.size());

Is passing nullptr to resolve() the behavior we want when out of memory?

> Source/WebCore/fileapi/FileReaderLoader.cpp:280
> +    return ArrayBuffer::create(*m_rawData.get());

No need for ".get()" here, just *m_rawData should do.

> Source/WebCore/html/track/DataCue.cpp:88
> +    return ArrayBuffer::create(*m_data.get());

No need for .get() here, just *m_data should do.

> Source/WebCore/html/track/DataCue.h:82
> +    const RefPtr<SerializedPlatformRepresentation> platformValue() const { return m_platformValue; }

This is the wrong return type. It should be SerializedPlatformRepresentation* because we are not transferring ownership.

> Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:147
> +    auto dataArray = ArrayBuffer::tryCreate([data bytes], [data length]);

I think this should be create, not tryCreate.

> Source/WebCore/xml/XMLHttpRequest.cpp:264
> +            m_responseArrayBuffer = ArrayBuffer::tryCreate(nullptr, 0);

This should be create, not tryCreate.

[youenn fablet]

> > Source/WebCore/Modules/fetch/FetchBody.cpp:100
> > +            promise.resolve(ArrayBuffer::tryCreate(nullptr, 0));
> 
> Is passing nullptr to resolve() the behavior we want when out of memory?

I think we  want to reject the promise, but this would be a change of behavior.
The current code is keeping the status quo and the behavior should be changed in bug 154849.
I am not sure how we can actually test that though.

> > Source/WebCore/Modules/fetch/FetchBody.cpp:177
> > +        promise.resolve<RefPtr<ArrayBuffer>>(ArrayBuffer::tryCreate(data.data(), data.size()));
> 
> Same question again.

Ditto.

> > Source/WebCore/Modules/fetch/FetchLoader.cpp:103
> > +        m_client.didFinishLoadingAsArrayBuffer(m_data ? m_data->createArrayBuffer() : ArrayBuffer::tryCreate(nullptr, 0));
> 
> Seems clear this should be create, not tryCreate. Is it really OK to pass a
> nullptr for the array buffer?
> 
> > Source/WebCore/bindings/js/JSDOMPromise.h:176
> > +    auto buffer = ArrayBuffer::tryCreate(result.data(), result.size());

I would tend to keep calling tryCreate and making the loader fail if buffer is null.
That way, fetch would just reject the promise if any.
This can probably be handled here or in bug 154849.

[Joonghun Park]

Created attachment 274421 [details]
Patch

[Joonghun Park]

Comment on attachment 274375 [details]
Fix build break of Win port

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

Darin, Could you take a look at this change again? I think it still needs to be went over, though this patch got r+ previously.

>> Source/JavaScriptCore/runtime/ArrayBuffer.h:163
>>      return create(numElements, elementByteSize, ArrayBufferContents::ZeroInitialize);
> 
> I suggest this instead:
> 
>     auto buffer  = tryCreate(numElements, elementByteSize);
>     if (!buffer)
>         CRASH();
>     return buffer.releaseNonNull();

Ok, I changed this one as commented.

>> Source/JavaScriptCore/runtime/ArrayBuffer.h:178
>>  }
> 
> I suggest this instead:
> 
>     auto buffer = tryCreate(source, byteLength);
>     if (!buffer)
>         CRASH();
>     return buffer.releaseNonNull();

Ok, done.

>> Source/JavaScriptCore/runtime/ArrayBuffer.h:212
>> +    return ArrayBuffer::createInternal(contents, source, byteLength);
> 
> Just "createInternal", not "ArrayBuffer::createInternal".

Ok, I removed this redundant namespace.

>> Source/JavaScriptCore/runtime/ArrayBuffer.h:232
>> +}
> 
> I suggest not adding this function, or making it a cover that calls through to tryCreate as above.

I made this to call through to tryCreate as you commented.

>> Source/JavaScriptCore/runtime/ArrayBuffer.h:237
>> +    ASSERT(!byteLength || source);
> 
> I’d put the assertion on the first line of the function. Why not validate the arguments *before* doing anything else? Cleaner.

I moved this ASSERT statement to the first line of the function as you said.

>> Source/WebCore/Modules/fetch/FetchBody.cpp:100
>> +            promise.resolve(ArrayBuffer::tryCreate(nullptr, 0));
> 
> Is passing nullptr to resolve() the behavior we want when out of memory?

I thought that "if (!buffer) return JSC::jsNull();" statement in "inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, JSC::ArrayBuffer* buffer)" function handles null case,
so by calling ArrayBuffer::tryCreate it keeps the status quo. 

But if it's not right, then maybe I can change this part as to "promise.resolve(ArrayBuffer::create(nullptr, 0).ptr());".
In addition, it seems that Youenn has a plan to change the behaviour to reject the promise in https://bugs.webkit.org/show_bug.cgi?id=154849, after this patch.

>> Source/WebCore/Modules/fetch/FetchBody.cpp:177
>> +        promise.resolve<RefPtr<ArrayBuffer>>(ArrayBuffer::tryCreate(data.data(), data.size()));
> 
> Same question again.

ditto.

>> Source/WebCore/Modules/fetch/FetchLoader.cpp:103
>> +        m_client.didFinishLoadingAsArrayBuffer(m_data ? m_data->createArrayBuffer() : ArrayBuffer::tryCreate(nullptr, 0));
> 
> Seems clear this should be create, not tryCreate. Is it really OK to pass a nullptr for the array buffer?

ditto.

>> Source/WebCore/bindings/js/JSDOMPromise.h:176
>> +    auto buffer = ArrayBuffer::tryCreate(result.data(), result.size());
> 
> Is passing nullptr to resolve() the behavior we want when out of memory?

ditto.

>> Source/WebCore/fileapi/FileReaderLoader.cpp:280
>> +    return ArrayBuffer::create(*m_rawData.get());
> 
> No need for ".get()" here, just *m_rawData should do.

Done.

>> Source/WebCore/html/track/DataCue.cpp:88
>> +    return ArrayBuffer::create(*m_data.get());
> 
> No need for .get() here, just *m_data should do.

Done.

>> Source/WebCore/html/track/DataCue.h:82
>> +    const RefPtr<SerializedPlatformRepresentation> platformValue() const { return m_platformValue; }
> 
> This is the wrong return type. It should be SerializedPlatformRepresentation* because we are not transferring ownership.

Ok, I changed this to return raw pointer.

>> Source/WebCore/platform/mac/SerializedPlatformRepresentationMac.mm:147
>> +    auto dataArray = ArrayBuffer::tryCreate([data bytes], [data length]);
> 
> I think this should be create, not tryCreate.

I thought that "if (!buffer) return JSC::jsNull();" statement in "inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, JSC::ArrayBuffer* buffer)" function handles null case,
so by calling ArrayBuffer::tryCreate it keeps the status quo. 

But if it's not right, then maybe I can change this part as to "promise.resolve(ArrayBuffer::create(nullptr, 0).ptr());".
In addition, it seems that Youenn has a plan to change the behaviour to reject the promise in https://bugs.webkit.org/show_bug.cgi?id=154849, after this patch.

>> Source/WebCore/xml/XMLHttpRequest.cpp:264
>> +            m_responseArrayBuffer = ArrayBuffer::tryCreate(nullptr, 0);
> 
> This should be create, not tryCreate.

Done.

[Darin Adler]

Comment on attachment 274375 [details]
Fix build break of Win port

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

>>> Source/WebCore/Modules/fetch/FetchBody.cpp:100
>>> +            promise.resolve(ArrayBuffer::tryCreate(nullptr, 0));
>> 
>> Is passing nullptr to resolve() the behavior we want when out of memory?
> 
> I thought that "if (!buffer) return JSC::jsNull();" statement in "inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, JSC::ArrayBuffer* buffer)" function handles null case,
> so by calling ArrayBuffer::tryCreate it keeps the status quo. 
> 
> But if it's not right, then maybe I can change this part as to "promise.resolve(ArrayBuffer::create(nullptr, 0).ptr());".
> In addition, it seems that Youenn has a plan to change the behaviour to reject the promise in https://bugs.webkit.org/show_bug.cgi?id=154849, after this patch.

I agree with both of your points:

1) The code converts a nullptr into a JavaScript null.
2) Your patch does not change WebKit behavior.

But my question, for someone else I think, is whether this behavior is acceptable. Is it desirable to just send a null to the JavaScript when there is insufficient memory?

[Joonghun Park]

(In reply to comment #8)
> Comment on attachment 274375 [details]
> Fix build break of Win port
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274375&action=review
> 
> >>> Source/WebCore/Modules/fetch/FetchBody.cpp:100
> >>> +            promise.resolve(ArrayBuffer::tryCreate(nullptr, 0));
> >> 
> >> Is passing nullptr to resolve() the behavior we want when out of memory?
> > 
> > I thought that "if (!buffer) return JSC::jsNull();" statement in "inline JSC::JSValue toJS(JSC::ExecState* exec, JSDOMGlobalObject* globalObject, JSC::ArrayBuffer* buffer)" function handles null case,
> > so by calling ArrayBuffer::tryCreate it keeps the status quo. 
> > 
> > But if it's not right, then maybe I can change this part as to "promise.resolve(ArrayBuffer::create(nullptr, 0).ptr());".
> > In addition, it seems that Youenn has a plan to change the behaviour to reject the promise in https://bugs.webkit.org/show_bug.cgi?id=154849, after this patch.
> 
> I agree with both of your points:
> 
> 1) The code converts a nullptr into a JavaScript null.
> 2) Your patch does not change WebKit behavior.
> 
> But my question, for someone else I think, is whether this behavior is
> acceptable. Is it desirable to just send a null to the JavaScript when there
> is insufficient memory?

Maybe it is fine to land this change because it keeps status quo anyway, after that we can handle your question as separate bug, I think. To do that, I will rebase this patch .

[Joonghun Park]

Created attachment 274591 [details]
Patch for landing

[youenn fablet]

> > But my question, for someone else I think, is whether this behavior is
> > acceptable. Is it desirable to just send a null to the JavaScript when there
> > is insufficient memory?
> 
> Maybe it is fine to land this change because it keeps status quo anyway,
> after that we can handle your question as separate bug, I think. To do that,
> I will rebase this patch .

Sounds good to me, landing this patch will help tackling the question Darin asked.

As of the particular question, we may consider that ArrayBuffer::create returning null is similar to throwing a JS exception.
Following the error handling of the specs, that could be translated to rejecting a promise, erroring a readable stream...

[WebKit Commit Bot]

Comment on attachment 274591 [details]
Patch for landing

Clearing flags on attachment: 274591

Committed r198488: <http://trac.webkit.org/changeset/198488>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.