Extending the lifetime of a NetworkProcessProxy / StorageProcessProxy may cause it...
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Sep 2018 18:31:21 +0000 (18:31 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 25 Sep 2018 18:31:21 +0000 (18:31 +0000)
https://bugs.webkit.org/show_bug.cgi?id=189851
<rdar://problem/44696263>

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, it is very
tempting for people to simply ref the NetworkProcessProxy / StorageProcessProxy given that they are refcounted.
For this reason, this patch updates NetworkProcessProxy / StorageProcessProxy so that they are no longer RefCounted
and so that the WebProcessPool truly owns them via std::unique_ptr<>.

* UIProcess/ChildProcessProxy.h:
* UIProcess/Network/NetworkProcessProxy.cpp:
(WebKit::NetworkProcessProxy::networkProcessCrashed):
(WebKit::NetworkProcessProxy::didClose):
(WebKit::NetworkProcessProxy::create): Deleted.
* UIProcess/Network/NetworkProcessProxy.h:
(WebKit::NetworkProcessProxy::throttler): Deleted.
(WebKit::NetworkProcessProxy::processPool): Deleted.
* UIProcess/Plugins/PluginProcessProxy.h:
(WebKit::PluginProcessProxy::pluginProcessAttributes const): Deleted.
(WebKit::PluginProcessProxy::pluginProcessToken const): Deleted.
(WebKit::PluginProcessProxy::isValid const): Deleted.
* UIProcess/Storage/StorageProcessProxy.cpp:
(WebKit::StorageProcessProxy::create): Deleted.
* UIProcess/Storage/StorageProcessProxy.h:
* UIProcess/WebProcessPool.cpp:
(WebKit::WebProcessPool::ensureNetworkProcess):
(WebKit::WebProcessPool::ensureStorageProcessAndWebsiteDataStore):
(WebKit::WebProcessPool::establishWorkerContextConnectionToStorageProcess):
* UIProcess/WebProcessPool.h:
* UIProcess/WebProcessProxy.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@236464 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/ChildProcessProxy.h
Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp
Source/WebKit/UIProcess/Network/NetworkProcessProxy.h
Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h
Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp
Source/WebKit/UIProcess/Storage/StorageProcessProxy.h
Source/WebKit/UIProcess/WebProcessPool.cpp
Source/WebKit/UIProcess/WebProcessPool.h
Source/WebKit/UIProcess/WebProcessProxy.h

index 6a1375f..ddf0d18 100644 (file)
@@ -1,3 +1,45 @@
+2018-09-25  Chris Dumez  <cdumez@apple.com>
+
+        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
+        <rdar://problem/44696263>
+
+        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, it is very
+        tempting for people to simply ref the NetworkProcessProxy / StorageProcessProxy given that they are refcounted.
+        For this reason, this patch updates NetworkProcessProxy / StorageProcessProxy so that they are no longer RefCounted
+        and so that the WebProcessPool truly owns them via std::unique_ptr<>.
+
+        * UIProcess/ChildProcessProxy.h:
+        * UIProcess/Network/NetworkProcessProxy.cpp:
+        (WebKit::NetworkProcessProxy::networkProcessCrashed):
+        (WebKit::NetworkProcessProxy::didClose):
+        (WebKit::NetworkProcessProxy::create): Deleted.
+        * UIProcess/Network/NetworkProcessProxy.h:
+        (WebKit::NetworkProcessProxy::throttler): Deleted.
+        (WebKit::NetworkProcessProxy::processPool): Deleted.
+        * UIProcess/Plugins/PluginProcessProxy.h:
+        (WebKit::PluginProcessProxy::pluginProcessAttributes const): Deleted.
+        (WebKit::PluginProcessProxy::pluginProcessToken const): Deleted.
+        (WebKit::PluginProcessProxy::isValid const): Deleted.
+        * UIProcess/Storage/StorageProcessProxy.cpp:
+        (WebKit::StorageProcessProxy::create): Deleted.
+        * UIProcess/Storage/StorageProcessProxy.h:
+        * UIProcess/WebProcessPool.cpp:
+        (WebKit::WebProcessPool::ensureNetworkProcess):
+        (WebKit::WebProcessPool::ensureStorageProcessAndWebsiteDataStore):
+        (WebKit::WebProcessPool::establishWorkerContextConnectionToStorageProcess):
+        * UIProcess/WebProcessPool.h:
+        * UIProcess/WebProcessProxy.h:
+
 2018-09-25  Alex Christensen  <achristensen@webkit.org>
 
         NetworkLoad::didReceiveResponse should pass its completion handler to its client
index 2d5f15c..c891279 100644 (file)
@@ -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:
index 810daf9..0f1b6c7 100644 (file)
@@ -66,11 +66,6 @@ static uint64_t generateCallbackID()
     return ++callbackID;
 }
 
