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.
Created attachment 346914 [details] Patch
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 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.
Created attachment 346923 [details] Patch
(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?
<rdar://problem/42657666>
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.
(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.
Created attachment 346932 [details] Patch
Created attachment 346939 [details] Patch for landing
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?
Created attachment 346940 [details] Patch
Comment on attachment 346940 [details] Patch r=me
Comment on attachment 346940 [details] Patch Clearing flags on attachment: 346940 Committed r234791: <https://trac.webkit.org/changeset/234791>
All reviewed patches have been landed. Closing bug.