Bug 154849

Created attachment 272560 [details] Patch

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.

I like what you are doing here.

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

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

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

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.

Created attachment 274655 [details] Patch

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

Created attachment 274825 [details] Patch for landing

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 on attachment 274825 [details] Patch for landing Clearing flags on attachment: 274825 Committed r198622: <http://trac.webkit.org/changeset/198622>

All reviewed patches have been landed. Closing bug.