Summary: | Layout tests imported/w3c/web-platform-tests/IndexedDB/*-exception-order.html are failing | ||
---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> |
Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | achristensen, alecflett, beidson, commit-queue, ews-watchlist, ggaren, jsbell, rniwa, webkit-bug-importer |
Priority: | P2 | Keywords: | InRadar |
Version: | WebKit Nightly Build | ||
Hardware: | Unspecified | ||
OS: | Unspecified | ||
Attachments: |
Description
Sihui Liu
2019-03-12 15:58:13 PDT
Created attachment 364466 [details]
Patch
Comment on attachment 364466 [details] Patch Attachment 364466 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/11482207 Number of test failures exceeded the failure limit. Created attachment 364473 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 364466 [details] Patch Attachment 364466 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/11482403 Number of test failures exceeded the failure limit. Created attachment 364474 [details]
Archive of layout-test-results from ews103 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 364466 [details] Patch Attachment 364466 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/11482204 Number of test failures exceeded the failure limit. Created attachment 364478 [details]
Archive of layout-test-results from ews112 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 364466 [details] Patch Attachment 364466 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/11484023 Number of test failures exceeded the failure limit. Created attachment 364498 [details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Created attachment 364561 [details]
Patch
Comment on attachment 364561 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364561&action=review > Source/WebCore/ChangeLog:8 > + Fix some exception orders in IDB. Please add a URL to the spec. > Source/WebCore/ChangeLog:11 > + (WebCore::IDBDatabase::createObjectStore): Step 6 of https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-createobjectstore > Source/WebCore/ChangeLog:12 > + (WebCore::IDBDatabase::transaction): Step 1 of https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction > Source/WebCore/ChangeLog:15 > + (WebCore::IDBIndex::openCursor): Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-opencursor > Source/WebCore/ChangeLog:16 > + (WebCore::IDBIndex::doOpenKeyCursor): Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-openkeycursor > Source/WebCore/ChangeLog:18 > + (WebCore::IDBIndex::count): Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-count > Source/WebCore/ChangeLog:20 > + (WebCore::IDBIndex::get): Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-get > Source/WebCore/ChangeLog:22 > + (WebCore::IDBIndex::getKey): Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-getkey > Source/WebCore/ChangeLog:24 > + (WebCore::IDBIndex::doGetAll): Step 5: https://www.w3.org/TR/IndexedDB-2/#dom-idbindex-getallkeys > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:175 > + if (m_versionChangeTransaction && !m_versionChangeTransaction->isFinishedOrFinishing()) > + return Exception { InvalidStateError, "Failed to execute 'transaction' on 'IDBDatabase': A version change transaction is running."_s }; > + For consistency with the spec, we should probably check this before m_closePending: https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:183 > if (objectStores.isEmpty()) > return Exception { InvalidAccessError, "Failed to execute 'transaction' on 'IDBDatabase': The storeNames parameter was empty."_s }; This check also probably needs to be after the NotFoundError check below per step 4: https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction > Source/WebCore/Modules/indexeddb/IDBDatabase.cpp:199 > + if (mode != IDBTransactionMode::Readonly && mode != IDBTransactionMode::Readwrite) Step 6 of https://www.w3.org/TR/IndexedDB-2/#dom-idbdatabase-transaction > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:176 > +ExceptionOr<Ref<IDBRequest>> IDBIndex::openCursor(ExecState& execState, IDBKeyRange* range, IDBCursorDirection direction) We should make this function take RefPtr<>&& like other functions. > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:178 > + return doOpenCursor(execState, direction, [range]() { And do range = WTFMove(range). > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:188 > + return ExceptionOr<RefPtr<IDBKeyRange>>(Exception(DataError, "Failed to execute 'openCursor' on 'IDBIndex': The parameter is not a valid key."_s)); Step 5 of https://www.w3.org/TR/IndexedDB-2/#dom-idbobjectstore-opencursor > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:190 > + return ExceptionOr<RefPtr<IDBKeyRange>> { makeRefPtr(onlyResult.releaseReturnValue().ptr()) }; You shouldn't have to manually construct RefPtr out of Ref like this. Just do WTFMove(onlyResult.releaseReturnValue()). Otherwise, we'd do a ref-churn. > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:216 > + return doOpenKeyCursor(execState, direction, [range]() { Ditto about making RefPtr earlier. > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:228 > + return ExceptionOr<RefPtr<IDBKeyRange>> { makeRefPtr(onlyResult.releaseReturnValue().ptr()) }; Ditto about using WTF instead. > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:279 > - return Exception { DataError, "Failed to execute 'get' on 'IDBIndex': The parameter is not a valid key."_s }; > + return doGet(execState, Exception(DataError, "Failed to execute 'get' on 'IDBIndex': The parameter is not a valid key."_s)); Hm... it's a bit funky that we still call doGet just to check two more conditions but okay. > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:377 > + return ExceptionOr<RefPtr<IDBKeyRange>> { makeRefPtr(onlyResult.releaseReturnValue().ptr()) }; Ditto about avoiding ref-churn. > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:400 > +ExceptionOr<Ref<IDBRequest>> IDBIndex::getAllKeys(ExecState& execState, RefPtr<IDBKeyRange> range, Optional<uint32_t> count) Use RefPtr<>&& > Source/WebCore/Modules/indexeddb/IDBIndex.cpp:414 > + return ExceptionOr<RefPtr<IDBKeyRange>> { makeRefPtr(onlyResult.releaseReturnValue().ptr()) }; Ditto about avoiding ref-churn. > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:172 > +ExceptionOr<Ref<IDBRequest>> IDBObjectStore::openCursor(ExecState& execState, RefPtr<IDBKeyRange> range, IDBCursorDirection direction) Use RefPtr&& > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:186 > + return ExceptionOr<RefPtr<IDBKeyRange>> { makeRefPtr(onlyResult.releaseReturnValue().ptr()) }; Fix this one also. > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:210 > +ExceptionOr<Ref<IDBRequest>> IDBObjectStore::openKeyCursor(ExecState& execState, RefPtr<IDBKeyRange> range, IDBCursorDirection direction) Ditto about RefPtr<>&& > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:224 > + return ExceptionOr<RefPtr<IDBKeyRange>> { makeRefPtr(onlyResult.releaseReturnValue().ptr()) }; Fix this one also. > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:431 > + Ref<IDBKey> idbKey = scriptValueToIDBKey(*state, key); Use auto? > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:434 > + return ExceptionOr<RefPtr<IDBKeyRange>> { makeRefPtr(IDBKeyRange::create(WTFMove(idbKey)).ptr()) }; Hm.. you should just be able to just return IDBKeyRange::create(WTFMove(idbKey)) or maybe you need to do WTFMove(~::create(~)) > Source/WebCore/Modules/indexeddb/IDBObjectStore.cpp:638 > + return ExceptionOr<RefPtr<IDBKeyRange>>(Exception(DataError, "Failed to execute 'getAll' on 'IDBObjectStore': The parameter is not a valid key."_s)); Why not use { ~ }? Created attachment 364923 [details]
Patch for landing
Comment on attachment 364923 [details] Patch for landing Clearing flags on attachment: 364923 Committed r243039: <https://trac.webkit.org/changeset/243039> All reviewed patches have been landed. Closing bug. |