WebProcessProxy should not hold WebsiteDataStore alive when there is no page

[Sihui Liu]

...

[Sihui Liu]

Created attachment 456860 [details]
Patch

[Sihui Liu]

Created attachment 456869 [details]
Patch

[Sihui Liu]

Created attachment 456887 [details]
Patch

[Sihui Liu]

Created attachment 457092 [details]
Patch

[Sihui Liu]

Created attachment 457110 [details]
Patch

[Sihui Liu]

Created attachment 457115 [details]
Patch

[Sihui Liu]

Created attachment 457489 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/91715517>

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

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.

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 458073 [details]
Patch

[youenn fablet]

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.

[Sihui Liu]

(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().

[Sihui Liu]

Created attachment 459325 [details]
Patch

[Sihui Liu]

Created attachment 459509 [details]
Patch

[Sihui Liu]

Created attachment 459511 [details]
Patch

[Sihui Liu]

Created attachment 459522 [details]
Patch for landing

[EWS]

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].