⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 236464 in webkit


Ignore:
Timestamp:
Sep 25, 2018, 11:31:21 AM (8 years ago)
Author:
Chris Dumez
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:
Location:
trunk/Source/WebKit
Files:
10 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebKit/ChangeLog

    r236463 r236464  
     12018-09-25  Chris Dumez  <cdumez@apple.com>
     2
     3        Extending the lifetime of a NetworkProcessProxy / StorageProcessProxy may cause it to have a stale WebProcessPool pointer
     4        https://bugs.webkit.org/show_bug.cgi?id=189851
     5        <rdar://problem/44696263>
     6
     7        Reviewed by Alex Christensen.
     8
     9        Extending the lifetime of a NetworkProcessProxy / StorageProcessProxy may cause it to have a stale WebProcessPool pointer:
     10        - NetworkProcessProxy::m_processPool
     11        - StorageProcessProxy::m_processPool
     12
     13        Those data members are C++ references because it is expected that the WebProcessPool owns the NetworkProcessProxy and
     14        StorageProcessProxy. However, since NetworkProcessProxy / StorageProcessProxy are refcounted, it has happened that code
     15        extends the lifetime of those past their process pool, leading to stale prrocess pool usage. The fix for these crashes
     16        so far as been to ref the WebProcessPool instead of the NetworkProcessProxy / StorageProcessProxy. However, it is very
     17        tempting for people to simply ref the NetworkProcessProxy / StorageProcessProxy given that they are refcounted.
     18        For this reason, this patch updates NetworkProcessProxy / StorageProcessProxy so that they are no longer RefCounted
     19        and so that the WebProcessPool truly owns them via std::unique_ptr<>.
     20
     21        * UIProcess/ChildProcessProxy.h:
     22        * UIProcess/Network/NetworkProcessProxy.cpp:
     23        (WebKit::NetworkProcessProxy::networkProcessCrashed):
     24        (WebKit::NetworkProcessProxy::didClose):
     25        (WebKit::NetworkProcessProxy::create): Deleted.
     26        * UIProcess/Network/NetworkProcessProxy.h:
     27        (WebKit::NetworkProcessProxy::throttler): Deleted.
     28        (WebKit::NetworkProcessProxy::processPool): Deleted.
     29        * UIProcess/Plugins/PluginProcessProxy.h:
     30        (WebKit::PluginProcessProxy::pluginProcessAttributes const): Deleted.
     31        (WebKit::PluginProcessProxy::pluginProcessToken const): Deleted.
     32        (WebKit::PluginProcessProxy::isValid const): Deleted.
     33        * UIProcess/Storage/StorageProcessProxy.cpp:
     34        (WebKit::StorageProcessProxy::create): Deleted.
     35        * UIProcess/Storage/StorageProcessProxy.h:
     36        * UIProcess/WebProcessPool.cpp:
     37        (WebKit::WebProcessPool::ensureNetworkProcess):
     38        (WebKit::WebProcessPool::ensureStorageProcessAndWebsiteDataStore):
     39        (WebKit::WebProcessPool::establishWorkerContextConnectionToStorageProcess):
     40        * UIProcess/WebProcessPool.h:
     41        * UIProcess/WebProcessProxy.h:
     42
    1432018-09-25  Alex Christensen  <achristensen@webkit.org>
    244

  • trunk/Source/WebKit/UIProcess/ChildProcessProxy.h

    r236425 r236464  
    3737namespace WebKit {
    3838
    39 class ChildProcessProxy : ProcessLauncher::Client, public IPC::Connection::Client, public ThreadSafeRefCounted<ChildProcessProxy> {
     39class ChildProcessProxy : ProcessLauncher::Client, public IPC::Connection::Client {
    4040    WTF_MAKE_NONCOPYABLE(ChildProcessProxy);
    4141

  • trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp

    r236425 r236464  
    6767}
    6868
    69 Ref<NetworkProcessProxy> NetworkProcessProxy::create(WebProcessPool& processPool)
    70 {
    71     return adoptRef(*new NetworkProcessProxy(processPool));
    72 }
    73 
    7469NetworkProcessProxy::NetworkProcessProxy(WebProcessPool& processPool)
    7570    : ChildProcessProxy(processPool.alwaysRunsAtBackgroundPriority())
    … …  
    206201        pendingReplies.append(WTFMove(reply));
    207202
    208     // Tell the network process manager to forget about this network process proxy. This may cause us to be deleted.
     203    // Tell the network process manager to forget about this network process proxy. This will cause us to be deleted.
    209204    m_processPool.networkProcessCrashed(*this, WTFMove(pendingReplies));
    210205}
    … …  
    274269    m_updateBlockCookiesCallbackMap.clear();
    275270   
    276     // This may cause us to be deleted.
     271    // This will cause us to be deleted.
    277272    networkProcessCrashed();
    278273}

  • trunk/Source/WebKit/UIProcess/Network/NetworkProcessProxy.h

    r236425 r236464  
    6565struct WebsiteData;
    6666
    67 class NetworkProcessProxy : public ChildProcessProxy, private ProcessThrottlerClient {
     67class NetworkProcessProxy final : public ChildProcessProxy, private ProcessThrottlerClient {
    6868public:
    69     static Ref<NetworkProcessProxy> create(WebProcessPool&);
     69    explicit NetworkProcessProxy(WebProcessPool&);
    7070    ~NetworkProcessProxy();
    7171
    … …  
    111111
    112112private:
    113     NetworkProcessProxy(WebProcessPool&);
    114 
    115113    // ChildProcessProxy
    116114    void getLaunchOptions(ProcessLauncher::LaunchOptions&) override;

  • trunk/Source/WebKit/UIProcess/Plugins/PluginProcessProxy.h

    r236425 r236464  
    6767#endif
    6868
    69 class PluginProcessProxy : public ChildProcessProxy {
     69class PluginProcessProxy final : public ChildProcessProxy, public ThreadSafeRefCounted<PluginProcessProxy> {
    7070public:
    7171    static Ref<PluginProcessProxy> create(PluginProcessManager*, const PluginProcessAttributes&, uint64_t pluginProcessToken);

  • trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.cpp

    r236425 r236464  
    4545}
    4646
    47 Ref<StorageProcessProxy> StorageProcessProxy::create(WebProcessPool& processPool)
    48 {
    49     return adoptRef(*new StorageProcessProxy(processPool));
    50 }
    51 
    5247StorageProcessProxy::StorageProcessProxy(WebProcessPool& processPool)
    5348    : ChildProcessProxy(processPool.alwaysRunsAtBackgroundPriority())

  • trunk/Source/WebKit/UIProcess/Storage/StorageProcessProxy.h

    r236425 r236464  
    4848struct WebsiteData;
    4949
    50 class StorageProcessProxy : public ChildProcessProxy {
     50class StorageProcessProxy final : public ChildProcessProxy {
    5151public:
    52     static Ref<StorageProcessProxy> create(WebProcessPool&);
     52    explicit StorageProcessProxy(WebProcessPool&);
    5353    ~StorageProcessProxy();
    5454
    … …  
    6262
    6363private:
    64     StorageProcessProxy(WebProcessPool&);
    65 
    6664    // ChildProcessProxy
    6765    void getLaunchOptions(ProcessLauncher::LaunchOptions&) override;

  • trunk/Source/WebKit/UIProcess/WebProcessPool.cpp

    r236425 r236464  
    472472    }
    473473
    474     m_networkProcess = NetworkProcessProxy::create(*this);
     474    m_networkProcess = std::make_unique<NetworkProcessProxy>(*this);
    475475
    476476    NetworkProcessCreationParameters parameters;
    … …  
    613613#endif
    614614
    615         m_storageProcess = StorageProcessProxy::create(*this);
     615        m_storageProcess = std::make_unique<StorageProcessProxy>(*this);
    616616        m_storageProcess->send(Messages::StorageProcess::InitializeWebsiteDataStore(parameters), 0);
    617617    }
    … …  
    649649void WebProcessPool::establishWorkerContextConnectionToStorageProcess(StorageProcessProxy& proxy, SecurityOriginData&& securityOrigin, std::optional<PAL::SessionID> sessionID)
    650650{
    651     ASSERT_UNUSED(proxy, &proxy == m_storageProcess);
     651    ASSERT_UNUSED(proxy, &proxy == m_storageProcess.get());
    652652
    653653    if (m_serviceWorkerProcesses.contains(securityOrigin))

  • trunk/Source/WebKit/UIProcess/WebProcessPool.h

    r236425 r236464  
    627627    bool m_canHandleHTTPSServerTrustEvaluation { true };
    628628    bool m_didNetworkProcessCrash { false };
    629     RefPtr<NetworkProcessProxy> m_networkProcess;
    630     RefPtr<StorageProcessProxy> m_storageProcess;
     629    std::unique_ptr<NetworkProcessProxy> m_networkProcess;
     630    std::unique_ptr<StorageProcessProxy> m_storageProcess;
    631631
    632632    HashMap<uint64_t, RefPtr<DictionaryCallback>> m_dictionaryCallbacks;

  • trunk/Source/WebKit/UIProcess/WebProcessProxy.h

    r236425 r236464  
    9292#endif
    9393
    94 class WebProcessProxy : public ChildProcessProxy, public ResponsivenessTimer::Client, private ProcessThrottlerClient {
     94class WebProcessProxy : public ChildProcessProxy, public ResponsivenessTimer::Client, public ThreadSafeRefCounted<WebProcessProxy>, private ProcessThrottlerClient {
    9595public:
    9696    typedef HashMap<uint64_t, RefPtr<WebFrameProxy>> WebFrameProxyMap;
Note: See TracChangeset for help on using the changeset viewer.