Bug 195650 – Layout tests imported/w3c/web-platform-tests/IndexedDB/*-exception-order.html are failing

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

The exception orders are wrong according to WPT tests.

[Sihui Liu]

Created attachment 364466 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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.

[EWS Watchlist]

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

[Sihui Liu]

Created attachment 364561 [details]
Patch

[Ryosuke Niwa]

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 { ~ }?

[Sihui Liu]

Created attachment 364923 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364923 [details]
Patch for landing

Clearing flags on attachment: 364923

Committed r243039: <https://trac.webkit.org/changeset/243039>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48951473>