Cleanup WorkerThreadableWebSocketChannel's logic to wait for the main thread

Source/modules/websockets/WorkerThreadableWebSocketChannel.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 102 matching lines @@
         , m_bufferedAmount(0)
     {
     }
 
     OwnPtr<blink::WebWaitableEvent> m_event;
     bool m_connectRequestResult;
     WebSocketChannel::SendResult m_sendRequestResult;
     unsigned long m_bufferedAmount;
 };
 
-WorkerThreadableWebSocketChannel::WorkerThreadableWebSocketChannel(WorkerGlobalScope& context, WebSocketChannelClient* client, const String& sourceURL, unsigned lineNumber)
-    : m_workerGlobalScope(context)
-    , m_workerClientWrapper(ThreadableWebSocketChannelClientWrapper::create(client))
-    , m_bridge(Bridge::create(m_workerClientWrapper, m_workerGlobalScope))
+WorkerThreadableWebSocketChannel::WorkerThreadableWebSocketChannel(WorkerGlobalScope& workerGlobalScope, WebSocketChannelClient* client, const String& sourceURL, unsigned lineNumber)
+    : m_workerClientWrapper(ThreadableWebSocketChannelClientWrapper::create(client))
+    , m_bridge(Bridge::create(m_workerClientWrapper, workerGlobalScope))
     , m_sourceURLAtConnection(sourceURL)
     , m_lineNumberAtConnection(lineNumber)
 {
     ASSERT(m_workerClientWrapper.get());
     m_bridge->initialize(sourceURL, lineNumber);
 }
 
 WorkerThreadableWebSocketChannel::~WorkerThreadableWebSocketChannel()
 {
     if (m_bridge)
@@ skipping 313 matching lines @@
     ASSERT_UNUSED(context, context->isWorkerGlobalScope());
     workerClientWrapper->didReceiveMessageError();
 }
 
 void WorkerThreadableWebSocketChannel::Peer::didReceiveMessageError()
 {
     ASSERT(isMainThread());
     m_loaderProxy.postTaskToWorkerGlobalScope(createCallbackTask(&workerGlobalScopeDidReceiveMessageError, m_workerClientWrapper));
 }
 
-WorkerThreadableWebSocketChannel::Bridge::Bridge(PassRefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper, PassRefPtrWillBeRawPtr<WorkerGlobalScope> workerGlobalScope)
+WorkerThreadableWebSocketChannel::Bridge::Bridge(PassRefPtr<ThreadableWebSocketChannelClientWrapper> workerClientWrapper, WorkerGlobalScope& workerGlobalScope)
     : m_workerClientWrapper(workerClientWrapper)
     , m_workerGlobalScope(workerGlobalScope)
     , m_loaderProxy(m_workerGlobalScope->thread()->workerLoaderProxy())
     , m_syncHelper(0)
 {
     ASSERT(m_workerClientWrapper.get());
 }
 
 WorkerThreadableWebSocketChannel::Bridge::~Bridge()
 {
@@ skipping 12 matching lines @@
     RefPtr<Bridge> protect(this);
     m_loaderProxy.postTaskToLoader(createCallbackTask(&Peer::initialize, reference.release(), AllowCrossThreadAccess(&m_loaderProxy), m_workerClientWrapper, sourceURL, lineNumber, syncHelper.release()));
     if (!waitForMethodCompletion()) {
         // The worker thread has been signalled to shutdown before method completion.
         terminatePeer();
     }
 }
 
 bool WorkerThreadableWebSocketChannel::Bridge::connect(const KURL& url, const String& protocol)
 {
-    if (!m_workerGlobalScope)
+    if (hasTerminatedPeer())
         return false;
-    ASSERT(m_syncHelper);
+
+    RefPtr<Bridge> protect(this);

[yhirano, 2014/05/07 08:05:18]

Can you tell me why this move is needed? I think posting a task can't delete
|this|.

[tyoshino (SeeGerritForStatus), 2014/05/08 05:03:00]

As discussed offline, it looks we no longer need these protects. But for this
CL, let's keep them and just let me make their position consistent.

     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::connect, m_peer, url.copy(), protocol.isolatedCopy())));
-    RefPtr<Bridge> protect(this);
-    waitForMethodCompletion();
+    if (!waitForMethodCompletion())

[yhirano, 2014/05/07 08:05:18]

[optional] We could have postTaskAndWait for this pattern.

[tyoshino (SeeGerritForStatus), 2014/05/08 05:03:00]

Done.

