[webkit-reviews] review granted: [Bug 238892] WebProcessProxy should not hold WebsiteDataStore alive when there is no page : [Attachment 458073] Patch

[bugzilla-daemon at webkit.org]

youenn fablet <youennf at gmail.com> has granted Sihui Liu <sihui_liu at apple.com>'s
request for review:
Bug 238892: WebProcessProxy should not hold WebsiteDataStore alive when there
is no page
https://bugs.webkit.org/show_bug.cgi?id=238892

Attachment 458073: Patch

https://bugs.webkit.org/attachment.cgi?id=458073&action=review




--- Comment #13 from youenn fablet <youennf at gmail.com> ---
Comment on attachment 458073
  --> https://bugs.webkit.org/attachment.cgi?id=458073
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.