Bug 194806 – IndexedDB: re-enable some leak tests

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Sihui Liu]

We have support for internals.observeGC for a while, so we should enable some we didn't run for lack of internals.observeGC.

[Sihui Liu]

Created attachment 362367 [details]
Patch

[Geoffrey Garen]

Comment on attachment 362367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362367&action=review

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-366
> -    VM& vm = context->vm();
> -    JSLockHolder lock(vm);

I think we may still need this lock of the JS VM, since assigning { } to m_resultWrapper may call weakClearSlowCase() and use the JS VM.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-384
> -    JSLockHolder lock(vm);

Ditto.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-402
> -    JSLockHolder lock(vm);

Ditto.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-414
> -

Maybe add a lock here?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-434
> -    JSLockHolder lock(vm);

Ditto.

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:427
> +        JSC::JSValue value = m_resultWrapper;
> +        m_cursorWrapper = value;

Why do we want our cursor to hold our result here?

Probably worth adding an assignment operator to JSValueInWrappedObject to make this more convenient.

[EWS Watchlist]

Comment on attachment 362367 [details]
Patch

Attachment 362367 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11200591

New failing tests:
storage/indexeddb/connection-leak.html
storage/indexeddb/request-leak.html
storage/indexeddb/connection-leak-private.html
storage/indexeddb/delete-closed-database-object-private.html
storage/indexeddb/delete-closed-database-object.html
storage/indexeddb/request-leak-private.html
storage/indexeddb/cursor-request-cycle.html
storage/indexeddb/cursor-leak.html
storage/indexeddb/cursor-request-cycle-private.html
storage/indexeddb/modern/gc-closes-database-private.html
storage/indexeddb/cursor-leak-private.html

[EWS Watchlist]

Created attachment 362368 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Sihui Liu]

Created attachment 362402 [details]
Patch

[Sihui Liu]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 362367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362367&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:-366
> > -    VM& vm = context->vm();
> > -    JSLockHolder lock(vm);
> 
> I think we may still need this lock of the JS VM, since assigning { } to
> m_resultWrapper may call weakClearSlowCase() and use the JS VM.
> 
Add a bool in JSValueInWrappedObject for explicitly invalidate the value, so we don't need the lock any more.

> > Source/WebCore/Modules/indexeddb/IDBRequest.cpp:427
> > +        JSC::JSValue value = m_resultWrapper;
> > +        m_cursorWrapper = value;
> 
> Why do we want our cursor to hold our result here?
> 
Added some explanation in the changelog. IDBCursor is a little special in that if the advance operation is not completed yet, we need to return error as result; and after the operation on success, we want to return the same JSIDBCursor object as before the advance operation. So I cached the result before the operation and restore it after success.

> Probably worth adding an assignment operator to JSValueInWrappedObject to
> make this more convenient.
Sure.

[Geoffrey Garen]

Comment on attachment 362402 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=362402&action=review

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:425
> +    if (m_resultWrapper && !m_resultWrapper.isStale())

I think you added the concept of isStale() to avoid acquiring the JS lock.

I'm worried that it's erorr-prone to have two null states in an object: both operator! null and isStale null. A programmer might forget to check both states.

Also, I just noticed that this object, and probably other objects related to it, need the JS lock in their destructors too.

I think we probably need to just acquire the JS lock. Another option might be to build this null state into JSValueInWrappedObject and make it automatic for things like assignment and operator bool, for convenience. But that won't solve the destructor problem :(.

[Sihui Liu]

Created attachment 363946 [details]
Patch

[Geoffrey Garen]

Comment on attachment 363946 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=363946&action=review

r=me

> Source/WebCore/Modules/indexeddb/IDBCursor.cpp:350
> +void IDBCursor::clearRequest()

I think we need a lock here too because we clear wrappers.

[EWS Watchlist]

Comment on attachment 363946 [details]
Patch

Attachment 363946 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11420183

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 363955 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363946 [details]
Patch

Attachment 363946 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11420429

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 363959 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363946 [details]
Patch

Attachment 363946 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11420880

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 363964 [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 363946 [details]
Patch

Attachment 363946 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11424049

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
compositing/geometry/ancestor-clip-change.html
css3/filters/blur-various-radii.html

[EWS Watchlist]

Created attachment 363992 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Sihui Liu]

Created attachment 363996 [details]
Patch

[EWS Watchlist]

Comment on attachment 363996 [details]
Patch

Attachment 363996 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11424857

New failing tests:
storage/indexeddb/pending-version-change-on-exit-private.html
storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html
storage/indexeddb/pending-version-change-stuck.html
http/tests/security/cross-origin-worker-indexeddb.html
storage/indexeddb/modern/blob-simple.html
imported/w3c/web-platform-tests/IndexedDB/idbcursor-advance-continue-async.htm
storage/indexeddb/objectstore-basics.html
storage/indexeddb/opencursor-key-private.html
storage/indexeddb/pending-version-change-on-exit.html
storage/indexeddb/basics.html
storage/indexeddb/transaction-complete-workers.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate.html
storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html
imported/w3c/IndexedDB-private-browsing/idbcursor-advance-continue-async.html
storage/indexeddb/pending-version-change-stuck-private.html

