[webkit-changes] [236368] trunk/Source/WebKit

Fri, 21 Sep 2018 16:19:19 -0700

Title: [236368] trunk/Source/WebKit
Revision
236368
Author
[email protected]
Date
2018-09-21 16:18:13 -0700 (Fri, 21 Sep 2018)

Log Message

Extending the lifetime of a NetworkProcessProxy / StorageProcessProxy may cause it to have a stale WebProcessPool pointer
https://bugs.webkit.org/show_bug.cgi?id=189851

Reviewed by Alex Christensen.

Extending the lifetime of a NetworkProcessProxy / StorageProcessProxy may cause it to have a stale WebProcessPool pointer:
- NetworkProcessProxy::m_processPool
- StorageProcessProxy::m_processPool

Those data members are C++ references because it is expected that the WebProcessPool owns the NetworkProcessProxy and
StorageProcessProxy. However, since NetworkProcessProxy / StorageProcessProxy are refcounted, it has happened that code
extends the lifetime of those past their process pool, leading to stale prrocess pool usage. The fix for these crashes
so far as been to ref the WebProcessPool instead of the NetworkProcessProxy / StorageProcessProxy. However, given how
error-prone this is, this patch updates NetworkProcessProxy / StorageProcessProxy so that they forward their refcounting
to the WebProcessPool.

* UIProcess/ChildProcessProxy.h:
* UIProcess/Network/NetworkProcessProxy.cpp:
(WebKit::NetworkProcessProxy::ref):
(WebKit::NetworkProcessProxy::deref):
(WebKit::NetworkProcessProxy::didClose):
(WebKit::NetworkProcessProxy::updatePrevalentDomainsToBlockCookiesFor):
(WebKit::NetworkProcessProxy::create): Deleted.
* UIProcess/Network/NetworkProcessProxy.h:
* UIProcess/Plugins/PluginProcessProxy.h:
* UIProcess/Storage/StorageProcessProxy.cpp:
(WebKit::StorageProcessProxy::ref):
(WebKit::StorageProcessProxy::deref):
(WebKit::StorageProcessProxy::create): Deleted.
* UIProcess/Storage/StorageProcessProxy.h:
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::ensureNetworkProcess):
(WebKit::WebProcessPool::ensureStorageProcessAndWebsiteDataStore):
* UIProcess/WebProcessPool.h:
* UIProcess/WebProcessProxy.h:

Modified Paths

Diff

Modified: trunk/Source/WebKit/ChangeLog (236367 => 236368)


--- trunk/Source/WebKit/ChangeLog	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/ChangeLog	2018-09-21 23:18:13 UTC (rev 236368)
@@ -1,3 +1,41 @@
+2018-09-21  Chris Dumez  <[email protected]>
+
+        Extending the lifetime of a NetworkProcessProxy / StorageProcessProxy may cause it to have a stale WebProcessPool pointer
+        https://bugs.webkit.org/show_bug.cgi?id=189851
+
+        Reviewed by Alex Christensen.
+
+        Extending the lifetime of a NetworkProcessProxy / StorageProcessProxy may cause it to have a stale WebProcessPool pointer:
+        - NetworkProcessProxy::m_processPool
+        - StorageProcessProxy::m_processPool
+
+        Those data members are C++ references because it is expected that the WebProcessPool owns the NetworkProcessProxy and
+        StorageProcessProxy. However, since NetworkProcessProxy / StorageProcessProxy are refcounted, it has happened that code
+        extends the lifetime of those past their process pool, leading to stale prrocess pool usage. The fix for these crashes
+        so far as been to ref the WebProcessPool instead of the NetworkProcessProxy / StorageProcessProxy. However, given how
+        error-prone this is, this patch updates NetworkProcessProxy / StorageProcessProxy so that they forward their refcounting
+        to the WebProcessPool.
+
+        * UIProcess/ChildProcessProxy.h:
+        * UIProcess/Network/NetworkProcessProxy.cpp:
+        (WebKit::NetworkProcessProxy::ref):
+        (WebKit::NetworkProcessProxy::deref):
+        (WebKit::NetworkProcessProxy::didClose):
+        (WebKit::NetworkProcessProxy::updatePrevalentDomainsToBlockCookiesFor):
+        (WebKit::NetworkProcessProxy::create): Deleted.
+        * UIProcess/Network/NetworkProcessProxy.h:
+        * UIProcess/Plugins/PluginProcessProxy.h:
+        * UIProcess/Storage/StorageProcessProxy.cpp:
+        (WebKit::StorageProcessProxy::ref):
+        (WebKit::StorageProcessProxy::deref):
+        (WebKit::StorageProcessProxy::create): Deleted.
+        * UIProcess/Storage/StorageProcessProxy.h:
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::ensureNetworkProcess):
+        (WebKit::WebProcessPool::ensureStorageProcessAndWebsiteDataStore):
+        * UIProcess/WebProcessPool.h:
+        * UIProcess/WebProcessProxy.h:
+
 2018-09-21  Alex Christensen  <[email protected]>
 
         Use a Variant for FormDataElement

