[WebSocket] Task creation should be separated from task posting.

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 437 matching lines @@
     RefPtr<WeakReference<Peer> > reference = WeakReference<Peer>::createUnbound();
     m_peer = WeakPtr<Peer>(reference);
 #endif
 
     RefPtrWillBeRawPtr<ThreadableWebSocketChannelSyncHelper> syncHelper = ThreadableWebSocketChannelSyncHelper::create(adoptPtr(blink::Platform::current()->createWaitableEvent()));
     // This pointer is guaranteed to be valid until we call terminatePeer.
     m_syncHelper = syncHelper.get();
 
     RefPtrWillBeRawPtr<Bridge> protect(this);
 #if ENABLE(OILPAN)
-    if (!waitForMethodCompletion(createCallbackTask(&Peer::initialize, &m_peer, AllowCrossThreadAccess(&m_loaderProxy), m_workerClientWrapper.get(), sourceURL, lineNumber, syncHelper.get()))) {
+    // In order to assure all temporally objects to be destroyed before
+    // posting the task, we should bind the task. In other words, it is
+    // dangerous to have a complicated expression as a waitForMethodCompletion
+    // argument.
+    OwnPtr<ExecutionContextTask> task = createCallbackTask(&Peer::initialize, &m_peer, AllowCrossThreadAccess(&m_loaderProxy), m_workerClientWrapper.get(), sourceURL, lineNumber, syncHelper.get());
 #else
-    if (!waitForMethodCompletion(createCallbackTask(&Peer::initialize, reference, AllowCrossThreadAccess(&m_loaderProxy), m_workerClientWrapper.get(), sourceURL, lineNumber, syncHelper.get()))) {
+    // See the above comment.
+    OwnPtr<ExecutionContextTask> task = createCallbackTask(&Peer::initialize, reference, AllowCrossThreadAccess(&m_loaderProxy), m_workerClientWrapper.get(), sourceURL, lineNumber, syncHelper.get());
 #endif
+    if (!waitForMethodCompletion(task.release())) {
         // The worker thread has been signalled to shutdown before method completion.
         disconnect();
     }
 }
 
 bool WorkerThreadableWebSocketChannel::Bridge::connect(const KURL& url, const String& protocol)
 {
     if (hasTerminatedPeer())
         return false;
 
     RefPtrWillBeRawPtr<Bridge> protect(this);
-    if (!waitForMethodCompletion(CallClosureTask::create(bind(&Peer::connect, m_peer, url.copy(), protocol.isolatedCopy()))))
+    // It is important to seprate the task creation from the
+    // waitForMethodCompletion call. See the above comment.
+    OwnPtr<ExecutionContextTask> task = CallClosureTask::create(bind(&Peer::connect, m_peer, url.copy(), protocol.isolatedCopy()));
+    if (!waitForMethodCompletion(task.release()))
         return false;
 
     return m_syncHelper->connectRequestResult();
 }
 
 WebSocketChannel::SendResult WorkerThreadableWebSocketChannel::Bridge::send(const String& message)
 {
     if (hasTerminatedPeer())
         return WebSocketChannel::SendFail;
 
     RefPtrWillBeRawPtr<Bridge> protect(this);
-    if (!waitForMethodCompletion(CallClosureTask::create(bind(&Peer::send, m_peer, message.isolatedCopy()))))
+    // It is important to seprate the task creation from the
+    // waitForMethodCompletion call. See the above comment.
+    OwnPtr<ExecutionContextTask> task = CallClosureTask::create(bind(&Peer::send, m_peer, message.isolatedCopy()));
+    if (!waitForMethodCompletion(task.release()))
         return WebSocketChannel::SendFail;
 
     return m_syncHelper->sendRequestResult();
 }
 
 WebSocketChannel::SendResult WorkerThreadableWebSocketChannel::Bridge::send(const ArrayBuffer& binaryData, unsigned byteOffset, unsigned byteLength)
 {
     if (hasTerminatedPeer())
         return WebSocketChannel::SendFail;
 
     // 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);
 
     RefPtrWillBeRawPtr<Bridge> protect(this);
-    if (!waitForMethodCompletion(CallClosureTask::create(bind(&Peer::sendArrayBuffer, m_peer, data.release()))))
+    // It is important to seprate the task creation from the
+    // waitForMethodCompletion call. See the above comment.
+    OwnPtr<ExecutionContextTask> task = CallClosureTask::create(bind(&Peer::sendArrayBuffer, m_peer, data.release()));
+    if (!waitForMethodCompletion(task.release()))
         return WebSocketChannel::SendFail;
 
     return m_syncHelper->sendRequestResult();
 }
 
 WebSocketChannel::SendResult WorkerThreadableWebSocketChannel::Bridge::send(PassRefPtr<BlobDataHandle> data)
 {
     if (hasTerminatedPeer())
         return WebSocketChannel::SendFail;
 
     RefPtrWillBeRawPtr<Bridge> protect(this);
-    if (!waitForMethodCompletion(CallClosureTask::create(bind(&Peer::sendBlob, m_peer, data))))
+    // It is important to seprate the task creation from the
+    // waitForMethodCompletion call. See the above comment.
+    OwnPtr<ExecutionContextTask> task = CallClosureTask::create(bind(&Peer::sendBlob, m_peer, data));
+    if (!waitForMethodCompletion(task.release()))
         return WebSocketChannel::SendFail;
 
     return m_syncHelper->sendRequestResult();
 }
 
 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())));

