The script below crashes due to code IDBKey::createBinary(JSC::JSArrayBufferView&) not handling the case where we can't get a buffer view due to running out of memory. -- test.html -- <script> const a = []; a.length = 2**30 a.__proto__ = {}; Object.defineProperty(a, 0, { get: foo }); function foo() { new Uint8Array(a); } try { foo(); } catch {} try { while(1) { new ArrayBuffer(2**20); } } catch {} let u8 = new Uint8Array(); IDBKeyRange.only(u8); </script> Steps To Reproduce: DYLD_FRAMEWORK_PATH=<WEBKIT_PATH>/WebKitBuild/Debug <WEBKIT_PATH>/WebKitBuild/Debug/WebKitTestRunner test.html Results: Crashed Thread: 0 Dispatch queue: com.apple.main-thread Exception Type: EXC_BAD_ACCESS (SIGSEGV) Exception Codes: KERN_INVALID_ADDRESS at 0x0000000000000020 Exception Note: EXC_CORPSE_NOTIFY Thread 0 Crashed:: Dispatch queue: com.apple.main-thread 0 com.apple.WebCore 0x00000004a14631fa WebCore::IDBKey::createBinary(JSC::JSArrayBufferView&) + 42 1 com.apple.WebCore 0x00000004a19abe62 WebCore::createIDBKeyFromValue(JSC::JSGlobalObject&, JSC::JSValue, WTF::Vector<JSC::JSArray*, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>&) + 2178 2 com.apple.WebCore 0x00000004a197ee62 WebCore::scriptValueToIDBKey(JSC::JSGlobalObject&, JSC::JSValue) + 34 3 com.apple.WebCore 0x00000004a147098a WebCore::IDBKeyRange::only(JSC::JSGlobalObject&, JSC::JSValue) + 122 4 com.apple.WebCore 0x00000004a097090f WebCore::jsIDBKeyRangeConstructorFunctionOnly(JSC::JSGlobalObject*, JSC::CallFrame*) + 287 5 ??? 0x0000251383201178 0 + 40765734523256 6 com.apple.JavaScriptCore 0x00000004a9a451b1 llint_entry + 120103
<rdar://problem/68498785>
Created attachment 408869 [details] Patch
Comment on attachment 408869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408869&action=review > LayoutTests/storage/indexeddb/IDBKey-create-array-buffer-view-oom.html:44 > +shouldBeTrue("exceptionString == undefined || exceptionString === 'DataError: Provided data is inadequate.'"); If this happened, shouldn't we throw out-of-memory error instead of DataError?
(In reply to Yusuke Suzuki from comment #3) > Comment on attachment 408869 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408869&action=review > > > LayoutTests/storage/indexeddb/IDBKey-create-array-buffer-view-oom.html:44 > > +shouldBeTrue("exceptionString == undefined || exceptionString === 'DataError: Provided data is inadequate.'"); > > If this happened, shouldn't we throw out-of-memory error instead of > DataError? That error is existing when the key range source is invalid. This fix uses that existing error path. The test code catches and throws away two out-of-memory errors before getting to this point. We could specifically throw out-of-memory, but I suspect that would be a riskier fix.
Comment on attachment 408869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408869&action=review > Source/WebCore/Modules/indexeddb/IDBKey.cpp:56 > + return adoptRef(*new IDBKey()); Can we try returning nullptr here as a RefPtr<IDBKey>? If we are low on memory, it seems best to not allocate a new object. Might be a bigger change though.
Comment on attachment 408869 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=408869&action=review >> Source/WebCore/Modules/indexeddb/IDBKey.cpp:56 >> + return adoptRef(*new IDBKey()); > > Can we try returning nullptr here as a RefPtr<IDBKey>? > If we are low on memory, it seems best to not allocate a new object. > Might be a bigger change though. I agree this seems cleaner, given this now can fail. It's nicer to document things using nullability.
(In reply to youenn fablet from comment #5) > Comment on attachment 408869 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=408869&action=review > > > Source/WebCore/Modules/indexeddb/IDBKey.cpp:56 > > + return adoptRef(*new IDBKey()); > > Can we try returning nullptr here as a RefPtr<IDBKey>? > If we are low on memory, it seems best to not allocate a new object. > Might be a bigger change though. Are you suggesting we change Ref<IDBKey> createBinary(JSC::JSArrayBufferView&) to RefPtr<IDBKey> createBinary(JSC::JSArrayBufferView&) If so, it seems weird to have two different return types for the overloaded createBinary(...) methods. If not, there isn't a way to convert a RefPtr<IDBKey>(nullptr) into a Ref<IDBKey>. This code is only being called by createIDBKeyFromValue() (via createIDBKeyFromValue()) which return IDBKey::createInvalid() aka adoptRef(*new IDBKey()) for a key with the value of RefPtr<IDBKey>(nullptr). So we aren't avoiding the allocation, only delaying it.
Comment on attachment 408869 [details] Patch > Are you suggesting we change > Ref<IDBKey> createBinary(JSC::JSArrayBufferView&) > to > RefPtr<IDBKey> createBinary(JSC::JSArrayBufferView&) > > If so, it seems weird to have two different return types for the overloaded createBinary(...) methods. We could introduce tryCreateBinary. Or at least return IDBKey::createInvalid() which seems a bit clearer.
(In reply to youenn fablet from comment #8) > Comment on attachment 408869 [details] > Patch > > > Are you suggesting we change > > Ref<IDBKey> createBinary(JSC::JSArrayBufferView&) > > to > > RefPtr<IDBKey> createBinary(JSC::JSArrayBufferView&) > > > > If so, it seems weird to have two different return types for the overloaded createBinary(...) methods. > > We could introduce tryCreateBinary. > Or at least return IDBKey::createInvalid() which seems a bit clearer. Let's go with IDBKey::createInvalid().
Created attachment 409043 [details] Updated Patch
Committed r267205: <https://trac.webkit.org/changeset/267205>
It looks like the new test storage/indexeddb/IDBKey-create-array-buffer-view-oom.html added in https://trac.webkit.org/changeset/267205/webkit is a flaky timeout History: https://results.webkit.org/?suite=layout-tests&test=storage%2Findexeddb%2FIDBKey-create-array-buffer-view-oom.html
(In reply to Truitt Savell from comment #12) > It looks like the new test storage/indexeddb/IDBKey-create-array-buffer-view-oom.html added in https://trac.webkit.org/changeset/267205/webkit > is a flaky timeout This test has been very flaky and slowing down EWS. e.g.: https://ews-build.webkit.org/#/builders/24/builds/27172 https://ews-build.webkit.org/#/builders/10/builds/37806 https://ews-build.webkit.org/#/builders/10/builds/37803 https://ews-build.webkit.org/#/builders/24/builds/27145 https://ews-build.webkit.org/#/builders/24/builds/27122 https://ews-build.webkit.org/#/builders/10/builds/37769 https://ews-build.webkit.org/#/builders/24/builds/27100 https://ews-build.webkit.org/#/builders/24/builds/27086 https://ews-build.webkit.org/#/builders/10/builds/37699 https://ews-build.webkit.org/#/builders/24/builds/27069 https://ews-build.webkit.org/#/builders/24/builds/27053 https://ews-build.webkit.org/#/builders/10/builds/37671 https://ews-build.webkit.org/#/builders/24/builds/27018 https://ews-build.webkit.org/#/builders/10/builds/37644