Bug 195650

Summary: Layout tests imported/w3c/web-platform-tests/IndexedDB/*-exception-order.html are failing
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: 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 Flags
Patch
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Archive of layout-test-results from ews103 for mac-highsierra
none
Archive of layout-test-results from ews112 for mac-highsierra
none
Archive of layout-test-results from ews201 for win-future
none
Patch
none
Patch for landing none

Description Sihui Liu 2019-03-12 15:58:13 PDT
The exception orders are wrong according to WPT tests.
Comment 1 Sihui Liu 2019-03-12 16:12:19 PDT
Created attachment 364466 [details]
Patch
Comment 2 EWS Watchlist 2019-03-12 16:51:50 PDT
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.
Comment 3 EWS Watchlist 2019-03-12 16:51:51 PDT
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 4 EWS Watchlist 2019-03-12 16:57:35 PDT
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.
Comment 5 EWS Watchlist 2019-03-12 16:57:37 PDT
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 6 EWS Watchlist 2019-03-12 17:05:53 PDT
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.
Comment 7 EWS Watchlist 2019-03-12 17:05:54 PDT
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 8 EWS Watchlist 2019-03-12 19:41:54 PDT
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.
Comment 9 EWS Watchlist 2019-03-12 19:42:05 PDT
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
Comment 10 Sihui Liu 2019-03-13 12:26:36 PDT
Created attachment 364561 [details]
Patch
Comment 11 Ryosuke Niwa 2019-03-14 17:48:18 PDT
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 { ~ }?
Comment 12 Sihui Liu 2019-03-16 01:20:23 PDT
Created attachment 364923 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2019-03-16 01:57:28 PDT
Comment on attachment 364923 [details]
Patch for landing

Clearing flags on attachment: 364923

Committed r243039: <https://trac.webkit.org/changeset/243039>
Comment 14 WebKit Commit Bot 2019-03-16 01:57:30 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Radar WebKit Bug Importer 2019-03-16 01:58:22 PDT
<rdar://problem/48951473>