Bug 238892

Summary: WebProcessProxy should not hold WebsiteDataStore alive when there is no page
Product: WebKit Reporter: Sihui Liu <sihui_liu>
Component: New BugsAssignee: 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 Flags
Patch
ews-feeder: commit-queue-
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Sihui Liu 2022-04-06 14:34:16 PDT
...
Comment 1 Sihui Liu 2022-04-06 14:36:36 PDT
Created attachment 456860 [details]
Patch
Comment 2 Sihui Liu 2022-04-06 15:04:29 PDT
Created attachment 456869 [details]
Patch
Comment 3 Sihui Liu 2022-04-06 21:19:42 PDT
Created attachment 456887 [details]
Patch
Comment 4 Sihui Liu 2022-04-08 10:46:49 PDT
Created attachment 457092 [details]
Patch
Comment 5 Sihui Liu 2022-04-08 12:39:08 PDT
Created attachment 457110 [details]
Patch
Comment 6 Sihui Liu 2022-04-08 13:51:59 PDT
Created attachment 457115 [details]
Patch
Comment 7 Sihui Liu 2022-04-12 16:22:37 PDT
Created attachment 457489 [details]
Patch
Comment 8 Radar WebKit Bug Importer 2022-04-13 14:35:14 PDT
<rdar://problem/91715517>
Comment 9 Carlos Garcia Campos 2022-04-19 02:28:13 PDT
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 10 Carlos Garcia Campos 2022-04-19 02:31:19 PDT
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.
Comment 11 Sihui Liu 2022-04-21 10:03:57 PDT
(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.
Comment 12 Sihui Liu 2022-04-21 10:05:20 PDT
Created attachment 458073 [details]
Patch
Comment 13 youenn fablet 2022-04-25 23:11:12 PDT
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.
Comment 14 Sihui Liu 2022-05-05 12:27:39 PDT
(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().
Comment 15 Sihui Liu 2022-05-13 16:09:38 PDT
Created attachment 459325 [details]
Patch
Comment 16 Sihui Liu 2022-05-17 11:18:41 PDT
Created attachment 459509 [details]
Patch
Comment 17 Sihui Liu 2022-05-17 12:54:48 PDT
Created attachment 459511 [details]
Patch
Comment 18 Sihui Liu 2022-05-17 19:19:14 PDT
Created attachment 459522 [details]
Patch for landing
Comment 19 EWS 2022-05-17 20:25:00 PDT
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].