IndexedDB: IDBDatabase and IDBTransaction are leaked in layout tests

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

Found by running layout tests using --leak option.

[Sihui Liu]

<rdar://problem/47769777 >

[Sihui Liu]

STACK OF 1 INSTANCE OF 'ROOT CYCLE: <WebCore>':
[thread 0x70000264c000]:
13  libsystem_pthread.dylib            0x7fff7f0f240d thread_start + 13
12  libsystem_pthread.dylib            0x7fff7f0f6249 _pthread_start + 66
11  libsystem_pthread.dylib            0x7fff7f0f32eb _pthread_body + 126
10  com.apple.JavaScriptCore              0x1087718e9 WTF::wtfThreadEntryPoint(void*) + 9  ThreadingPOSIX.cpp:203
9   com.apple.JavaScriptCore              0x10876fc32 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 194  memory:2633
8   com.apple.WebCore                     0x105a15ec5 WebCore::WorkerThread::workerThread() + 917  RefPtr.h:58
7   com.apple.WebCore                     0x105a13090 WebCore::WorkerRunLoop::run(WebCore::WorkerGlobalScope*) + 96  WorkerRunLoop.cpp:143
6   com.apple.WebCore                     0x105a13284 WebCore::WorkerRunLoop::runInMode(WebCore::WorkerGlobalScope*, WebCore::ModePredicate const&, WebCore::WorkerRunLoop::WaitMode) + 404  Function.h:0
5   com.apple.WebCore                     0x104ba1d2a WebCore::IDBOpenDBRequest::onUpgradeNeeded(WebCore::IDBResultData const&) + 90  DumbPtrTraits.h:41
4   com.apple.WebCore                     0x104b8eae6 WebCore::IDBDatabase::startVersionChangeTransaction(WebCore::IDBTransactionInfo const&, WebCore::IDBOpenDBRequest&) + 22  DumbPtrTraits.h:41
3   com.apple.WebCore                     0x104ba4d64 WebCore::IDBTransaction::create(WebCore::IDBDatabase&, WebCore::IDBTransactionInfo const&, WebCore::IDBOpenDBRequest&) + 36  ThreadSafeRefCounted.h:37
2   com.apple.JavaScriptCore              0x10873843b WTF::fastMalloc(unsigned long) + 91  FastMalloc.cpp:279
1   com.apple.JavaScriptCore              0x1087ac24c bmalloc::DebugHeap::malloc(unsigned long) + 12  DebugHeap.cpp:49
0   libsystem_malloc.dylib             0x7fff7f0afb2d malloc_zone_malloc + 139 
====
    40 (9.03K) ROOT CYCLE: <WebCore::IDBTransaction 0x7fca9d38f430> [608]
       CYCLE BACK TO <WebCore::IDBTransaction 0x7fca9d38f430> [608]
       12 (3.31K) ROOT CYCLE: <WebCore::IDBDatabase 0x7fca9d390550> [288]
          CYCLE BACK TO <WebCore::IDBTransaction 0x7fca9d38f430> [608]
          1 (192 bytes) ROOT CYCLE: 0x7fca9d44fd00 [192]
             CYCLE BACK TO <WebCore::IDBTransaction 0x7fca9d38f430> [608]
          9 (2.78K) 0x7fcaa00cd000 [1536]

[Sihui Liu]

Created attachment 362122 [details]
Patch

[Geoffrey Garen]

Comment on attachment 362122 [details]
Patch

Can we test this in a direct way rather than relying on the leaks tool? For example, can we introduce window.internals.numberOfIDBTransactions, and use that to write a regression test?

[Sihui Liu]

Created attachment 362196 [details]
Patch

[Geoffrey Garen]

Comment on attachment 362196 [details]
Patch

r=me

[Sihui Liu]

Created attachment 362300 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 362300 [details]
Patch for landing

Clearing flags on attachment: 362300

Committed r241722: <https://trac.webkit.org/changeset/241722>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

Test does not have an expectation file and is showing a missing failure in flakiness dashboard :

storage/indexeddb/IDBObject-leak.html

[Shawn Roberts]

Flakiness Dashboard :

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=storage%2Findexeddb%2FIDBObject-leak.html

[Sihui Liu]

Regression test doesn't have an expectation file.

[Sihui Liu]

Created attachment 362332 [details]
Patch

[EWS Watchlist]

Comment on attachment 362332 [details]
Patch

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

New failing tests:
storage/indexeddb/IDBObject-leak.html

[EWS Watchlist]

