- Revision
- 236464
- Author
- cdu...@apple.com
- Date
- 2018-09-25 11:31:21 -0700 (Tue, 25 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
<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:
Modified Paths
Diff
Modified: trunk/Source/WebKit/ChangeLog (236463 => 236464)
--- trunk/Source/WebKit/ChangeLog 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/ChangeLog 2018-09-25 18:31:21 UTC (rev 236464)
@@ -1,3 +1,45 @@
+2018-09-25 Chris Dumez <cdu...@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 <achristen...@webkit.org>
NetworkLoad::didReceiveResponse should pass its completion handler to its client
Modified: trunk/Source/WebKit/UIProcess/ChildProcessProxy.h (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/ChildProcessProxy.h 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/ChildProcessProxy.h 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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 (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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)
@@ -205,7 +200,7 @@
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 @@
callback();
m_updateBlockCookiesCallbackMap.clear();
- // This may cause us to be deleted.
+ // This will cause us to be deleted.
networkProcessCrashed();
}
Modified: trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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&&);
@@ -110,8 +110,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 (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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 (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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)
Modified: trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.h (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.h 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.h 2018-09-25 18:31:21 UTC (rev 236464)
@@ -47,9 +47,9 @@
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 @@
void terminateForTesting();
private:
- StorageProcessProxy(WebProcessPool&);
-
// ChildProcessProxy
void getLaunchOptions(ProcessLauncher::LaunchOptions&) override;
void processWillShutDown(IPC::Connection&) override;
Modified: trunk/Source/WebKit/UIProcess/WebProcessPool.cpp (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.cpp 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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 (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/WebProcessPool.h 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/WebProcessPool.h 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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 (236463 => 236464)
--- trunk/Source/WebKit/UIProcess/WebProcessProxy.h 2018-09-25 17:36:19 UTC (rev 236463)
+++ trunk/Source/WebKit/UIProcess/WebProcessProxy.h 2018-09-25 18:31:21 UTC (rev 236464)
@@ -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;
