Bug 188474 – CrashTracer: com.apple.WebKit.Storage at WebCore::IDBServer::UniqueIDBDatabase::connectionClosedFromClient(WebCore::IDBServer::UniqueIDBDatabaseConnection&)

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 346914 [details]
Patch

[Alex Christensen]

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?

[Chris Dumez]

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.

[Sihui Liu]

Created attachment 346923 [details]
Patch

[Sihui Liu]

(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?

[Sihui Liu]

<rdar://problem/42657666>

[Chris Dumez]

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.

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 346932 [details]
Patch

[Sihui Liu]

Created attachment 346939 [details]
Patch for landing

[Chris Dumez]

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?

[Sihui Liu]

Created attachment 346940 [details]
Patch

[Chris Dumez]

Comment on attachment 346940 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 346940 [details]
Patch

Clearing flags on attachment: 346940

Committed r234791: <https://trac.webkit.org/changeset/234791>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.