+        return false;
+
     return m_syncHelper->connectRequestResult();
 }
 
 WebSocketChannel::SendResult WorkerThreadableWebSocketChannel::Bridge::send(const String& message)
 {
-    if (!m_workerGlobalScope)
+    if (hasTerminatedPeer())
         return WebSocketChannel::SendFail;
-    ASSERT(m_syncHelper);
+
+    RefPtr<Bridge> protect(this);
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::send, m_peer, message.isolatedCopy())));
-    RefPtr<Bridge> protect(this);
-    waitForMethodCompletion();
+    if (!waitForMethodCompletion())
+        return WebSocketChannel::SendFail;
+
     return m_syncHelper->sendRequestResult();
 }
 
 WebSocketChannel::SendResult WorkerThreadableWebSocketChannel::Bridge::send(const ArrayBuffer& binaryData, unsigned byteOffset, unsigned byteLength)
 {
-    if (!m_workerGlobalScope)
+    if (hasTerminatedPeer())
         return WebSocketChannel::SendFail;
-    ASSERT(m_syncHelper);
+
     // ArrayBuffer isn't thread-safe, hence the content of ArrayBuffer is copied into Vector<char>.
     OwnPtr<Vector<char> > data = adoptPtr(new Vector<char>(byteLength));
     if (binaryData.byteLength())
         memcpy(data->data(), static_cast<const char*>(binaryData.data()) + byteOffset, byteLength);
 
+    RefPtr<Bridge> protect(this);
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::sendArrayBuffer, m_peer, data.release())));
-    RefPtr<Bridge> protect(this);
-    waitForMethodCompletion();
+    if (!waitForMethodCompletion())
+        return WebSocketChannel::SendFail;
+
     return m_syncHelper->sendRequestResult();
 }
 
 WebSocketChannel::SendResult WorkerThreadableWebSocketChannel::Bridge::send(PassRefPtr<BlobDataHandle> data)
 {
-    if (!m_workerGlobalScope)
+    if (hasTerminatedPeer())
         return WebSocketChannel::SendFail;
-    ASSERT(m_syncHelper);
+
+    RefPtr<Bridge> protect(this);
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::sendBlob, m_peer, data)));
-    RefPtr<Bridge> protect(this);
-    waitForMethodCompletion();
+    if (!waitForMethodCompletion())
+        return WebSocketChannel::SendFail;
+
     return m_syncHelper->sendRequestResult();
 }
 
 unsigned long WorkerThreadableWebSocketChannel::Bridge::bufferedAmount()
 {
-    if (!m_workerGlobalScope)
+    if (hasTerminatedPeer())
         return 0;
-    ASSERT(m_syncHelper);
+
+    RefPtr<Bridge> protect(this);
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::bufferedAmount, m_peer)));
-    RefPtr<Bridge> protect(this);
-    waitForMethodCompletion();
+    if (!waitForMethodCompletion())
+        return 0;
+
     return m_syncHelper->bufferedAmount();
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::close(int code, const String& reason)
 {
+    if (hasTerminatedPeer())
+        return;
+
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::close, m_peer, code, reason.isolatedCopy())));
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::fail(const String& reason, MessageLevel level, const String& sourceURL, unsigned lineNumber)
 {
+    if (hasTerminatedPeer())
+        return;
+
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::fail, m_peer, reason.isolatedCopy(), level, sourceURL.isolatedCopy(), lineNumber)));
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::disconnect()
 {
     clearClientWrapper();
     terminatePeer();
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::suspend()
 {
+    if (hasTerminatedPeer())
+        return;
+
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::suspend, m_peer)));
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::resume()
 {
+    if (hasTerminatedPeer())
+        return;
+
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::resume, m_peer)));
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::clearClientWrapper()
 {
     m_workerClientWrapper->clearClient();
 }
 
 // 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 WorkerThreadableWebSocketChannel::Bridge::waitForMethodCompletion()
 {
-    if (!m_syncHelper)
-        return true;
+    ASSERT(m_workerGlobalScope);
+    ASSERT(m_syncHelper);
 
     blink::WebWaitableEvent* shutdownEvent = m_workerGlobalScope->thread()->shutdownEvent();
     Vector<blink::WebWaitableEvent*> events;
     events.append(shutdownEvent);
     events.append(m_syncHelper->event());
 
     ThreadState::SafePointScope scope(ThreadState::HeapPointersOnStack);
     blink::WebWaitableEvent* signalled = blink::Platform::current()->waitMultipleEvents(events);
     return signalled != shutdownEvent;
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::terminatePeer()
 {
     m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::destroy, m_peer)));
+    // Peer::destroy() deletes m_peer and then m_syncHelper will be released.
+    // We must not touch m_syncHelper any more.
+    m_syncHelper = 0;
+
+    // We won't use this any more.
     m_workerGlobalScope = nullptr;
-    m_syncHelper = 0;
 }
 
 } // namespace WebCore