Summary: | Crash under StringImpl::endsWith() in SQLiteIDBBackingStore::fullDatabaseDirectoryWithUpgrade() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | WebKit2 | Assignee: | 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
Chris Dumez
2019-08-21 12:16:24 PDT
Created attachment 376903 [details]
Patch
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 on attachment 376903 [details] Patch Clearing flags on attachment: 376903 Committed r248971: <https://trac.webkit.org/changeset/248971> All reviewed patches have been landed. Closing bug. 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 agree it's not the best and expressed this in https://bugs.webkit.org/show_bug.cgi?id=200989#c3 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. Was there an answer to this question offline at all? I'm also not sure what our coding style is in these cases. Not that I am aware of. (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? (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. |