| Summary: | REGRESSION (r289474): cacheStoragePath is empty in NetworkStorageManager::localOriginStorageManager | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Simon Fraser (smfr) <simon.fraser> | ||||||||||
| Component: | Website Storage | Assignee: | Sihui Liu <sihui_liu> | ||||||||||
| Status: | RESOLVED FIXED | ||||||||||||
| Severity: | Normal | CC: | annulen, cdumez, cgarcia, ews-watchlist, gyuyoung.kim, ryuan.choi, sergio, sihui_liu, simon.fraser, webkit-bug-importer | ||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||
| Version: | Safari Technology Preview | ||||||||||||
| Hardware: | Unspecified | ||||||||||||
| OS: | Unspecified | ||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=234925 | ||||||||||||
| Attachments: |
|
||||||||||||
Created attachment 453551 [details]
Patch
Created attachment 453564 [details]
Patch
Comment on attachment 453564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453564&action=review r=me with comments. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:419 > + RetainPtr<CFStringRef> cfIdentifier = makeString(identifierBase, ".PrivateBrowsing.", createVersion4UUIDString()).createCFString(); Could use auto. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:442 > + if (m_networkSessions.contains(sessionID)) This could be dropped and we could use HashMap::ensure() below to avoid double hash map lookup. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2177 > + if (auto* session = networkSession(sessionID)) BUG: You're now failing to call the completionHandler in the else case. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2282 > +const String& NetworkProcess::uiProcessBundleIdentifier() Given that this is a one liner, I would keep inline in the header. Outside the class but inline in the header: ``` #if !PLATFORM(COCOA) inline const String& NetworkProcess::uiProcessBundleIdentifier() const { return m_uiProcessBundleIdentifier; } #endif ``` > Source/WebKit/NetworkProcess/NetworkProcess.h:303 > + const String& uiProcessBundleIdentifier(); I think this should remain const. > Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp:480 > + , WTFMove(*shouldUseCustomStoragePaths) Please don't use WTFMove() for booleans and integers. > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:258 > +const String& NetworkProcess::uiProcessBundleIdentifier() I think we should keep the getter as const and mark m_uiProcessBundleIdentifier as mutable. It is a typical pattern for lazy initialization. > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:260 > + if (m_uiProcessBundleIdentifier.isEmpty()) Shouldn't this is a isNull() check? > Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:261 > + m_uiProcessBundleIdentifier = String([[NSBundle mainBundle] bundleIdentifier]); Is String() really needed? > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1856 > + FileSystem::makeAllDirectories(directory); Not new in this patch but we really should NOT be blocking the main thread of the UIProcess (or any process for that matter but the UIProcess is probably this worse) on file system operations. If you don't want to fix in this patch, at the very least add a FIXME because we likely need to revisit. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1857 > + FileSystem::excludeFromBackup(directory); ditto. > Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:257 > + const String& resolvedIndexedDBDirectory() const { return m_resolvedConfiguration->indexedDBDatabaseDirectory(); } The lack of consistency in the naming between "resolvedIndexedDBDirectory" and `m_resolvedConfiguration->indexedDBDatabaseDirectory()` is bothering me. Comment on attachment 453564 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=453564&action=review >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:419 >> + RetainPtr<CFStringRef> cfIdentifier = makeString(identifierBase, ".PrivateBrowsing.", createVersion4UUIDString()).createCFString(); > > Could use auto. Sure. >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:442 >> + if (m_networkSessions.contains(sessionID)) > > This could be dropped and we could use HashMap::ensure() below to avoid double hash map lookup. Okay. >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2177 >> + if (auto* session = networkSession(sessionID)) > > BUG: You're now failing to call the completionHandler in the else case. Ooops >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2282 >> +const String& NetworkProcess::uiProcessBundleIdentifier() > > Given that this is a one liner, I would keep inline in the header. Outside the class but inline in the header: > ``` > #if !PLATFORM(COCOA) > inline const String& NetworkProcess::uiProcessBundleIdentifier() const > { > return m_uiProcessBundleIdentifier; > } > #endif > ``` Sure. >> Source/WebKit/NetworkProcess/NetworkProcess.h:303 >> + const String& uiProcessBundleIdentifier(); > > I think this should remain const. We may change its value in its definition on Cocoa platforms >> Source/WebKit/NetworkProcess/NetworkSessionCreationParameters.cpp:480 >> + , WTFMove(*shouldUseCustomStoragePaths) > > Please don't use WTFMove() for booleans and integers. Sure. >> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:258 >> +const String& NetworkProcess::uiProcessBundleIdentifier() > > I think we should keep the getter as const and mark m_uiProcessBundleIdentifier as mutable. It is a typical pattern for lazy initialization. Okay. >> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:260 >> + if (m_uiProcessBundleIdentifier.isEmpty()) > > Shouldn't this is a isNull() check? Will change it back to isNull() >> Source/WebKit/NetworkProcess/cocoa/NetworkProcessCocoa.mm:261 >> + m_uiProcessBundleIdentifier = String([[NSBundle mainBundle] bundleIdentifier]); > > Is String() really needed? Maybe not. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1856 >> + FileSystem::makeAllDirectories(directory); > > Not new in this patch but we really should NOT be blocking the main thread of the UIProcess (or any process for that matter but the UIProcess is probably this worse) on file system operations. > > If you don't want to fix in this patch, at the very least add a FIXME because we likely need to revisit. Sure, will add. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1857 >> + FileSystem::excludeFromBackup(directory); > > ditto. Sure. >> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:257 >> + const String& resolvedIndexedDBDirectory() const { return m_resolvedConfiguration->indexedDBDatabaseDirectory(); } > > The lack of consistency in the naming between "resolvedIndexedDBDirectory" and `m_resolvedConfiguration->indexedDBDatabaseDirectory()` is bothering me. Will change to resolvedIndexedDBDatabaseDirectory (the inconsistency between resolvedIndexedDatabaseDirectory and indexedDBDatabaseDirectory is more annoying to me) (In reply to Sihui Liu from comment #4) > Comment on attachment 453564 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=453564&action=review > > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:419 > >> + RetainPtr<CFStringRef> cfIdentifier = makeString(identifierBase, ".PrivateBrowsing.", createVersion4UUIDString()).createCFString(); > > > > Could use auto. > > Sure. > > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:442 > >> + if (m_networkSessions.contains(sessionID)) > > > > This could be dropped and we could use HashMap::ensure() below to avoid double hash map lookup. > > Okay. > > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2177 > >> + if (auto* session = networkSession(sessionID)) > > > > BUG: You're now failing to call the completionHandler in the else case. > > Ooops > > >> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2282 > >> +const String& NetworkProcess::uiProcessBundleIdentifier() > > > > Given that this is a one liner, I would keep inline in the header. Outside the class but inline in the header: > > ``` > > #if !PLATFORM(COCOA) > > inline const String& NetworkProcess::uiProcessBundleIdentifier() const > > { > > return m_uiProcessBundleIdentifier; > > } > > #endif > > ``` > > Sure. > > >> Source/WebKit/NetworkProcess/NetworkProcess.h:303 > >> + const String& uiProcessBundleIdentifier(); > > > > I think this should remain const. > > We may change its value in its definition on Cocoa platforms Yes and I commented on how to address that. Created attachment 453661 [details]
Patch
Created attachment 453672 [details]
Patch
Committed r290766 (248010@main): <https://commits.webkit.org/248010@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 453672 [details]. |
Whenever I launch MiniBrowser now, I get: ERROR: File failed to delete. Error message: Operation not permitted /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(265) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &) the file is /Users/smfr/Library/Containers/org.webkit.MiniBrowser/Data/Library/Caches/WebKit/CacheStorage/salt The backtrace is: * frame #0: 0x000000012d60e324 JavaScriptCore`WTF::FileSystemImpl::deleteFile(path={ length = 98, contents = '/Users/smfr/Library/Containers/org.webkit.MiniBrowser/Data/Library/Caches/WebKit/CacheStorage/salt' }) at FileSystemPOSIX.cpp:267:12 frame #1: 0x000000012d60265d JavaScriptCore`WTF::FileSystemImpl::readOrMakeSalt(path={ length = 98, contents = '/Users/smfr/Library/Containers/org.webkit.MiniBrowser/Data/Library/Caches/WebKit/CacheStorage/salt' }) at FileSystem.cpp:501:9 frame #2: 0x000000011a6bd755 WebKit`WebKit::CacheStorage::Engine::storagePath(rootDirectory={ length = 93, contents = '/Users/smfr/Library/Containers/org.webkit.MiniBrowser/Data/Library/Caches/WebKit/CacheStorage' }, origin=0x0000700002c542d0) at CacheStorageEngine.cpp:215:17 frame #3: 0x000000011a82ba72 WebKit`WebKit::NetworkStorageManager::localOriginStorageManager(this=0x0000700002c53ec0)::$_3::operator()() const at NetworkStorageManager.cpp:240:33 frame #4: 0x000000011a82b71a WebKit`void WTF::HashMapEnsureTranslator<WTF::HashMap<WebCore::ClientOrigin, std::__1::unique_ptr<WebKit::OriginStorageManager, std::__1::default_delete<WebKit::OriginStorageManager> >, WTF::DefaultHash<WebCore::ClientOrigin>, WTF::HashTraits<WebCore::ClientOrigin>, WTF::HashTraits<std::__1::unique_ptr<WebKit::OriginStorageManager, std::__1::defau...kStorageManager::localOriginStorageManager(this={ tableSize = 0, keyCount = 0 }, key=0x0000700002c542d0, functor=0x0000700002c53ec0)::$_3>(WebCore::ClientOrigin const&, WebKit::NetworkStorageManager::localOriginStorageManager(WebCore::ClientOrigin const&, WebKit::NetworkStorageManager::ShouldWriteOriginFile)::$_3&&) at HashMap.h:389:28 frame #7: 0x000000011a80dc50 WebKit`WTF::HashTableAddResult<WTF::HashTableIterator<WTF::HashTable<WebCore::ClientOrigin, WTF::KeyValueP...anager(this={ tableSize = 0, keyCount = 0 }, key=0x0000700002c542d0, functor=0x0000700002c53ec0)::$_3>(WebCore::ClientOrigin const&, WebKit::NetworkStorageManager::localOriginStorageManager(WebCore::ClientOrigin const&, WebKit::NetworkStorageManager::ShouldWriteOriginFile)::$_3&&) at HashMap.h:445:12 frame #8: 0x000000011a80dbcc WebKit`WebKit::NetworkStorageManager::localOriginStorageManager(this=0x00000001060d0100, origin=0x0000700002c542d0, shouldWriteOriginFile=No) at NetworkStorageManager.cpp:234:42 frame #9: 0x000000011a812908 WebKit`WebKit::NetworkStorageManager::connectToStorageArea(this=0x00000001060d0100, connection=0x000000010605c340, type=Local, sourceIdentifier=(m_identifier = 11), namespaceIdentifier=(m_identifier = 1), origin=0x0000700002c542d0, completionHandler=0x0000700002c54118)>&&) at NetworkStorageManager.cpp:727:34 frame #10: 0x00000001199ba661 WebKit`void IPC::callMemberFunctionImpl<WebKit::NetworkStorageManager, void (WebKit::NetworkStorageManager::*)(IPC:..._1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul>) at HandleMessage.h:153:5 frame #11: 0x00000001199b5e8b WebKit`void IPC::callMemberFunction<WebKit::NetworkStorageManager, void (WebKit::NetworkStorageManager::*)(IPC::Connection&, WebCore::StorageType, WTF::ObjectIdentifier<WebKit::StorageAreaMapIdentifierType>, WTF::ObjectIdentifier<WebKit::StorageNamespaceIdentifierType>, WebCore::ClientOrigin const&, WTF::CompletionHandler<void (WTF::ObjectIdentifier<WebKit::StorageAreaIdentifierType>, WTF::HashMap<WTF::String, WTF::String, WTF::DefaultHash<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTableTraits>, unsigned long long)>&&), void (WTF::ObjectIdentifier<WebKit::StorageAreaIdentifierType> const&, WTF::HashMap<WTF::String, WTF::String, WTF::DefaultHash<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTableTraits> const&, unsigned long long), std::__1::tuple<WebCore::StorageType, WTF::ObjectIdentifier<WebKit::StorageAreaMapIdentifierType>, WTF::ObjectIdentifier<WebKit::StorageNamespaceIdentifierType>, WebCore::ClientOrigin>, std::__1::integer_sequence<unsigned long, 0ul, 1ul, 2ul, 3ul> >(connection=0x000000010605c340, args=size=4, completionHandler=0x0000700002c542a0, object=0x00000001060d0100, function=40 28 81 1a 01 00 00 00 00 00 00 00 00 00 00 00)>&&, WebKit::NetworkStorageManager*, void (WebKit::NetworkStorageManager::*)(IPC::Connection&, WebCore::StorageType, WTF::ObjectIdentifier<WebKit::StorageAreaMapIdentifierType>, WTF::ObjectIdentifier<WebKit::StorageNamespaceIdentifierType>, WebCore::ClientOrigin const&, WTF::CompletionHandler<void (WTF::ObjectIdentifier<WebKit::StorageAreaIdentifierType>, WTF::HashMap<WTF::String, WTF::String, WTF::DefaultHash<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTableTraits>, unsigned long long)>&&)) at HandleMessage.h:159:5 frame #12: 0x0000000119986096 WebKit`void IPC::handleMessageAsyncWantsConnection<Messages::NetworkStorageManager::ConnectToStorageArea, WebKit::NetworkStorageManager, void (WebKit::NetworkStorageManager::*)(IPC::Connection&, WebCore::StorageType, WTF::ObjectIdentifier<WebKit::StorageAreaMapIdentifierType>, WTF::ObjectIdentifier<WebKit::StorageNamespaceIdentifierType>, WebCore::ClientOrigin const&, WTF::CompletionHandler<void (WTF::ObjectIdentifier<WebKit::StorageAreaIdentifierType>, WTF::HashMap<WTF::String, WTF::String, WTF::DefaultHash<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTableTraits>, unsigned long long)>&&)>(connection=0x000000010605c340, decoder=0x00000001061501d0, object=0x00000001060d0100, function=40 28 81 1a 01 00 00 00 00 00 00 00 00 00 00 00)(IPC::Connection&, WebCore::StorageType, WTF::ObjectIdentifier<WebKit::StorageAreaMapIdentifierType>, WTF::ObjectIdentifier<WebKit::StorageNamespaceIdentifierType>, WebCore::ClientOrigin const&, WTF::CompletionHandler<void (WTF::ObjectIdentifier<WebKit::StorageAreaIdentifierType>, WTF::HashMap<WTF::String, WTF::String, WTF::DefaultHash<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTraits<WTF::String>, WTF::HashTableTraits>, unsigned long long)>&&)) at HandleMessage.h:298:5 frame #13: 0x000000011998377d WebKit`WebKit::NetworkStorageManager::didReceiveMessage(this=0x00000001060d0100, connection=0x000000010605c340, decoder=0x00000001061501d0) at NetworkStorageManagerMessageReceiver.cpp:533:16 frame #14: 0x000000011a8b84c7 WebKit`IPC::Connection::dispatchMessageReceiverMessage(this=0x000000010605c340, messageReceiver=0x00000001060d0100, decoder=IPC::Decoder @ 0x00000001061501d0) at Connection.cpp:391:25 frame #15: 0x000000011a8c134b WebKit`IPC::WorkQueueMessageReceiverQueue::enqueueMessage(this=0x00000001060780c8)::'lambda'()::operator()() at MessageReceiveQueues.h:86:25 frame #16: 0x000000011a8c10e9 WebKit`WTF::Detail::CallableWrapper<IPC::WorkQueueMessageReceiverQueue::enqueueMessage(IPC::Connection&, std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >&&)::'lambda'(), void>::call(this=0x00000001060780c0) at Function.h:53:39 frame #17: 0x000000012d5e8d52 JavaScriptCore`WTF::Function<void ()>::operator(this=0x00000001060780f0)() const at Function.h:82:35 frame #18: 0x000000012d69ddd9 JavaScriptCore`WTF::SuspendableWorkQueue::dispatch(this=0x00000001060780e8)>&&)::$_1::operator()() const at SuspendableWorkQueue.cpp:77:9 frame #19: 0x000000012d69dd09 JavaScriptCore`WTF::Detail::CallableWrapper<WTF::SuspendableWorkQueue::dispatch(WTF::Function<void ()>&&)::$_1, void>::call(this=0x00000001060780e0) at Function.h:53:39 frame #20: 0x000000012d5e8d52 JavaScriptCore`WTF::Function<void ()>::operator(this=0x00000001060dc138)() const at Function.h:82:35 frame #21: 0x000000012d70bd79 JavaScriptCore`WTF::(anonymous namespace)::DispatchWorkItem::operator(this=0x00000001060dc130)() at WorkQueueCocoa.cpp:40:25 frame #22: 0x000000012d70aa2d JavaScriptCore`void WTF::dispatchWorkItem<WTF::(anonymous namespace)::DispatchWorkItem>(dispatchContext=0x00000001060dc130) at WorkQueueCocoa.cpp:48:5