[EWS Watchlist]

Created attachment 363999 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363996 [details]
Patch

Attachment 363996 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11424863

New failing tests:
storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html
storage/indexeddb/pending-version-change-stuck.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html
storage/indexeddb/opencursor-key-private.html
storage/indexeddb/modern/blob-simple.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate.html
imported/w3c/web-platform-tests/IndexedDB/idbcursor-advance-continue-async.htm
storage/indexeddb/pending-version-change-on-exit.html
http/wpt/cache-storage/cache-quota.any.html
storage/indexeddb/objectstore-basics.html
storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html
storage/indexeddb/basics.html
storage/indexeddb/transaction-complete-workers.html
storage/indexeddb/cursor-advance.html
http/tests/security/cross-origin-worker-indexeddb.html
imported/w3c/IndexedDB-private-browsing/idbcursor-advance-continue-async.html
storage/indexeddb/pending-activity.html
storage/indexeddb/index-basics.html
storage/indexeddb/dont-wedge-private.html

[EWS Watchlist]

Created attachment 364000 [details]
Archive of layout-test-results from ews107 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363996 [details]
Patch

Attachment 363996 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11424885

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 364002 [details]
Archive of layout-test-results from ews115 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363996 [details]
Patch

Attachment 363996 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11426624

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall.https.html
storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html
storage/indexeddb/pending-version-change-stuck.html
storage/indexeddb/modern/blob-simple.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate.html
imported/w3c/web-platform-tests/IndexedDB/idbcursor-advance-continue-async.htm
storage/indexeddb/objectstore-basics.html
storage/indexeddb/opencursor-key-private.html
storage/indexeddb/pending-version-change-on-exit.html
storage/indexeddb/basics.html
storage/indexeddb/transaction-complete-workers.html
storage/indexeddb/cursor-advance.html
storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html
imported/w3c/IndexedDB-private-browsing/idbcursor-advance-continue-async.html
storage/indexeddb/pending-activity.html
storage/indexeddb/index-basics.html
storage/indexeddb/dont-wedge-private.html

[EWS Watchlist]

Created attachment 364018 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 363996 [details]
Patch

Attachment 363996 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11460454

New failing tests:
storage/indexeddb/pending-version-change-on-exit-private.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate-private.html
storage/indexeddb/modern/worker-getall.html
storage/indexeddb/modern/blob-simple-workers.html
storage/indexeddb/pending-version-change-stuck-private.html
storage/indexeddb/deletedatabase-delayed-by-open-and-versionchange-workers.html
storage/indexeddb/dont-commit-on-blocked-private.html
storage/indexeddb/pending-version-change-stuck.html
storage/indexeddb/pending-version-change-on-exit.html
storage/indexeddb/basics-workers.html
storage/indexeddb/dont-commit-on-blocked.html
storage/indexeddb/transaction-complete-workers.html
storage/indexeddb/pending-version-change-stuck-works-with-terminate.html
storage/indexeddb/transaction-complete-workers-private.html
storage/indexeddb/modern/worker-transaction-open-after-worker-stop.html
storage/indexeddb/pending-activity-workers.html
storage/indexeddb/cursor-advance-workers.html
storage/indexeddb/index-basics-workers.html
storage/indexeddb/open-twice-workers.html

[EWS Watchlist]

Created attachment 364294 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Geoffrey Garen]

Seems like you've discovered a pre-existing use-after-free.

We either need to perform this shutdown in a slightly different order, such that script() and script()->vm() are not null during shutdown, or we need another way to access VM* from IDB destructors.

[Sihui Liu]

Created attachment 364667 [details]
Patch

[Geoffrey Garen]

Comment on attachment 364667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364667&action=review

r=me with two suggested fixes

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:155
> +    if (!m_contextStopped)
> +        clearWrappers();

I think you can remove this. Let's discuss.

> Source/WebCore/bindings/js/JSValueInWrappedObject.h:39
> +    JSValueInWrappedObject(JSValueInWrappedObject&);

Should this be const JSValueInWrappedObject&? I don't think this is a copy constructor if you forget the const.

[Geoffrey Garen]

Comment on attachment 364667 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=364667&action=review

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:297
> +        auto* context = scriptExecutionContext();
> +        VM& vm = context->vm();
> +        JSLockHolder lock(vm);

For clarity, we might as well move this lock into the the callee (clearWrappers()).

[Sihui Liu]

Created attachment 364749 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 364749 [details]
Patch for landing

Clearing flags on attachment: 364749

Committed r242986: <https://trac.webkit.org/changeset/242986>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48912775>