Attachment 372413 Details for Bug 198935 – Patch

[patch] Patch

bug-198935-20190618180311.patch (text/plain), 3.92 KB, created by Sihui Liu on 2019-06-18 18:03:12 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sihui Liu

Created: 2019-06-18 18:03:12 PDT

Size: 3.92 KB

patch

obsolete

>Subversion Revision: 246557
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 85bd4af3e298ca810f5a0e6bb492b16c2025c1ef..498bd8890ddc7540132a08e709e860c1e7b05445 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,24 @@
>+2019-06-18  Sihui Liu  <sihui_liu@apple.com>
>+
>+        Crash at com.apple.WebKit: WebKit::WebsiteDataStore::processPools const
>+        https://bugs.webkit.org/show_bug.cgi?id=198935
>+        <rdar://problem/51549308>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        When WebProcessProxy is in WebProcessCache or is pre-warmed, it does not hold a strong reference of 
>+        WebProcessPool. In this case, we should not store the raw pointer of WebProcessPool and perform websiteDataStore
>+        operations with it.
>+        This patch should fix the crash at dereferencing null pointer of WebProcessPool in 
>+        WebsiteDataStore::processPools, but it is unclear why websiteDataStore comes to observe cached or prewarmed web 
>+        process that should not have web page. The release log may help us find the cause.
>+
>+        * UIProcess/WebProcessProxy.cpp:
>+        (WebKit::WebProcessProxy::processPoolIfExists const):
>+        * UIProcess/WebProcessProxy.h:
>+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
>+        (WebKit::WebsiteDataStore::processPools const):
>+
> 2019-06-18  Youenn Fablet  <youenn@apple.com>
> 
>         StorageManager::removeAllowedSessionStorageNamespaceConnection should make sure its storageNamespaceID is valid
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.cpp b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>index 3a0c12d7e381f7d19eef6e6508b29d6bf0b73e33..73ed98c482db33c1831d3df198c57d4c420cae71 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.cpp
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.cpp
>@@ -1516,6 +1516,15 @@ void WebProcessProxy::decrementSuspendedPageCount()
>         send(Messages::WebProcess::SetHasSuspendedPageProxy(false), 0);
> }
> 
>+WebProcessPool* WebProcessProxy::processPoolIfExists() const
>+{
>+    if (m_isPrewarmed || m_isInProcessCache)
>+        RELEASE_LOG_ERROR(Process, "%p - WebProcessProxy::processPoolIfExists: trying to get WebProcessPool from an inactive WebProcessProxy %i", this, processIdentifier());
>+    else
>+        ASSERT(m_processPool);
>+    return m_processPool.get();
>+}
>+
> WebProcessPool& WebProcessProxy::processPool() const
> {
>     ASSERT(m_processPool);
>diff --git a/Source/WebKit/UIProcess/WebProcessProxy.h b/Source/WebKit/UIProcess/WebProcessProxy.h
>index e790bc6b2b4bb5f129ad58023147ec9814e4470a..ecbc5d8fc4c9d4279fe6a41dbb01441414deab9f 100644
>--- a/Source/WebKit/UIProcess/WebProcessProxy.h
>+++ b/Source/WebKit/UIProcess/WebProcessProxy.h
>@@ -119,6 +119,7 @@ public:
>     void incrementSuspendedPageCount();
>     void decrementSuspendedPageCount();
> 
>+    WebProcessPool* processPoolIfExists() const;
>     WebProcessPool& processPool() const;
> 
>     WebCore::RegistrableDomain registrableDomain() const { return m_registrableDomain.valueOr(WebCore::RegistrableDomain { }); }
>diff --git a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>index 3db893c976750d4d554fc91380f25ec8a7fcece4..4c637e5abdcec9132934596b8046a2ba51f54a50 100644
>--- a/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>+++ b/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp
>@@ -1717,8 +1717,10 @@ bool WebsiteDataStore::isAssociatedProcessPool(WebProcessPool& processPool) cons
> HashSet<RefPtr<WebProcessPool>> WebsiteDataStore::processPools(size_t count, bool ensureAPoolExists) const
> {
>     HashSet<RefPtr<WebProcessPool>> processPools;
>-    for (auto& process : processes())
>-        processPools.add(&process->processPool());
>+    for (auto& process : processes()) {
>+        if (auto* processPool = process->processPoolIfExists())
>+            processPools.add(processPool);
>+    }
> 
>     if (processPools.isEmpty()) {
>         // Check if we're one of the legacy data stores.

Actions: View | Formatted Diff | Diff

Attachments on bug 198935: 372279 | 372413