Summary: | WebProcessProxy should not hold WebsiteDataStore alive when there is no page | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Sihui Liu <sihui_liu> | ||||||||||||||||||||||||||
Component: | New Bugs | Assignee: | Sihui Liu <sihui_liu> | ||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, cdumez, cgarcia, mcatanzaro, pgriffis, webkit-bug-importer, youennf | ||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||
Bug Depends on: | |||||||||||||||||||||||||||||
Bug Blocks: | 241353 | ||||||||||||||||||||||||||||
Attachments: |
|
Description
Sihui Liu
2022-04-06 14:34:16 PDT
Created attachment 456860 [details]
Patch
Created attachment 456869 [details]
Patch
Created attachment 456887 [details]
Patch
Created attachment 457092 [details]
Patch
Created attachment 457110 [details]
Patch
Created attachment 457115 [details]
Patch
Created attachment 457489 [details]
Patch
The problem is a crash when trying to add a web process to the cache in WebProcessCache::CachedProcess::CachedProcess(Ref<WebProcessProxy>&& process) because we hit RELEASE_ASSERT(dataStore). In current code, the process proxy keeps a reference of the data store, so when added to the cache it still has the data sore, but now the data store is destroyed before the process is added to the cache. This happens in the tests because every test case is creating a new web context (WebProcessPool) with a new website data store. When the test case finishes the web context is destroyed and the data store too. Since WebPageProxy::close() sends the close message in the following run loop iteration, WebProcessProxy::maybeShutDown() (which adds the process to the cache) is called when the new data store has already been removed. We should make WebProcessCache::canCacheProcess() return false if the process session ID doesn't have a valid data store. Comment on attachment 457489 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=457489&action=review > Source/WebKit/UIProcess/WebProcessCache.cpp:83 > - auto sessionID = process.websiteDataStore().sessionID(); > - if (sessionID.isEphemeral() && !process.processPool().hasPagesUsingWebsiteDataStore(process.websiteDataStore())) { > + auto sessionID = process.sessionID(); > + if (sessionID.isEphemeral() && !process.processPool().hasPagesUsingWebsiteDataStore(process.sessionID())) { > WEBPROCESSCACHE_RELEASE_LOG("canCacheProcess: Not caching process because this session has been destroyed", process.processIdentifier()); > return false; > } Why are we doing this only for ephemeral sessions? With this patch I think we can do: if (!WebsiteDataStore::existingDataStoreForSessionID(process.sessionID()) if there isn't a data store for the process session ID is because the session has been destroyed. (In reply to Carlos Garcia Campos from comment #10) > Comment on attachment 457489 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=457489&action=review > > > Source/WebKit/UIProcess/WebProcessCache.cpp:83 > > - auto sessionID = process.websiteDataStore().sessionID(); > > - if (sessionID.isEphemeral() && !process.processPool().hasPagesUsingWebsiteDataStore(process.websiteDataStore())) { > > + auto sessionID = process.sessionID(); > > + if (sessionID.isEphemeral() && !process.processPool().hasPagesUsingWebsiteDataStore(process.sessionID())) { > > WEBPROCESSCACHE_RELEASE_LOG("canCacheProcess: Not caching process because this session has been destroyed", process.processIdentifier()); > > return false; > > } > > Why are we doing this only for ephemeral sessions? With this patch I think > we can do: > > if (!WebsiteDataStore::existingDataStoreForSessionID(process.sessionID()) > > if there isn't a data store for the process session ID is because the > session has been destroyed. Thanks for looking into this! I will update the patch as suggested. Created attachment 458073 [details]
Patch
Comment on attachment 458073 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=458073&action=review > Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:119 > + if (m_process->sessionID() != m_page.websiteDataStore().sessionID()) So we could now have a WebProcessProxy with a session ID pointing to nothing. Should we proactively destroy WebProcessProxies in case its session data store is destroyed? > Source/WebKit/UIProcess/WebProcessProxy.cpp:1752 > + return m_sessionID; We could probably add an ASSERT(m_sessionID.isValid()). Previously we would return m_websiteDataStore which might cause a nullptr crash. If we return an invalid session ID and query the website data store map, we will corrupt the map. > Source/WebKit/UIProcess/WebProcessProxy.h:-170 > - WebsiteDataStore& websiteDataStore() const { ASSERT(m_websiteDataStore); return *m_websiteDataStore; } WebProcessProxy could continue providing a WebsiteDataStore* getter() which would call WebsiteDataStore::existingDataStoreForSessionID under the hood. If we later think we should use a WeakPtr<WebsiteDataStore> instead, that will make the code easier to migrate. (In reply to youenn fablet from comment #13) > Comment on attachment 458073 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=458073&action=review > > > Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:119 > > + if (m_process->sessionID() != m_page.websiteDataStore().sessionID()) > > So we could now have a WebProcessProxy with a session ID pointing to nothing. > Should we proactively destroy WebProcessProxies in case its session data > store is destroyed? I think WebProcessProxy may have an invalid sessionID if it's pre-warmed; so we cannot say WebProcessProxy with invalid session is invalid. > > > Source/WebKit/UIProcess/WebProcessProxy.cpp:1752 > > + return m_sessionID; > > We could probably add an ASSERT(m_sessionID.isValid()). > Previously we would return m_websiteDataStore which might cause a nullptr > crash. > If we return an invalid session ID and query the website data store map, we > will corrupt the map. > > > Source/WebKit/UIProcess/WebProcessProxy.h:-170 > > - WebsiteDataStore& websiteDataStore() const { ASSERT(m_websiteDataStore); return *m_websiteDataStore; } > > WebProcessProxy could continue providing a WebsiteDataStore* getter() which > would call WebsiteDataStore::existingDataStoreForSessionID under the hood. > If we later think we should use a WeakPtr<WebsiteDataStore> instead, that > will make the code easier to migrate. Sounds good; and I can add the SessionID validity check inside websiteDataStore(). Created attachment 459325 [details]
Patch
Created attachment 459509 [details]
Patch
Created attachment 459511 [details]
Patch
Created attachment 459522 [details]
Patch for landing
Committed r294381 (250678@main): <https://commits.webkit.org/250678@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 459522 [details]. |