Bug 200990

Summary: Crash under StringImpl::endsWith() in SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade()
Product: WebKit Reporter: Chris Dumez <cdumez>
Component: WebKit2Assignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, alecflett, beidson, commit-queue, cturner, darin, ews-watchlist, ggaren, jsbell, sihui_liu, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=200989
https://bugs.webkit.org/show_bug.cgi?id=201318
Attachments:
Description Flags
Patch none

Chris Dumez
Reported 2019-08-21 12:16:24 PDT
Crash under StringImpl::endsWith() in SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade(): Thread 3 name: IndexedDatabase Server Thread 3 Crashed ↩: 0 JavaScriptCore 0x00000001927c26f0 WTF::StringImpl::endsWith(unsigned short) const + 52 (StringImpl.h:1100) 1 JavaScriptCore 0x000000019279c508 WTF::FileSystemImpl::pathByAppendingComponent(WTF::String const&, WTF::String const&) + 56 (WTFString.h:221) 2 WebCore 0x000000018befdb78 WebCore::IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(WebCore::SecurityOriginData const&, WebCore::SecurityOriginData const&, WTF::String const&, WTF::String const&) + 52 (IDBDatabaseIdentifier.cpp:65) 3 WebCore 0x000000018bf62cc0 WebCore::IDBServer::SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade() + 76 (SQLiteIDBBackingStore.cpp:796) 4 WebCore 0x000000018bf62acc WebCore::IDBServer::SQLiteIDBBackingStore::SQLiteIDBBackingStore(WebCore::IDBDatabaseIdentifier const&, WTF::String const&, WebCore::IDBServer::IDBBackingStoreTemporaryFileHandler&, unsigned long long) + 284 (SQLiteIDBBackingStore.cpp:237) 5 WebCore 0x000000018bf3abd4 WebCore::IDBServer::IDBServer::createBackingStore(WebCore::IDBDatabaseIdentifier const&) + 92 (memory:3132) 6 WebCore 0x000000018bf7a010 WebCore::IDBServer::UniqueIDBDatabase::openBackingStore(WebCore::IDBDatabaseIdentifier const&) + 36 (UniqueIDBDatabase.cpp:752) 7 WebCore 0x000000018bf8defc WTF::Detail::CallableWrapper<WTF::CrossThreadTask WTF::createCrossThreadTask<WebCore::IDBServer::UniqueIDBDatabase, 0, WebCore::IDBDatabaseIdentifier const&, WebCore::IDBDatabaseIdentifier>(WebCore::IDBServer::UniqueIDBDatabase&, void (WebCore::IDBServer::UniqueIDBDatabase::*)(WebCore::IDBDatabaseIdentifier const&), WebCore::IDBDatabaseIdentifier const&)::'lambda'(), void>::call() + 72 (CrossThreadTask.h:78) 8 WebCore 0x000000018bf86c14 WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTask() + 192 (Function.h:79) 9 WebCore 0x000000018bf8dfa4 WTF::Detail::CallableWrapper<WTF::CrossThreadTask WTF::createCrossThreadTask<WebCore::IDBServer::UniqueIDBDatabase, 0>(WebCore::IDBServer::UniqueIDBDatabase&, void (WebCore::IDBServer::UniqueIDBDatabase::*)())::'lambda'(), void>::call() + 64 (CrossThreadTask.h:78) 10 JavaScriptCore 0x00000001927919a4 WTF::CrossThreadTaskHandler::taskRunLoop() + 256 (Function.h:79) 11 JavaScriptCore 0x00000001927cbf18 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 260 (Function.h:79) 12 JavaScriptCore 0x00000001927cda64 WTF::wtfThreadEntryPoint(void*) + 16 (ThreadingPOSIX.cpp:200) 13 libsystem_pthread.dylib 0x0000000183690d60 _pthread_start + 128 (pthread.c:895) 14 libsystem_pthread.dylib 0x0000000183698c88 thread_start + 8
Attachments
Patch (4.73 KB, patch)
2019-08-21 12:20 PDT, Chris Dumez 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-08-21 12:16:47 PDT
<rdar://problem/54566439>
Chris Dumez
Comment 2 2019-08-21 12:20:25 PDT
Created attachment 376903 [details] Patch
WebKit Commit Bot
Comment 3 2019-08-21 15:45:10 PDT
The commit-queue encountered the following flaky tests while processing attachment 376903 [details]: media/modern-media-controls/compact-media-controls/compact-media-controls-constructor.html bug 193587 (author: graouts@apple.com) The commit-queue is continuing to process your patch.
WebKit Commit Bot
Comment 4 2019-08-21 15:45:52 PDT
Comment on attachment 376903 [details] Patch Clearing flags on attachment: 376903 Committed r248971: <https://trac.webkit.org/changeset/248971>
WebKit Commit Bot
Comment 5 2019-08-21 15:45:54 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 6 2019-08-22 09:25:05 PDT
Comment on attachment 376903 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376903&action=review > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:112 > + String databaseRootDirectory() const { return m_databaseRootDirectory.isolatedCopy(); } I’m not sure I understand this idiom. Why does the getter do an isolated copy? How does it know this is a cross-thread case? I’m not doubting that this patch works, but I’m trying to understand our general coding style approach for this in the future.
Alex Christensen
Comment 7 2019-08-22 09:40:53 PDT
I agree it's not the best and expressed this in https://bugs.webkit.org/show_bug.cgi?id=200989#c3
Darin Adler
Comment 8 2019-08-22 10:03:30 PDT
It’s clear that isolated copies are needed, but my first thought without deep reflection is that putting one inside the getter seems confusing to me and a pattern that won’t work well for future cases like this.
Charlie Turner
Comment 9 2019-08-29 08:55:48 PDT
Was there an answer to this question offline at all? I'm also not sure what our coding style is in these cases.
Darin Adler
Comment 10 2019-08-29 09:01:09 PDT
Not that I am aware of.
Chris Dumez
Comment 11 2019-08-29 09:32:30 PDT
(In reply to Darin Adler from comment #10) > Not that I am aware of. (In reply to Darin Adler from comment #6) > Comment on attachment 376903 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376903&action=review > > > Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:112 > > + String databaseRootDirectory() const { return m_databaseRootDirectory.isolatedCopy(); } > > I’m not sure I understand this idiom. Why does the getter do an isolated > copy? How does it know this is a cross-thread case? > > I’m not doubting that this patch works, but I’m trying to understand our > general coding style approach for this in the future. I did this for convenience here because it is a private method and all all sites actually needed the isolatedCopy. This is a pattern we were already using in the network cache as well. Maybe I could add "IsolatedCopy" in the method name to make it clearer?
Darin Adler
Comment 12 2019-08-29 11:37:31 PDT
(In reply to Chris Dumez from comment #11) > Maybe I could add "IsolatedCopy" in the method name to make it clearer? Yes, I think that’s a good idea. Make sure nobody makes a mistake like calling it in a loop and spending lots of memory or something like that. Not that it’s likely in this case, but it seems like a good idiom.
Note You need to log in before you can comment on or make changes to this bug.