Bug 194806 - IndexedDB: re-enable some leak tests
Summary: IndexedDB: re-enable some leak tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Tools / Tests (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-18 20:55 PST by Sihui Liu
Modified: 2019-03-14 19:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (16.66 KB, patch)
2019-02-18 21:02 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews200 for win-future (12.89 MB, application/zip)
2019-02-18 22:43 PST, Build Bot
no flags Details
Patch (23.51 KB, patch)
2019-02-19 11:30 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (21.59 KB, patch)
2019-03-07 16:03 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.07 MB, application/zip)
2019-03-07 16:37 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (1.48 MB, application/zip)
2019-03-07 16:59 PST, Build Bot
no flags Details
Archive of layout-test-results from ews112 for mac-highsierra (2.43 MB, application/zip)
2019-03-07 17:42 PST, Build Bot
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (1.22 MB, application/zip)
2019-03-07 23:49 PST, Build Bot
no flags Details
Patch (22.91 KB, patch)
2019-03-08 01:07 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (4.73 MB, application/zip)
2019-03-08 02:28 PST, Build Bot
no flags Details
Archive of layout-test-results from ews107 for mac-highsierra-wk2 (4.72 MB, application/zip)
2019-03-08 02:34 PST, Build Bot
no flags Details
Archive of layout-test-results from ews115 for mac-highsierra (3.53 MB, application/zip)
2019-03-08 03:08 PST, Build Bot
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (11.99 MB, application/zip)
2019-03-08 09:25 PST, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (13.04 MB, application/zip)
2019-03-11 15:03 PDT, Build Bot
no flags Details
Patch (22.41 KB, patch)
2019-03-14 11:27 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (22.12 KB, patch)
2019-03-14 18:46 PDT, Sihui Liu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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 Build Bot 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>