WebSocket: Stop Bridge::disconnect() from waiting until Peer::disconnect() is complete

third_party/WebKit/Source/modules/websockets/WorkerWebSocketChannel.cpp

 /*
  * Copyright (C) 2011, 2012 Google Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 40 matching lines @@
 #include <memory>
 
 namespace blink {
 
 typedef WorkerWebSocketChannel::Bridge Bridge;
 typedef WorkerWebSocketChannel::Peer Peer;
 
 // Created and destroyed on the worker thread. All setters of this class are
 // called on the main thread, while all getters are called on the worker
 // thread. signalWorkerThread() must be called before any getters are called.
-class WebSocketChannelSyncHelper : public GarbageCollectedFinalized<WebSocketChannelSyncHelper> {
+class WebSocketChannelSyncHelper {
 public:
-    static WebSocketChannelSyncHelper* create(std::unique_ptr<WaitableEvent> event)
-    {
-        return new WebSocketChannelSyncHelper(std::move(event));
-    }
-
-    ~WebSocketChannelSyncHelper()
-    {
-    }
+    WebSocketChannelSyncHelper() {}
+    ~WebSocketChannelSyncHelper() {}
 
     // All setters are called on the main thread.
     void setConnectRequestResult(bool connectRequestResult)
     {
+        DCHECK(isMainThread());
         m_connectRequestResult = connectRequestResult;
     }
 
-    // All getter are called on the worker thread.
+    // All getters are called on the worker thread.
     bool connectRequestResult() const
     {
+        DCHECK(!isMainThread());
         return m_connectRequestResult;
     }
 
     // This should be called after all setters are called and before any
     // getters are called.
     void signalWorkerThread()
     {
-        m_event->signal();
+        DCHECK(isMainThread());
+        m_event.signal();
     }
+
     void wait()
     {
-        m_event->wait();
+        DCHECK(!isMainThread());
+        m_event.wait();
     }
 
-    DEFINE_INLINE_TRACE() { }
-
 private:
-    explicit WebSocketChannelSyncHelper(std::unique_ptr<WaitableEvent> event)
-        : m_event(std::move(event))
-        , m_connectRequestResult(false)
-    {
-    }
-
-    std::unique_ptr<WaitableEvent> m_event;
-    bool m_connectRequestResult;
+    WaitableEvent m_event;
+    bool m_connectRequestResult = false;
 };
 
 WorkerWebSocketChannel::WorkerWebSocketChannel(WorkerGlobalScope& workerGlobalScope, WebSocketChannelClient* client, std::unique_ptr<SourceLocation> location)
     : m_bridge(new Bridge(client, workerGlobalScope))
     , m_locationAtConnection(std::move(location))
 {
 }
 
 WorkerWebSocketChannel::~WorkerWebSocketChannel()
 {
@@ skipping 55 matching lines @@
     m_bridge->disconnect();
     m_bridge.clear();
 }
 
 DEFINE_TRACE(WorkerWebSocketChannel)
 {
     visitor->trace(m_bridge);
     WebSocketChannel::trace(visitor);
 }
 
-Peer::Peer(Bridge* bridge, PassRefPtr<WorkerLoaderProxy> loaderProxy, WebSocketChannelSyncHelper* syncHelper, WorkerThreadLifecycleContext* workerThreadLifecycleContext)
+Peer::Peer(Bridge* bridge, PassRefPtr<WorkerLoaderProxy> loaderProxy, WorkerThreadLifecycleContext* workerThreadLifecycleContext)
     : WorkerThreadLifecycleObserver(workerThreadLifecycleContext)
     , m_bridge(bridge)
     , m_loaderProxy(loaderProxy)
     , m_mainWebSocketChannel(nullptr)
-    , m_syncHelper(syncHelper)
 {
     DCHECK(isMainThread());
 }
 
 Peer::~Peer()
 {
     DCHECK(isMainThread());
 }
 
 bool Peer::initialize(std::unique_ptr<SourceLocation> location, ExecutionContext* context)
@@ skipping 31 matching lines @@
 void Peer::sendBlob(PassRefPtr<BlobDataHandle> blobData)
 {
     ASSERT(isMainThread());
     if (m_mainWebSocketChannel)
         m_mainWebSocketChannel->send(blobData);
 }
 
 void Peer::close(int code, const String& reason)
 {
     ASSERT(isMainThread());
-    ASSERT(m_syncHelper);
     if (!m_mainWebSocketChannel)
         return;
     m_mainWebSocketChannel->close(code, reason);
 }
 
 void Peer::fail(const String& reason, MessageLevel level, std::unique_ptr<SourceLocation> location)
 {
     ASSERT(isMainThread());
-    ASSERT(m_syncHelper);
     if (!m_mainWebSocketChannel)
         return;
     m_mainWebSocketChannel->fail(reason, level, std::move(location));
 }
 
 void Peer::disconnect()
 {
     ASSERT(isMainThread());
-    ASSERT(m_syncHelper);
-    if (m_mainWebSocketChannel) {
-        m_mainWebSocketChannel->disconnect();
-        m_mainWebSocketChannel = nullptr;
-    }
-    m_syncHelper->signalWorkerThread();
+    if (!m_mainWebSocketChannel)
+        return;
+    m_mainWebSocketChannel->disconnect();
+    m_mainWebSocketChannel = nullptr;
 }
 
 static void workerGlobalScopeDidConnect(Bridge* bridge, const String& subprotocol, const String& extensions, ExecutionContext* context)
 {
     ASSERT_UNUSED(context, context->isWorkerGlobalScope());
     if (bridge && bridge->client())
         bridge->client()->didConnect(subprotocol, extensions);
 }
 
 void Peer::didConnect(const String& subprotocol, const String& extensions)
@@ skipping 90 matching lines @@
     if (m_mainWebSocketChannel) {
         m_mainWebSocketChannel->disconnect();
         m_mainWebSocketChannel = nullptr;
     }
     m_bridge = nullptr;
 }
 
 DEFINE_TRACE(Peer)
 {
     visitor->trace(m_mainWebSocketChannel);
-    visitor->trace(m_syncHelper);
     WebSocketChannelClient::trace(visitor);
     WorkerThreadLifecycleObserver::trace(visitor);
 }
 
 Bridge::Bridge(WebSocketChannelClient* client, WorkerGlobalScope& workerGlobalScope)
     : m_client(client)
     , m_workerGlobalScope(workerGlobalScope)
     , m_loaderProxy(m_workerGlobalScope->thread()->workerLoaderProxy())
-    , m_syncHelper(WebSocketChannelSyncHelper::create(wrapUnique(new WaitableEvent())))
 {
 }
 
 Bridge::~Bridge()
 {
     ASSERT(!m_peer);
 }
 
-void Bridge::connectOnMainThread(std::unique_ptr<SourceLocation> location, WorkerThreadLifecycleContext* workerThreadLifecycleContext, const KURL& url, const String& protocol, ExecutionContext* context)
+void Bridge::connectOnMainThread(std::unique_ptr<SourceLocation> location, WorkerThreadLifecycleContext* workerThreadLifecycleContext, const KURL& url, const String& protocol, WebSocketChannelSyncHelper* syncHelper, ExecutionContext* context)
 {
     DCHECK(isMainThread());
     DCHECK(!m_peer);
-    Peer* peer = new Peer(this, m_loaderProxy, m_syncHelper, workerThreadLifecycleContext);
+    Peer* peer = new Peer(this, m_loaderProxy, workerThreadLifecycleContext);
     if (peer->initialize(std::move(location), context)) {
         m_peer = peer;
-        bool result = m_peer->connect(url, protocol);
-        m_syncHelper->setConnectRequestResult(result);
+        syncHelper->setConnectRequestResult(m_peer->connect(url, protocol));
     }
-    m_syncHelper->signalWorkerThread();
+    syncHelper->signalWorkerThread();
 }
 
 bool Bridge::connect(std::unique_ptr<SourceLocation> location, const KURL& url, const String& protocol)
 {
     // Wait for completion of the task on the main thread because the mixed
     // content check must synchronously be conducted.
-    if (!waitForMethodCompletion(BLINK_FROM_HERE, createCrossThreadTask(&Bridge::connectOnMainThread, wrapCrossThreadPersistent(this), passed(location->clone()), wrapCrossThreadPersistent(m_workerGlobalScope->thread()->getWorkerThreadLifecycleContext()), url, protocol)))
-        return false;
-
-    return m_syncHelper->connectRequestResult();
+    WebSocketChannelSyncHelper syncHelper;
+    m_loaderProxy->postTaskToLoader(BLINK_FROM_HERE, createCrossThreadTask(&Bridge::connectOnMainThread, wrapCrossThreadPersistent(this), passed(location->clone()), wrapCrossThreadPersistent(m_workerGlobalScope->thread()->getWorkerThreadLifecycleContext()), url, protocol, crossThreadUnretained(&syncHelper)));
+    {
+        SafePointScope scope(BlinkGC::HeapPointersOnStack);
+        syncHelper.wait();
+    }
+    return syncHelper.connectRequestResult();
 }
 
 void Bridge::send(const CString& message)
 {
     ASSERT(m_peer);
     std::unique_ptr<Vector<char>> data = wrapUnique(new Vector<char>(message.length()));
     if (message.length())
         memcpy(data->data(), static_cast<const char*>(message.data()), message.length());
 
     m_loaderProxy->postTaskToLoader(BLINK_FROM_HERE, createCrossThreadTask(&Peer::sendTextAsCharVector, wrapCrossThreadPersistent(m_peer.get()), passed(std::move(data))));
@@ skipping 26 matching lines @@
 {
     ASSERT(m_peer);
     m_loaderProxy->postTaskToLoader(BLINK_FROM_HERE, createCrossThreadTask(&Peer::fail, wrapCrossThreadPersistent(m_peer.get()), reason, level, passed(location->clone())));
 }
 
 void Bridge::disconnect()
 {
     if (!m_peer)
         return;
 
-    // Wait for completion of the task on the main thread to ensure that
-    // |m_peer| does not touch this Bridge object after this point.
-    waitForMethodCompletion(BLINK_FROM_HERE, createCrossThreadTask(&Peer::disconnect, wrapCrossThreadPersistent(m_peer.get())));
+    m_loaderProxy->postTaskToLoader(BLINK_FROM_HERE, createCrossThreadTask(&Peer::disconnect, wrapCrossThreadPersistent(m_peer.get())));

[yhirano, 2016/08/24 08:01:42]

|m_peer| is a CrossThreadPersistent, so wrapping is not needed.

[nhiroki, 2016/08/24 08:37:18]

Done.

 
     m_client = nullptr;
     m_peer = nullptr;
-    m_syncHelper = nullptr;
-    // We won't use this any more.
     m_workerGlobalScope.clear();
 }
 
-// Caller of this function should hold a reference to the bridge, because this function may call WebSocket::didClose() in the end,
-// which causes the bridge to get disconnected from the WebSocket and deleted if there is no other reference.
-bool Bridge::waitForMethodCompletion(const WebTraceLocation& location, std::unique_ptr<ExecutionContextTask> task)
-{
-    ASSERT(m_workerGlobalScope);
-    ASSERT(m_syncHelper);
-
-    m_loaderProxy->postTaskToLoader(location, std::move(task));
-
-    // We wait for the syncHelper event even if a shutdown event is fired.
-    // See https://codereview.chromium.org/267323004/#msg43 for why we need to wait this.
-    SafePointScope scope(BlinkGC::HeapPointersOnStack);
-    m_syncHelper->wait();
-    // This is checking whether a shutdown event is fired or not.
-    return !m_workerGlobalScope->thread()->terminated();
-}
-
 DEFINE_TRACE(Bridge)
 {
     visitor->trace(m_client);
     visitor->trace(m_workerGlobalScope);
-    visitor->trace(m_syncHelper);
 }
 
 } // namespace blink