Bug 194806

Summary: IndexedDB: re-enable some leak tests
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: Tools / TestsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: alecflett, beidson, commit-queue, ews-watchlist, ggaren, jsbell, lforschler, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 236277    
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews200 for win-future
none
Patch
none
Patch
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews112 for mac-highsierra
none
Archive of layout-test-results from ews124 for ios-simulator-wk2
none
Patch
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews107 for mac-highsierra-wk2
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Archive of layout-test-results from ews122 for ios-simulator-wk2
none
Archive of layout-test-results from ews200 for win-future
none
Patch
none
Patch for landing none

Description Sihui Liu 2019-02-18 20:55:56 PST
We have support for internals.observeGC for a while, so we should enable some we didn't run for lack of internals.observeGC.
Comment 1 Sihui Liu 2019-02-18 21:02:50 PST
Created attachment 362367 [details]
Patch
Comment 2 Geoffrey Garen 2019-02-18 21:34:17 PST
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.
Comment 3 EWS Watchlist 2019-02-18 22:43:03 PST
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
Comment 4 EWS Watchlist 2019-02-18 22:43:14 PST
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
Comment 5 Sihui Liu 2019-02-19 11:30:28 PST
Created attachment 362402 [details]
Patch
Comment 6 Sihui Liu 2019-02-19 11:35:28 PST
(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.
Comment 7 Geoffrey Garen 2019-03-05 17:23:56 PST
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 :(.
Comment 8 Sihui Liu 2019-03-07 16:03:50 PST
Created attachment 363946 [details]
Patch
Comment 9 Geoffrey Garen 2019-03-07 16:09:17 PST
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.
Comment 10 EWS Watchlist 2019-03-07 16:37:03 PST
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.
Comment 11 EWS Watchlist 2019-03-07 16:37:05 PST
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
Comment 12 EWS Watchlist 2019-03-07 16:59:31 PST
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.
Comment 13 EWS Watchlist 2019-03-07 16:59:32 PST
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
Comment 14 EWS Watchlist 2019-03-07 17:42:17 PST
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.
Comment 15 EWS Watchlist 2019-03-07 17:42:19 PST
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
Comment 16 EWS Watchlist 2019-03-07 23:49:42 PST
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
Comment 17 EWS Watchlist 2019-03-07 23:49:44 PST
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
Comment 18 Sihui Liu 2019-03-08 01:07:18 PST
Created attachment 363996 [details]
Patch
Comment 19 EWS Watchlist 2019-03-08 02:28:05 PST
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
Comment 20 EWS Watchlist 2019-03-08 02:28:06 PST
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
Comment 21 EWS Watchlist 2019-03-08 02:34:47 PST
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
Comment 22 EWS Watchlist 2019-03-08 02:34:49 PST
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
Comment 23 EWS Watchlist 2019-03-08 03:08:07 PST
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.
Comment 24 EWS Watchlist 2019-03-08 03:08:09 PST
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
Comment 25 EWS Watchlist 2019-03-08 09:25:21 PST
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
Comment 26 EWS Watchlist 2019-03-08 09:25:23 PST
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
Comment 27 EWS Watchlist 2019-03-11 15:02:54 PDT
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
Comment 28 EWS Watchlist 2019-03-11 15:03:05 PDT
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
Comment 29 Geoffrey Garen 2019-03-12 12:49:42 PDT
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.
Comment 30 Sihui Liu 2019-03-14 11:27:31 PDT
Created attachment 364667 [details]
Patch
Comment 31 Geoffrey Garen 2019-03-14 16:27:44 PDT
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.
Comment 32 Geoffrey Garen 2019-03-14 17:00:39 PDT
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()).
Comment 33 Sihui Liu 2019-03-14 18:46:24 PDT
Created attachment 364749 [details]
Patch for landing
Comment 34 WebKit Commit Bot 2019-03-14 19:25:00 PDT
Comment on attachment 364749 [details]
Patch for landing

Clearing flags on attachment: 364749

Committed r242986: <https://trac.webkit.org/changeset/242986>
Comment 35 WebKit Commit Bot 2019-03-14 19:25:03 PDT
All reviewed patches have been landed.  Closing bug.
Comment 36 Radar WebKit Bug Importer 2019-03-14 19:28:52 PDT
<rdar://problem/48912775>