Summary: | Segfault in WebCore::IDBKey::createBinary | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Michael Saboff <msaboff> | ||||||
Component: | WebCore JavaScript | Assignee: | Michael Saboff <msaboff> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | aakash_jain, alecflett, beidson, ews-watchlist, jsbell, saam, tsavell, webkit-bug-importer, youennf, ysuzuki | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | WebKit Nightly Build | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=216960 | ||||||||
Attachments: |
|
Description
Michael Saboff
2020-09-14 16:19:10 PDT
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 |