Summary: | Crash under StringImpl::~StringImpl() in IDBServer::computeSpaceUsedForOrigin() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||
Component: | WebKit2 | Assignee: | Chris Dumez <cdumez> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | achristensen, alecflett, beidson, commit-queue, ews-watchlist, ggaren, jsbell, sihui_liu, webkit-bug-importer, youennf | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=200990 | ||||||
Attachments: |
|
Description
Chris Dumez
2019-08-21 11:55:13 PDT
Created attachment 376901 [details]
Patch
Comment on attachment 376901 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376901&action=review > Source/WebCore/Modules/indexeddb/server/IDBServer.h:133 > + String databaseDirectoryPath() const { return m_databaseDirectoryPath.isolatedCopy(); } There's nothing obvious about this code location that indicates it's going to be sent to another thread. (In reply to Alex Christensen from comment #3) > Comment on attachment 376901 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=376901&action=review > > > Source/WebCore/Modules/indexeddb/server/IDBServer.h:133 > > + String databaseDirectoryPath() const { return m_databaseDirectoryPath.isolatedCopy(); } > > There's nothing obvious about this code location that indicates it's going > to be sent to another thread. What's the concern here? Performance? I doubt this is hot. Also, this is private and mostly called from background threads. My concern is, "are there other strings that are being passed to other threads with this string?" is a hard question to answer by looking at this change. (In reply to Chris Dumez from comment #4) > (In reply to Alex Christensen from comment #3) > > Comment on attachment 376901 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=376901&action=review > > > > > Source/WebCore/Modules/indexeddb/server/IDBServer.h:133 > > > + String databaseDirectoryPath() const { return m_databaseDirectoryPath.isolatedCopy(); } > > > > There's nothing obvious about this code location that indicates it's going > > to be sent to another thread. > > What's the concern here? Performance? I doubt this is hot. Also, this is > private and mostly called from background threads. Also, we already use this pattern in network cache code at least: String Storage::basePath() const { return m_basePath.isolatedCopy(); } String Storage::versionPath() const { return makeVersionedDirectoryPath(basePath()); } String Storage::recordsPath() const { return m_recordsPath.isolatedCopy(); } (In reply to Alex Christensen from comment #5) > My concern is, "are there other strings that are being passed to other > threads with this string?" is a hard question to answer by looking at this > change. You can see in the bug description the crash trace: auto oldVersionOriginDirectory = IDBDatabaseIdentifier::databaseDirectoryRelativeToRoot(origin.topOrigin, origin.clientOrigin, m_databaseDirectoryPath, "v0"); and then: String versionDirectory = FileSystem::pathByAppendingComponent(rootDirectory, versionString); So issue is either with m_databaseDirectoryPath or "v0". It cannot be "v0" so it must be m_databaseDirectoryPath. origin is properly isolated copied: postDatabaseTask(createCrossThreadTask(*this, &IDBServer::computeSpaceUsedForOrigin, origin)); Comment on attachment 376901 [details] Patch Clearing flags on attachment: 376901 Committed r248969: <https://trac.webkit.org/changeset/248969> All reviewed patches have been landed. Closing bug. |