Created attachment 362346 [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 362332 [details]
Patch

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

New failing tests:
storage/indexeddb/IDBObject-leak.html

[EWS Watchlist]

Created attachment 362353 [details]
Archive of layout-test-results from ews117 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 362332 [details]
Patch

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

New failing tests:
storage/indexeddb/IDBObject-leak.html

[EWS Watchlist]

Created attachment 362356 [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

[Ryosuke Niwa]

Surely, we don't mean to leak these objects in layout tests LOL.

[Shawn Roberts]

(In reply to Ryosuke Niwa from comment #20)
> Surely, we don't mean to leak these objects in layout tests LOL.

Of course not. Had to roll it out however due to the failures. Please let me know when a fix is up and I will verify it is resolved.

[Shawn Roberts]

https://bugs.webkit.org/show_bug.cgi?id=194801

Rolling out in r241722

[Shawn Roberts]

Sorry,

Rolled out in https://trac.webkit.org/changeset/241761

[Chris Dumez]

Comment on attachment 362300 [details]
Patch for landing

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

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:82
> +    auto addResult = allIDBTransactions().add(this);

How is this safe? Aren't IDBTransaction objects constructed from multiple threads? (IDB is exposed to workers AFAIK).

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:113
> +    allIDBTransactions().remove(this);

ditto.

[Alexey Proskuryakov]

rdar://problem/47769777

[Sihui Liu]

Created attachment 362504 [details]
Patch

[Chris Dumez]

Comment on attachment 362504 [details]
Patch

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

> Source/WebCore/testing/Internals.cpp:2390
> +    return IDBTransaction::allIDBTransactions().size();

Still not safe since you're accessing the map without locking here.

> LayoutTests/storage/indexeddb/IDBObject-leak.html:6
> +

jsTestIsAsync = true;

> LayoutTests/storage/indexeddb/IDBObject-leak.html:30
> +        testRunner.waitUntilDone();

You should not use this since you're using js-test and finishJSTest().

> LayoutTests/storage/indexeddb/IDBObject-leak.html:34
> +        }), 1);

Why 1?

[Chris Dumez]

Comment on attachment 362504 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:125
> +    static NeverDestroyed<HashSet<IDBTransaction*>> transactions;

You should ASSERT() that m_countMutex is locked when this method is called.

> Source/WebCore/Modules/indexeddb/IDBTransaction.h:264
> +    Lock m_countMutex;

That lock needs to be static or it's not helping anything since here object gets its own lock.

[Sihui Liu]

Created attachment 362509 [details]
Patch

[Sihui Liu]

Created attachment 362516 [details]
Patch

[Sihui Liu]

Created attachment 362517 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #27)
> Comment on attachment 362504 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362504&action=review
> 
> > Source/WebCore/testing/Internals.cpp:2390
> > +    return IDBTransaction::allIDBTransactions().size();
> 
> Still not safe since you're accessing the map without locking here.
> 
True. Add a shared lock.

> > LayoutTests/storage/indexeddb/IDBObject-leak.html:6
> > +
> 
> jsTestIsAsync = true;
> 
Okay.

> > LayoutTests/storage/indexeddb/IDBObject-leak.html:30
> > +        testRunner.waitUntilDone();
> 
> You should not use this since you're using js-test and finishJSTest().
> 
Removed.

> > LayoutTests/storage/indexeddb/IDBObject-leak.html:34
> > +        }), 1);
> 
> Why 1?

An attempt for waiting network process to end and IDBTransaction to be notified of the disconnection. Removed as it is unnecessary.

[Sihui Liu]

(In reply to Chris Dumez from comment #28)
> Comment on attachment 362504 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=362504&action=review
> 
> > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:125
> > +    static NeverDestroyed<HashSet<IDBTransaction*>> transactions;
> 
> You should ASSERT() that m_countMutex is locked when this method is called.
> 
ASSERT() added.

> > Source/WebCore/Modules/indexeddb/IDBTransaction.h:264
> > +    Lock m_countMutex;
> 
> That lock needs to be static or it's not helping anything since here object
> gets its own lock.
>
static added.

[Chris Dumez]

Comment on attachment 362517 [details]
Patch

Looks correct but it seems like a lot of complexity simply to get a count, especially for testing. You did not really need a HashMap in the first place. It would have simply been a std::atomic<unsigned>, then no need for locks, the code would have been a lot simpler.

[Sihui Liu]

Created attachment 362773 [details]
Patch

[Sihui Liu]

(In reply to Chris Dumez from comment #34)
> Comment on attachment 362517 [details]
> Patch
> 
> Looks correct but it seems like a lot of complexity simply to get a count,
> especially for testing. You did not really need a HashMap in the first
> place. It would have simply been a std::atomic<unsigned>, then no need for
> locks, the code would have been a lot simpler.

Okay, changed to use std::atomic<unsigned>.

[Geoffrey Garen]

Comment on attachment 362773 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 362773 [details]
Patch

Clearing flags on attachment: 362773

Committed r242043: <https://trac.webkit.org/changeset/242043>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

Test is a flaky failure on Mac WK2

storage/indexeddb/IDBObject-leak.html

Started failing after r242043

Reproduced in High Sierra and Mojave

run-webkit-tests storage/indexeddb/IDBObject-leak.html --iterations 100 -f

Flakiness Dashboard:

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=storage%2Findexeddb%2FIDBObject-leak.html

Diff: 

--- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/storage/indexeddb/IDBObject-leak-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/storage/indexeddb/IDBObject-leak-actual.txt
@@ -3,8 +3,9 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS internals.numberOfIDBTransactions() is 0
+FAIL internals.numberOfIDBTransactions() should be 0. Was 1.
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE