Bug 188474

Summary: CrashTracer: com.apple.WebKit.Storage at WebCore::IDBServer::UniqueIDBDatabase::connectionClosedFromClient(WebCore::IDBServer::UniqueIDBDatabaseConnection&)
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: Sihui Liu <sihui_liu>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, cdumez, commit-queue, ews-watchlist, jsbell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch for landing
none
Patch none

Description Sihui Liu 2018-08-10 11:01:06 PDT 
Thread 0 Crashed ↩:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00007fff4b774a9d WTF::ListHashSet<WTF::RefPtr<WebCore::IDBServer::UniqueIDBDatabaseConnection, WTF::DumbPtrTraits<WebCore::IDBServer::UniqueIDBDatabaseConnection> >, WTF::PtrHash<WTF::RefPtr<WebCore::IDBServer::UniqueIDBDatabaseConnection, WTF::DumbPtrTraits<WebCore::IDBServer::UniqueIDBDatabaseConnection> > > >::find(WTF::RefPtr<WebCore::IDBServer::UniqueIDBDatabaseConnection, WTF::DumbPtrTraits<WebCore::IDBServer::UniqueIDBDatabaseConnection> > const&) + 173
1   com.apple.WebCore             	0x00007fff4b7614a7 WebCore::IDBServer::UniqueIDBDatabase::connectionClosedFromClient(WebCore::IDBServer::UniqueIDBDatabaseConnection&) + 55
2   com.apple.WebCore             	0x00007fff4b727a9d WebCore::IDBServer::IDBConnectionToClient::connectionToClientClosed() + 317
3   com.apple.WebCore             	0x00007fff4b7294a5 WebCore::IDBServer::IDBServer::unregisterConnection(WebCore::IDBServer::IDBConnectionToClient&) + 21
4   com.apple.WebKit              	0x00007fff4c99772a WebKit::StorageToWebProcessConnection::didClose(IPC::Connection&) + 226
5   com.apple.JavaScriptCore      	0x00007fff40e63e97 WTF::RunLoop::performWork() + 231
6   com.apple.JavaScriptCore      	0x00007fff40e64122 WTF::RunLoop::performWork(void*) + 34

As Chris suggested, this crash may be caused by stale reference to UniqueIDBDatabase in UniqueIDBDatabaseConnection. UniqueIDBDatabaseConnection could outlive UniqueIDBDatabase because it's refcounted by UniqueIDBDatabaseTransaction, and it holds refcount of UniqueIDBDatabaseTransaction in m_transactionMap.
To make the code more robust, and also put up a speculative fix for this crash, we should make the UniqueIDBDatabase a WeakPtr. Also, assertions are added to make it easier for debugging related storage process crashes.
Comment 1 Sihui Liu 2018-08-10 11:49:28 PDT 
Created attachment 346914 [details]
Patch
Comment 2 Alex Christensen 2018-08-10 11:51:49 PDT 
Comment on attachment 346914 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:66
> +UniqueIDBDatabase& UniqueIDBDatabaseConnection::database()
> +{
> +    ASSERT(m_database);
> +    return *m_database;
> +}

What's this used for?  Doesn't this defeat the purpose of a WeakPtr?
Comment 3 Chris Dumez 2018-08-10 11:53:00 PDT 
Comment on attachment 346914 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:66
>> +}
> 
> What's this used for?  Doesn't this defeat the purpose of a WeakPtr?

I agree with Alex, we probably want to return a raw pointer and have the caller null-check.
Comment 4 Sihui Liu 2018-08-10 14:14:07 PDT 
Created attachment 346923 [details]
Patch
Comment 5 Sihui Liu 2018-08-10 14:18:07 PDT 
(In reply to Chris Dumez from comment #3)
> Comment on attachment 346914 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346914&action=review
> 
> >> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:66
> >> +}
> > 
> > What's this used for?  Doesn't this defeat the purpose of a WeakPtr?
> 
> I agree with Alex, we probably want to return a raw pointer and have the
> caller null-check.

Added assertions to callers in IDBDatabaseTransaction and made database() raw pointer. Do you think we should make early returns on null?
Comment 6 Sihui Liu 2018-08-10 14:18:41 PDT 
<rdar://problem/42657666>
Comment 7 Chris Dumez 2018-08-10 14:38:56 PDT 
Comment on attachment 346923 [details]
Patch

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

r=me with comments.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:75
> +    if (m_database) {

This would look better as an early return.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:62
> +    ASSERT(database);

We probably want to null check it since we believe the transaction / connection can outlive their database.
Comment 8 Sihui Liu 2018-08-10 14:59:59 PDT 
(In reply to Chris Dumez from comment #7)
> Comment on attachment 346923 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346923&action=review
> 
> r=me with comments.
> 
> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:75
> > +    if (m_database) {
> 
> This would look better as an early return.
> 
Okay.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:62
> > +    ASSERT(database);
> 
> We probably want to null check it since we believe the transaction /
> connection can outlive their database.
Done.
Comment 9 Sihui Liu 2018-08-10 15:00:06 PDT 
Created attachment 346932 [details]
Patch
Comment 10 Sihui Liu 2018-08-10 16:46:02 PDT 
Created attachment 346939 [details]
Patch for landing
Comment 11 Chris Dumez 2018-08-10 16:47:26 PDT 
Comment on attachment 346939 [details]
Patch for landing

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

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:189
> +    RELEASE_ASSERT(injectedBundle.isTestRunning());

Why is this in there?
Comment 12 Sihui Liu 2018-08-10 16:50:03 PDT 
Created attachment 346940 [details]
Patch
Comment 13 Chris Dumez 2018-08-10 16:59:26 PDT 
Comment on attachment 346940 [details]
Patch

r=me
Comment 14 WebKit Commit Bot 2018-08-12 23:33:14 PDT 
Comment on attachment 346940 [details]
Patch

Clearing flags on attachment: 346940

Committed r234791: <https://trac.webkit.org/changeset/234791>
Comment 15 WebKit Commit Bot 2018-08-12 23:33:16 PDT 
All reviewed patches have been landed.  Closing bug.