200990 2019-08-21 12:16:24 -0700 Crash under StringImpl::endsWith() in SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade() 2019-08-29 16:19:40 -0700 1 1 1 Unclassified WebKit WebKit2 WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED https://bugs.webkit.org/show_bug.cgi?id=200989 https://bugs.webkit.org/show_bug.cgi?id=201318 InRadar P2 Normal --- 1 cdumez cdumez achristensen alecflett beidson commit-queue cturner darin ews-watchlist ggaren jsbell sihui_liu webkit-bug-importer oldest_to_newest 1563602 0 cdumez 2019-08-21 12:16:24 -0700 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 1563603 1 webkit-bug-importer 2019-08-21 12:16:47 -0700 <rdar://problem/54566439> 1563609 2 376903 cdumez 2019-08-21 12:20:25 -0700 Created attachment 376903 Patch 1563760 3 commit-queue 2019-08-21 15:45:10 -0700 The commit-queue encountered the following flaky tests while processing attachment 376903: 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. 1563761 4 376903 commit-queue 2019-08-21 15:45:52 -0700 Comment on attachment 376903 Patch Clearing flags on attachment: 376903 Committed r248971: <https://trac.webkit.org/changeset/248971> 1563762 5 commit-queue 2019-08-21 15:45:54 -0700 All reviewed patches have been landed. Closing bug. 1564080 6 376903 darin 2019-08-22 09:25:05 -0700 Comment on attachment 376903 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. 1564085 7 achristensen 2019-08-22 09:40:53 -0700 I agree it's not the best and expressed this in https://bugs.webkit.org/show_bug.cgi?id=200989#c3 1564101 8 darin 2019-08-22 10:03:30 -0700 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. 1566254 9 cturner 2019-08-29 08:55:48 -0700 Was there an answer to this question offline at all? I'm also not sure what our coding style is in these cases. 1566257 10 darin 2019-08-29 09:01:09 -0700 Not that I am aware of. 1566270 11 cdumez 2019-08-29 09:32:30 -0700 (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? 1566305 12 darin 2019-08-29 11:37:31 -0700 (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. 376903 2019-08-21 12:20:25 -0700 2019-08-21 15:45:52 -0700 Patch bug-200990-20190821122024.patch text/plain 4841 cdumez U3VidmVyc2lvbiBSZXZpc2lvbjogMjQ4OTU0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggZDk5MTMwYTE4MmM3MWFj YTQyMDQxZmYzMWM1YjJlOTY2ZDAwZjg1Ni4uNjFjNzU5YzczMmIxMWFiM2U1M2NkMDIyYTY2MDlm MmMzMGU1YjJjOCAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDE5LTA4LTIxICBDaHJp cyBEdW1leiAgPGNkdW1lekBhcHBsZS5jb20+CisKKyAgICAgICAgQ3Jhc2ggdW5kZXIgU3RyaW5n SW1wbDo6ZW5kc1dpdGgoKSBpbiBTUUxpdGVJREJCYWNraW5nU3RvcmU6OmZ1bGxEYXRhYmFzZURp cmVjdG9yeVdpdGhVcGdyYWRlKCkKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hv d19idWcuY2dpP2lkPTIwMDk5MAorICAgICAgICA8cmRhcjovL3Byb2JsZW0vNTQ1NjY0Mzk+CisK KyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgTWFrZSBzdXJl IHdlIGNhbGwgaXNvbGF0ZWRDb3B5KCkgb24gU1FMaXRlSURCQmFja2luZ1N0b3JlOjptX2RhdGFi YXNlUm9vdERpcmVjdG9yeSBiZWZvcmUgdXNpbmcKKyAgICAgICAgaXQgZnJvbSBiYWNrZ3JvdW5k IHRocmVhZHMuCisKKyAgICAgICAgKiBNb2R1bGVzL2luZGV4ZWRkYi9zZXJ2ZXIvU1FMaXRlSURC QmFja2luZ1N0b3JlLmNwcDoKKyAgICAgICAgKFdlYkNvcmU6OklEQlNlcnZlcjo6U1FMaXRlSURC QmFja2luZ1N0b3JlOjpmdWxsRGF0YWJhc2VEaXJlY3RvcnlXaXRoVXBncmFkZSk6CisgICAgICAg IChXZWJDb3JlOjpJREJTZXJ2ZXI6OlNRTGl0ZUlEQkJhY2tpbmdTdG9yZTo6ZGF0YWJhc2VzU2l6 ZUZvck9yaWdpbiBjb25zdCk6CisgICAgICAgIChXZWJDb3JlOjpJREJTZXJ2ZXI6OlNRTGl0ZUlE QkJhY2tpbmdTdG9yZTo6ZGVsZXRlQmFja2luZ1N0b3JlKToKKyAgICAgICAgKiBNb2R1bGVzL2lu ZGV4ZWRkYi9zZXJ2ZXIvU1FMaXRlSURCQmFja2luZ1N0b3JlLmg6CisgICAgICAgIChXZWJDb3Jl OjpJREJTZXJ2ZXI6OlNRTGl0ZUlEQkJhY2tpbmdTdG9yZTo6ZGF0YWJhc2VSb290RGlyZWN0b3J5 IGNvbnN0KToKKwogMjAxOS0wOC0yMSAgSmVyIE5vYmxlICA8amVyLm5vYmxlQGFwcGxlLmNvbT4K IAogICAgICAgICBbaU9TXSBITFMgc3RyZWFtcyBkaXNhcHBlYXIgZnJvbSBOb3cgUGxheWluZyB3 aGVuIHBhdXNlZCBvbiB0aGUgbG9jayBzY3JlZW4KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3Jl L01vZHVsZXMvaW5kZXhlZGRiL3NlcnZlci9TUUxpdGVJREJCYWNraW5nU3RvcmUuY3BwIGIvU291 cmNlL1dlYkNvcmUvTW9kdWxlcy9pbmRleGVkZGIvc2VydmVyL1NRTGl0ZUlEQkJhY2tpbmdTdG9y ZS5jcHAKaW5kZXggYzBlODFkZGE0NjMwMTM5OWI0ZDgxODg5ODEzZmM3NzdjNzE2YjA1Zi4uMDYz NzQxMjNlNTZmODU2YjQ4MTA0MjllMDA4M2YyZDY1MTY0Mjg0MyAxMDA2NDQKLS0tIGEvU291cmNl L1dlYkNvcmUvTW9kdWxlcy9pbmRleGVkZGIvc2VydmVyL1NRTGl0ZUlEQkJhY2tpbmdTdG9yZS5j cHAKKysrIGIvU291cmNlL1dlYkNvcmUvTW9kdWxlcy9pbmRleGVkZGIvc2VydmVyL1NRTGl0ZUlE QkJhY2tpbmdTdG9yZS5jcHAKQEAgLTc2NSw5ICs3NjUsMTAgQEAgU3RyaW5nIFNRTGl0ZUlEQkJh Y2tpbmdTdG9yZTo6ZGF0YWJhc2VOYW1lRnJvbUZpbGUoY29uc3QgU3RyaW5nJiBkYXRhYmFzZVBh dGgpCiAKIFN0cmluZyBTUUxpdGVJREJCYWNraW5nU3RvcmU6OmZ1bGxEYXRhYmFzZURpcmVjdG9y eVdpdGhVcGdyYWRlKCkKIHsKLSAgICBTdHJpbmcgb2xkT3JpZ2luRGlyZWN0b3J5ID0gbV9pZGVu dGlmaWVyLmRhdGFiYXNlRGlyZWN0b3J5UmVsYXRpdmVUb1Jvb3QobV9kYXRhYmFzZVJvb3REaXJl Y3RvcnksICJ2MCIpOworICAgIGF1dG8gZGF0YWJhc2VSb290RGlyZWN0b3J5ID0gdGhpcy0+ZGF0 YWJhc2VSb290RGlyZWN0b3J5KCk7CisgICAgU3RyaW5nIG9sZE9yaWdpbkRpcmVjdG9yeSA9IG1f aWRlbnRpZmllci5kYXRhYmFzZURpcmVjdG9yeVJlbGF0aXZlVG9Sb290KGRhdGFiYXNlUm9vdERp cmVjdG9yeSwgInYwIik7CiAgICAgU3RyaW5nIG9sZERhdGFiYXNlRGlyZWN0b3J5ID0gRmlsZVN5 c3RlbTo6cGF0aEJ5QXBwZW5kaW5nQ29tcG9uZW50KG9sZE9yaWdpbkRpcmVjdG9yeSwgZmlsZW5h bWVGb3JEYXRhYmFzZU5hbWUoKSk7Ci0gICAgU3RyaW5nIG5ld09yaWdpbkRpcmVjdG9yeSA9IG1f aWRlbnRpZmllci5kYXRhYmFzZURpcmVjdG9yeVJlbGF0aXZlVG9Sb290KG1fZGF0YWJhc2VSb290 RGlyZWN0b3J5LCAidjEiKTsKKyAgICBTdHJpbmcgbmV3T3JpZ2luRGlyZWN0b3J5ID0gbV9pZGVu dGlmaWVyLmRhdGFiYXNlRGlyZWN0b3J5UmVsYXRpdmVUb1Jvb3QoZGF0YWJhc2VSb290RGlyZWN0 b3J5LCAidjEiKTsKICAgICBTdHJpbmcgZmlsZU5hbWVIYXNoID0gU1FMaXRlRmlsZVN5c3RlbTo6 Y29tcHV0ZUhhc2hGb3JGaWxlTmFtZShtX2lkZW50aWZpZXIuZGF0YWJhc2VOYW1lKCkpOwogICAg IFZlY3RvcjxTdHJpbmc+IGRpcmVjdG9yaWVzV2l0aFNhbWVIYXNoID0gRmlsZVN5c3RlbTo6bGlz dERpcmVjdG9yeShuZXdPcmlnaW5EaXJlY3RvcnksIGZpbGVOYW1lSGFzaCArICIqIik7CiAgICAg U3RyaW5nIG5ld0RhdGFiYXNlRGlyZWN0b3J5ID0gRmlsZVN5c3RlbTo6cGF0aEJ5QXBwZW5kaW5n Q29tcG9uZW50KG5ld09yaWdpbkRpcmVjdG9yeSwgZmlsZU5hbWVIYXNoKTsKQEAgLTg1Niw4ICs4 NTcsOSBAQCB1aW50NjRfdCBTUUxpdGVJREJCYWNraW5nU3RvcmU6OmRhdGFiYXNlc1NpemVGb3JG b2xkZXIoY29uc3QgU3RyaW5nJiBmb2xkZXIpCiAKIHVpbnQ2NF90IFNRTGl0ZUlEQkJhY2tpbmdT dG9yZTo6ZGF0YWJhc2VzU2l6ZUZvck9yaWdpbigpIGNvbnN0CiB7Ci0gICAgU3RyaW5nIG9sZFZl cnNpb25PcmlnaW5EaXJlY3RvcnkgPSBtX2lkZW50aWZpZXIuZGF0YWJhc2VEaXJlY3RvcnlSZWxh dGl2ZVRvUm9vdChtX2RhdGFiYXNlUm9vdERpcmVjdG9yeSwgInYwIik7Ci0gICAgU3RyaW5nIG5l d1ZlcnNpb25PcmlnaW5EaXJlY3RvcnkgPSBtX2lkZW50aWZpZXIuZGF0YWJhc2VEaXJlY3RvcnlS ZWxhdGl2ZVRvUm9vdChtX2RhdGFiYXNlUm9vdERpcmVjdG9yeSwgInYxIik7CisgICAgYXV0byBk YXRhYmFzZVJvb3REaXJlY3RvcnkgPSB0aGlzLT5kYXRhYmFzZVJvb3REaXJlY3RvcnkoKTsKKyAg ICBTdHJpbmcgb2xkVmVyc2lvbk9yaWdpbkRpcmVjdG9yeSA9IG1faWRlbnRpZmllci5kYXRhYmFz ZURpcmVjdG9yeVJlbGF0aXZlVG9Sb290KGRhdGFiYXNlUm9vdERpcmVjdG9yeSwgInYwIik7Cisg ICAgU3RyaW5nIG5ld1ZlcnNpb25PcmlnaW5EaXJlY3RvcnkgPSBtX2lkZW50aWZpZXIuZGF0YWJh c2VEaXJlY3RvcnlSZWxhdGl2ZVRvUm9vdChkYXRhYmFzZVJvb3REaXJlY3RvcnksICJ2MSIpOwog ICAgIHJldHVybiBkYXRhYmFzZXNTaXplRm9yRm9sZGVyKG9sZFZlcnNpb25PcmlnaW5EaXJlY3Rv cnkpICsgZGF0YWJhc2VzU2l6ZUZvckZvbGRlcihuZXdWZXJzaW9uT3JpZ2luRGlyZWN0b3J5KTsK IH0KIApAQCAtMjU2MCw3ICsyNTYyLDcgQEAgdm9pZCBTUUxpdGVJREJCYWNraW5nU3RvcmU6OmRl bGV0ZUJhY2tpbmdTdG9yZSgpCiAKICAgICBTUUxpdGVGaWxlU3lzdGVtOjpkZWxldGVEYXRhYmFz ZUZpbGUoZGJGaWxlbmFtZSk7CiAgICAgU1FMaXRlRmlsZVN5c3RlbTo6ZGVsZXRlRW1wdHlEYXRh YmFzZURpcmVjdG9yeShtX2RhdGFiYXNlRGlyZWN0b3J5KTsKLSAgICBTUUxpdGVGaWxlU3lzdGVt OjpkZWxldGVFbXB0eURhdGFiYXNlRGlyZWN0b3J5KG1faWRlbnRpZmllci5kYXRhYmFzZURpcmVj dG9yeVJlbGF0aXZlVG9Sb290KG1fZGF0YWJhc2VSb290RGlyZWN0b3J5KSk7CisgICAgU1FMaXRl RmlsZVN5c3RlbTo6ZGVsZXRlRW1wdHlEYXRhYmFzZURpcmVjdG9yeShtX2lkZW50aWZpZXIuZGF0 YWJhc2VEaXJlY3RvcnlSZWxhdGl2ZVRvUm9vdChkYXRhYmFzZVJvb3REaXJlY3RvcnkoKSkpOwog fQogCiB2b2lkIFNRTGl0ZUlEQkJhY2tpbmdTdG9yZTo6dW5yZWdpc3RlckN1cnNvcihTUUxpdGVJ REJDdXJzb3ImIGN1cnNvcikKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL01vZHVsZXMvaW5k ZXhlZGRiL3NlcnZlci9TUUxpdGVJREJCYWNraW5nU3RvcmUuaCBiL1NvdXJjZS9XZWJDb3JlL01v ZHVsZXMvaW5kZXhlZGRiL3NlcnZlci9TUUxpdGVJREJCYWNraW5nU3RvcmUuaAppbmRleCAxOGY1 OGU0ZjI5YTZhZTE3OTZiM2ExZDQxNDRkNjc2NDFjMTlkZGQyLi5kYjY0NzdlN2ZmM2FjOGQ5MzZj ZjBiNTg3ZWY5YTY4NWVlMTkwZmYzIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9Nb2R1bGVz L2luZGV4ZWRkYi9zZXJ2ZXIvU1FMaXRlSURCQmFja2luZ1N0b3JlLmgKKysrIGIvU291cmNlL1dl YkNvcmUvTW9kdWxlcy9pbmRleGVkZGIvc2VydmVyL1NRTGl0ZUlEQkJhY2tpbmdTdG9yZS5oCkBA IC0xMDgsNiArMTA4LDggQEAgcHJpdmF0ZToKICAgICBTdHJpbmcgZmlsZW5hbWVGb3JEYXRhYmFz ZU5hbWUoKSBjb25zdDsKICAgICBTdHJpbmcgZnVsbERhdGFiYXNlUGF0aCgpIGNvbnN0OwogICAg IFN0cmluZyBmdWxsRGF0YWJhc2VEaXJlY3RvcnlXaXRoVXBncmFkZSgpOworICAgIAorICAgIFN0 cmluZyBkYXRhYmFzZVJvb3REaXJlY3RvcnkoKSBjb25zdCB7IHJldHVybiBtX2RhdGFiYXNlUm9v dERpcmVjdG9yeS5pc29sYXRlZENvcHkoKTsgfQogCiAgICAgYm9vbCBlbnN1cmVWYWxpZFJlY29y ZHNUYWJsZSgpOwogICAgIGJvb2wgZW5zdXJlVmFsaWRJbmRleFJlY29yZHNUYWJsZSgpOwo=