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

Description Chris Dumez 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
Comment 1 Radar WebKit Bug Importer 2019-08-21 12:16:47 PDT
<rdar://problem/54566439>
Comment 2 Chris Dumez 2019-08-21 12:20:25 PDT
Created attachment 376903 [details]
Patch
Comment 3 WebKit Commit Bot 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.
Comment 4 WebKit Commit Bot 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>
Comment 5 WebKit Commit Bot 2019-08-21 15:45:54 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Darin Adler 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.
Comment 7 Alex Christensen 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
Comment 8 Darin Adler 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.
Comment 9 Charlie Turner 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.
Comment 10 Darin Adler 2019-08-29 09:01:09 PDT
Not that I am aware of.
Comment 11 Chris Dumez 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?
Comment 12 Darin Adler 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.