Modified: trunk/Source/WebKit/UIProcess/ChildProcessProxy.h (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/ChildProcessProxy.h	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/ChildProcessProxy.h	2018-09-21 23:18:13 UTC (rev 236368)
@@ -36,7 +36,7 @@
 
 namespace WebKit {
 
-class ChildProcessProxy : ProcessLauncher::Client, public IPC::Connection::Client, public ThreadSafeRefCounted<ChildProcessProxy> {
+class ChildProcessProxy : ProcessLauncher::Client, public IPC::Connection::Client {
     WTF_MAKE_NONCOPYABLE(ChildProcessProxy);
 
 protected:

Modified: trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp	2018-09-21 23:18:13 UTC (rev 236368)
@@ -66,11 +66,6 @@
     return ++callbackID;
 }
 
-Ref<NetworkProcessProxy> NetworkProcessProxy::create(WebProcessPool& processPool)
-{
-    return adoptRef(*new NetworkProcessProxy(processPool));
-}
-
 NetworkProcessProxy::NetworkProcessProxy(WebProcessPool& processPool)
     : ChildProcessProxy(processPool.alwaysRunsAtBackgroundPriority())
     , m_processPool(processPool)
@@ -97,6 +92,16 @@
 #endif
 }
 
+void NetworkProcessProxy::ref()
+{
+    m_processPool.ref();
+}
+
+void NetworkProcessProxy::deref()
+{
+    m_processPool.deref();
+}
+
 void NetworkProcessProxy::getLaunchOptions(ProcessLauncher::LaunchOptions& launchOptions)
 {
     launchOptions.processType = ProcessLauncher::ProcessType::Network;
@@ -248,7 +253,7 @@
 
 void NetworkProcessProxy::didClose(IPC::Connection&)
 {
-    auto protectedProcessPool = makeRef(m_processPool);
+    auto protectedThis = makeRef(*this);
 
     if (m_downloadProxyMap)
         m_downloadProxyMap->processDidClose();
@@ -400,7 +405,7 @@
     }
     
     auto callbackId = generateCallbackID();
-    auto addResult = m_updateBlockCookiesCallbackMap.add(callbackId, [protectedProcessPool = makeRef(m_processPool), token = throttler().backgroundActivityToken(), completionHandler = WTFMove(completionHandler)]() mutable {
+    auto addResult = m_updateBlockCookiesCallbackMap.add(callbackId, [protectedThis = makeRef(*this), token = throttler().backgroundActivityToken(), completionHandler = WTFMove(completionHandler)]() mutable {
         completionHandler();
     });
     ASSERT_UNUSED(addResult, addResult.isNewEntry);

Modified: trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h	2018-09-21 23:18:13 UTC (rev 236368)
@@ -64,9 +64,9 @@
 class WebUserContentControllerProxy;
 struct WebsiteData;
 
-class NetworkProcessProxy : public ChildProcessProxy, private ProcessThrottlerClient {
+class NetworkProcessProxy final : public ChildProcessProxy, private ProcessThrottlerClient {
 public:
-    static Ref<NetworkProcessProxy> create(WebProcessPool&);
+    explicit NetworkProcessProxy(WebProcessPool&);
     ~NetworkProcessProxy();
 
     void getNetworkProcessConnection(Messages::WebProcessProxy::GetNetworkProcessConnection::DelayedReply&&);
@@ -87,6 +87,9 @@
     void resetCacheMaxAgeCapForPrevalentResources(PAL::SessionID, CompletionHandler<void()>&&);
 #endif
 
+    void ref();
+    void deref();
+
     void writeBlobToFilePath(const WebCore::URL&, const String& path, CompletionHandler<void(bool)>&& callback);
 
     void processReadyToSuspend();
@@ -110,8 +113,6 @@
     void removeSession(PAL::SessionID);
 
 private:
-    NetworkProcessProxy(WebProcessPool&);
-
     // ChildProcessProxy
     void getLaunchOptions(ProcessLauncher::LaunchOptions&) override;
     void connectionWillOpen(IPC::Connection&) override;

Modified: trunk/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h	2018-09-21 23:18:13 UTC (rev 236368)
@@ -66,7 +66,7 @@
 int pluginProcessThroughputQOS();
 #endif
 
-class PluginProcessProxy : public ChildProcessProxy {
+class PluginProcessProxy final : public ChildProcessProxy, public ThreadSafeRefCounted<PluginProcessProxy> {
 public:
     static Ref<PluginProcessProxy> create(PluginProcessManager*, const PluginProcessAttributes&, uint64_t pluginProcessToken);
     ~PluginProcessProxy();

Modified: trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp	2018-09-21 23:18:13 UTC (rev 236368)
@@ -44,11 +44,6 @@
     return ++callbackID;
 }
 
-Ref<StorageProcessProxy> StorageProcessProxy::create(WebProcessPool& processPool)
-{
-    return adoptRef(*new StorageProcessProxy(processPool));
-}
-
 StorageProcessProxy::StorageProcessProxy(WebProcessPool& processPool)
     : ChildProcessProxy(processPool.alwaysRunsAtBackgroundPriority())
     , m_processPool(processPool)
@@ -57,6 +52,16 @@
     connect();
 }
 
+void StorageProcessProxy::ref()
+{
+    m_processPool.ref();
+}
+
+void StorageProcessProxy::deref()
+{
+    m_processPool.deref();
+}
+
 StorageProcessProxy::~StorageProcessProxy()
 {
     ASSERT(m_pendingFetchWebsiteDataCallbacks.isEmpty());

Modified: trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.h (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.h	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.h	2018-09-21 23:18:13 UTC (rev 236368)
@@ -47,11 +47,14 @@
 enum class WebsiteDataType;
 struct WebsiteData;
 
-class StorageProcessProxy : public ChildProcessProxy {
+class StorageProcessProxy final : public ChildProcessProxy {
 public:
-    static Ref<StorageProcessProxy> create(WebProcessPool&);
+    explicit StorageProcessProxy(WebProcessPool&);
     ~StorageProcessProxy();
 
+    void ref();
+    void deref();
+
     void fetchWebsiteData(PAL::SessionID, OptionSet<WebsiteDataType>, CompletionHandler<void(WebsiteData)>&&);
     void deleteWebsiteData(PAL::SessionID, OptionSet<WebsiteDataType>, WallTime modifiedSince, CompletionHandler<void()>&&);
     void deleteWebsiteDataForOrigins(PAL::SessionID, OptionSet<WebsiteDataType>, const Vector<WebCore::SecurityOriginData>&, CompletionHandler<void()>&&);
@@ -61,8 +64,6 @@
     void terminateForTesting();
 
 private:
-    StorageProcessProxy(WebProcessPool&);
-
     // ChildProcessProxy
     void getLaunchOptions(ProcessLauncher::LaunchOptions&) override;
     void processWillShutDown(IPC::Connection&) override;

Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.cpp (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/WebProcessPool.cpp	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.cpp	2018-09-21 23:18:13 UTC (rev 236368)
@@ -471,7 +471,7 @@
         return *m_networkProcess;
     }
 
-    m_networkProcess = NetworkProcessProxy::create(*this);
+    m_networkProcess = std::make_unique<NetworkProcessProxy>(*this);
 
     NetworkProcessCreationParameters parameters;
 
@@ -612,7 +612,7 @@
         parameters.shouldDisableServiceWorkerProcessTerminationDelay = m_shouldDisableServiceWorkerProcessTerminationDelay;
 #endif
 
-        m_storageProcess = StorageProcessProxy::create(*this);
+        m_storageProcess = std::make_unique<StorageProcessProxy>(*this);
         m_storageProcess->send(Messages::StorageProcess::InitializeWebsiteDataStore(parameters), 0);
     }
 
@@ -648,7 +648,7 @@
 #if ENABLE(SERVICE_WORKER)
 void WebProcessPool::establishWorkerContextConnectionToStorageProcess(StorageProcessProxy& proxy, SecurityOriginData&& securityOrigin, std::optional<PAL::SessionID> sessionID)
 {
-    ASSERT_UNUSED(proxy, &proxy == m_storageProcess);
+    ASSERT_UNUSED(proxy, &proxy == m_storageProcess.get());
 
     if (m_serviceWorkerProcesses.contains(securityOrigin))
         return;

Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.h (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/WebProcessPool.h	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.h	2018-09-21 23:18:13 UTC (rev 236368)
@@ -626,8 +626,8 @@
 
     bool m_canHandleHTTPSServerTrustEvaluation { true };
     bool m_didNetworkProcessCrash { false };
-    RefPtr<NetworkProcessProxy> m_networkProcess;
-    RefPtr<StorageProcessProxy> m_storageProcess;
+    std::unique_ptr<NetworkProcessProxy> m_networkProcess;
+    std::unique_ptr<StorageProcessProxy> m_storageProcess;
 
     HashMap<uint64_t, RefPtr<DictionaryCallback>> m_dictionaryCallbacks;
     HashMap<uint64_t, RefPtr<StatisticsRequest>> m_statisticsRequests;

Modified: trunk/Source/WebKit/UIProcess/WebProcessProxy.h (236367 => 236368)


--- trunk/Source/WebKit/UIProcess/WebProcessProxy.h	2018-09-21 23:06:40 UTC (rev 236367)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.h	2018-09-21 23:18:13 UTC (rev 236368)
@@ -91,7 +91,7 @@
 typedef BackgroundWebProcessCounter::Token BackgroundWebProcessToken;
 #endif
 
-class WebProcessProxy : public ChildProcessProxy, public ResponsivenessTimer::Client, private ProcessThrottlerClient {
+class WebProcessProxy : public ChildProcessProxy, public ResponsivenessTimer::Client, public ThreadSafeRefCounted<WebProcessProxy>, private ProcessThrottlerClient {
 public:
     typedef HashMap<uint64_t, RefPtr<WebFrameProxy>> WebFrameProxyMap;
     typedef HashMap<uint64_t, WebPageProxy*> WebPageProxyMap;

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to