[hiroshige, 2014/07/01 14:13:41]

Should we modify this line as well?
The temporary objects (if not omitted by compiler optimization) are not
destroyed strictly before submitting the task.
(The probability of error is much lower than waitForMethodCompletion though)

[yhirano, 2014/07/02 01:35:23]

Thanks, done.

 }
 
 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)));

[hiroshige, 2014/07/01 14:13:41]

same as above.

[yhirano, 2014/07/02 01:35:23]

Done.

 }
 
 void WorkerThreadableWebSocketChannel::Bridge::disconnect()
 {
     if (hasTerminatedPeer())
         return;
 
     clearClientWrapper();
     terminatePeer();
 }
@@ skipping 18 matching lines @@
     events.append(m_syncHelper->event());
     ThreadState::SafePointScope scope(ThreadState::HeapPointersOnStack);
     blink::Platform::current()->waitMultipleEvents(events);
     // This is checking whether a shutdown event is fired or not.
     return !m_workerGlobalScope->thread()->runLoop().terminated();
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::terminatePeer()
 {
     ASSERT(!hasTerminatedPeer());
+
+    // It is important to seprate the task creation from the
+    // waitForMethodCompletion call. See the above comment.
+    OwnPtr<ExecutionContextTask> task = CallClosureTask::create(bind(&Peer::destroy, m_peer));
 #if ENABLE(OILPAN)
     // The worker thread has to wait for the main thread to complete Peer::destroy,
     // because the worker thread has to make sure that the main thread does not have any
     // references to on-heap objects allocated in the thread heap of the worker thread
     // before the worker thread shuts down.
-    waitForMethodCompletion(CallClosureTask::create(bind(&Peer::destroy, m_peer)));
+    waitForMethodCompletion(task.release());
 #else
-    m_loaderProxy.postTaskToLoader(CallClosureTask::create(bind(&Peer::destroy, m_peer)));
+    m_loaderProxy.postTaskToLoader(task.release());
 #endif
 
     // Peer::destroy() deletes m_peer and then m_syncHelper will be released.
     // We must not touch m_syncHelper any more.
     m_syncHelper = nullptr;
 
     // We won't use this any more.
     m_workerGlobalScope = nullptr;
 }
 
 void WorkerThreadableWebSocketChannel::Bridge::trace(Visitor* visitor)
 {
     visitor->trace(m_workerClientWrapper);
     visitor->trace(m_workerGlobalScope);
     visitor->trace(m_syncHelper);
     visitor->trace(m_peer);
 }
 
 } // namespace WebCore