-Ref<NetworkProcessProxy> NetworkProcessProxy::create(WebProcessPool& processPool)
-{
-    return adoptRef(*new NetworkProcessProxy(processPool));
-}
-
 NetworkProcessProxy::NetworkProcessProxy(WebProcessPool& processPool)
     : ChildProcessProxy(processPool.alwaysRunsAtBackgroundPriority())
     , m_processPool(processPool)
@@ -205,7 +200,7 @@ void NetworkProcessProxy::networkProcessCrashed()
     for (auto& reply : m_pendingConnectionReplies)
         pendingReplies.append(WTFMove(reply));
 
-    // Tell the network process manager to forget about this network process proxy. This may cause us to be deleted.
+    // Tell the network process manager to forget about this network process proxy. This will cause us to be deleted.
     m_processPool.networkProcessCrashed(*this, WTFMove(pendingReplies));
 }
 
@@ -273,7 +268,7 @@ void NetworkProcessProxy::didClose(IPC::Connection&)
         callback();
     m_updateBlockCookiesCallbackMap.clear();
     
-    // This may cause us to be deleted.
+    // This will cause us to be deleted.
     networkProcessCrashed();
 }
 
index 6f9648d..db2c8ed 100644 (file)
@@ -64,9 +64,9 @@ struct NetworkProcessCreationParameters;
 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&&);
@@ -110,8 +110,6 @@ public:
     void removeSession(PAL::SessionID);
 
 private:
-    NetworkProcessProxy(WebProcessPool&);
-
     // ChildProcessProxy
     void getLaunchOptions(ProcessLauncher::LaunchOptions&) override;
     void connectionWillOpen(IPC::Connection&) override;
index 09600b1..386af00 100644 (file)
@@ -66,7 +66,7 @@ int pluginProcessLatencyQOS();
 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();
index 8bc9a74..ee4bf9d 100644 (file)
@@ -44,11 +44,6 @@ static uint64_t generateStorageProcessCallbackID()
     return ++callbackID;
 }
 
-Ref<StorageProcessProxy> StorageProcessProxy::create(WebProcessPool& processPool)
-{
-    return adoptRef(*new StorageProcessProxy(processPool));
-}
-
 StorageProcessProxy::StorageProcessProxy(WebProcessPool& processPool)
     : ChildProcessProxy(processPool.alwaysRunsAtBackgroundPriority())
     , m_processPool(processPool)
index a266c77..dfe1f29 100644 (file)
@@ -47,9 +47,9 @@ class WebProcessProxy;
 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 fetchWebsiteData(PAL::SessionID, OptionSet<WebsiteDataType>, CompletionHandler<void(WebsiteData)>&&);
@@ -61,8 +61,6 @@ public:
     void terminateForTesting();
 
 private:
-    StorageProcessProxy(WebProcessPool&);
-
     // ChildProcessProxy
     void getLaunchOptions(ProcessLauncher::LaunchOptions&) override;
     void processWillShutDown(IPC::Connection&) override;
index f880690..9927cc9 100644 (file)
@@ -471,7 +471,7 @@ NetworkProcessProxy& WebProcessPool::ensureNetworkProcess(WebsiteDataStore* with
         return *m_networkProcess;
     }
 
-    m_networkProcess = NetworkProcessProxy::create(*this);
+    m_networkProcess = std::make_unique<NetworkProcessProxy>(*this);
 
     NetworkProcessCreationParameters parameters;
 
@@ -612,7 +612,7 @@ void WebProcessPool::ensureStorageProcessAndWebsiteDataStore(WebsiteDataStore* r
         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 @@ void WebProcessPool::storageProcessCrashed(StorageProcessProxy* storageProcessPr
 #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;
index 3935184..dd45b51 100644 (file)
@@ -626,8 +626,8 @@ private:
 
     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;
index 7d8085b..f7bc733 100644 (file)
@@ -91,7 +91,7 @@ typedef RefCounter<BackgroundWebProcessCounterType> BackgroundWebProcessCounter;
 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;