Bug 194709 - IndexedDB: IDBDatabase and IDBTransaction are leaked in layout tests
Summary: IndexedDB: IDBDatabase and IDBTransaction are leaked in layout tests
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Sihui Liu
URL:
Keywords: InRadar
Depends on: 194801
Blocks:
  Show dependency treegraph
 
Reported: 2019-02-15 09:56 PST by Sihui Liu
Modified: 2019-02-25 16:55 PST (History)
11 users (show)

See Also:


Attachments
Patch (3.33 KB, patch)
2019-02-15 10:12 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (9.65 KB, patch)
2019-02-15 17:59 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch for landing (9.33 KB, patch)
2019-02-18 08:52 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (1.27 KB, patch)
2019-02-18 14:16 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-highsierra (2.42 MB, application/zip)
2019-02-18 15:19 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-highsierra (2.17 MB, application/zip)
2019-02-18 15:58 PST, Build Bot
no flags Details
Archive of layout-test-results from ews200 for win-future (12.82 MB, application/zip)
2019-02-18 16:20 PST, Build Bot
no flags Details
Patch (11.74 KB, patch)
2019-02-20 09:33 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (11.80 KB, patch)
2019-02-20 10:11 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (11.88 KB, patch)
2019-02-20 11:13 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (11.86 KB, patch)
2019-02-20 11:20 PST, Sihui Liu
no flags Details | Formatted Diff | Diff
Patch (11.50 KB, patch)
2019-02-22 15:51 PST, 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-15 09:56:05 PST
Found by running layout tests using --leak option.
Comment 1 Sihui Liu 2019-02-15 09:56:47 PST
<rdar://problem/47769777 >
Comment 2 Sihui Liu 2019-02-15 09:57:28 PST
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]
Comment 3 Sihui Liu 2019-02-15 10:12:17 PST
Created attachment 362122 [details]
Patch
Comment 4 Geoffrey Garen 2019-02-15 10:49:26 PST
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?
Comment 5 Sihui Liu 2019-02-15 17:59:37 PST
Created attachment 362196 [details]
Patch
Comment 6 Geoffrey Garen 2019-02-15 20:07:20 PST
Comment on attachment 362196 [details]
Patch

r=me
Comment 7 Sihui Liu 2019-02-18 08:52:33 PST
Created attachment 362300 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2019-02-18 09:30:23 PST
Comment on attachment 362300 [details]
Patch for landing

Clearing flags on attachment: 362300

Committed r241722: <https://trac.webkit.org/changeset/241722>
Comment 9 WebKit Commit Bot 2019-02-18 09:30:25 PST
All reviewed patches have been landed.  Closing bug.
Comment 10 Shawn Roberts 2019-02-18 14:06:26 PST
Test does not have an expectation file and is showing a missing failure in flakiness dashboard :

storage/indexeddb/IDBObject-leak.html
Comment 12 Sihui Liu 2019-02-18 14:13:18 PST
Regression test doesn't have an expectation file.
Comment 13 Sihui Liu 2019-02-18 14:16:12 PST
Created attachment 362332 [details]
Patch
Comment 14 Build Bot 2019-02-18 15:19:10 PST
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
Comment 15 Build Bot 2019-02-18 15:19:12 PST
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
Comment 16 Build Bot 2019-02-18 15:58:37 PST
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
Comment 17 Build Bot 2019-02-18 15:58:39 PST
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
Comment 18 Build Bot 2019-02-18 16:20:21 PST
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
Comment 19 Build Bot 2019-02-18 16:20:33 PST
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
Comment 20 Ryosuke Niwa 2019-02-18 20:11:56 PST
Surely, we don't mean to leak these objects in layout tests LOL.
Comment 21 Shawn Roberts 2019-02-19 08:50:55 PST
(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.
Comment 22 Shawn Roberts 2019-02-19 08:54:14 PST
https://bugs.webkit.org/show_bug.cgi?id=194801

Rolling out in r241722
Comment 23 Shawn Roberts 2019-02-19 08:56:15 PST
Sorry,

Rolled out in https://trac.webkit.org/changeset/241761
Comment 24 Chris Dumez 2019-02-19 08:59:17 PST
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.
Comment 25 Alexey Proskuryakov 2019-02-19 12:01:47 PST
rdar://problem/47769777
Comment 26 Sihui Liu 2019-02-20 09:33:36 PST
Created attachment 362504 [details]
Patch
Comment 27 Chris Dumez 2019-02-20 09:36:55 PST
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?
Comment 28 Chris Dumez 2019-02-20 09:43:16 PST
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.
Comment 29 Sihui Liu 2019-02-20 10:11:41 PST
Created attachment 362509 [details]
Patch
Comment 30 Sihui Liu 2019-02-20 11:13:41 PST
Created attachment 362516 [details]
Patch
Comment 31 Sihui Liu 2019-02-20 11:20:04 PST
Created attachment 362517 [details]
Patch
Comment 32 Sihui Liu 2019-02-20 11:35:26 PST
(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.
Comment 33 Sihui Liu 2019-02-20 11:36:19 PST
(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.
Comment 34 Chris Dumez 2019-02-21 07:53:17 PST
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.
Comment 35 Sihui Liu 2019-02-22 15:51:15 PST
Created attachment 362773 [details]
Patch
Comment 36 Sihui Liu 2019-02-22 15:55:51 PST
(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>.
Comment 37 Geoffrey Garen 2019-02-23 09:21:19 PST
Comment on attachment 362773 [details]
Patch

r=me
Comment 38 WebKit Commit Bot 2019-02-25 08:50:26 PST
Comment on attachment 362773 [details]
Patch

Clearing flags on attachment: 362773

Committed r242043: <https://trac.webkit.org/changeset/242043>
Comment 39 WebKit Commit Bot 2019-02-25 08:50:28 PST
All reviewed patches have been landed.  Closing bug.
Comment 40 Shawn Roberts 2019-02-25 16:55:26 PST
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