Call WebWorkerClient on the main thread. This makes it consistent with the rest of the WebKit API, which is single threaded. Also a bunch of small fixes to make layout tests pass: the dll was being unloaded while its functions were still queued to be dispatched, and a string allocated in the dll was being GC'd in test shell.

BUG=11011
Review URL: http://codereview.chromium.org/102005

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@15087 0039d316-1c4b-4281-b951-d872f2087c98

8 files changed, 200 insertions, 120 deletions

diff --git a/chrome/worker/webworkerclient_proxy.cc b/chrome/worker/webworkerclient_proxy.cc
index fa2bd39..4e154e2 100644
--- a/chrome/worker/webworkerclient_proxy.cc
+++ b/chrome/worker/webworkerclient_proxy.cc
@@ -20,7 +20,6 @@ WebWorkerClientProxy::WebWorkerClientProxy(const GURL& url, int route_id)
     : url_(url),
       route_id_(route_id),
       ALLOW_THIS_IN_INITIALIZER_LIST(impl_(WebWorker::create(this))) {
-  AddRef();
   WorkerThread::current()->AddRoute(route_id_, this);
   ChildProcess::current()->AddRefProcess();
 }
@@ -67,18 +66,11 @@ void WebWorkerClientProxy::reportPendingActivity(bool has_pending_activity) {
 
 void WebWorkerClientProxy::workerContextDestroyed() {
   Send(new WorkerHostMsg_WorkerContextDestroyed(route_id_));
-  impl_ = NULL;
 
-  WorkerThread::current()->message_loop()->ReleaseSoon(FROM_HERE, this);
+  delete this;
 }
 
 bool WebWorkerClientProxy::Send(IPC::Message* message) {
-  if (MessageLoop::current() != WorkerThread::current()->message_loop()) {
-    WorkerThread::current()->message_loop()->PostTask(FROM_HERE,
-        NewRunnableMethod(this, &WebWorkerClientProxy::Send, message));
-    return true;
-  }
-
   return WorkerThread::current()->Send(message);
 }
 
diff --git a/chrome/worker/webworkerclient_proxy.h b/chrome/worker/webworkerclient_proxy.h
index f650d58..ccf9557 100644
--- a/chrome/worker/webworkerclient_proxy.h
+++ b/chrome/worker/webworkerclient_proxy.h
@@ -6,7 +6,6 @@
 #define CHROME_WORKER_WEBWORKERCLIENT_PROXY_H_
 
 #include "base/basictypes.h"
-#include "base/ref_counted.h"
 #include "chrome/common/ipc_channel.h"
 #include "googleurl/src/gurl.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebWorkerClient.h"
@@ -21,8 +20,7 @@ class WebWorker;
 // IPCs that are sent to the renderer, where they're converted back to function
 // calls by WebWorkerProxy.
 class WebWorkerClientProxy : public WebKit::WebWorkerClient,
-                             public IPC::Channel::Listener,
-                             public base::RefCounted<WebWorkerClientProxy> {
+                             public IPC::Channel::Listener {
  public:
   WebWorkerClientProxy(const GURL& url, int route_id);
 
@@ -47,7 +45,6 @@ class WebWorkerClientProxy : public WebKit::WebWorkerClient,
   virtual void OnMessageReceived(const IPC::Message& message);
 
  private:
-  friend class base::RefCounted<WebWorkerClientProxy>;
   ~WebWorkerClientProxy ();
 
   bool Send(IPC::Message* message);
diff --git a/webkit/glue/webworker_impl.cc b/webkit/glue/webworker_impl.cc
index a8e461d..ba78c7d 100644
--- a/webkit/glue/webworker_impl.cc
+++ b/webkit/glue/webworker_impl.cc
@@ -12,6 +12,7 @@
 #include "SecurityOrigin.h"
 #include "WorkerContext.h"
 #include "WorkerThread.h"
+#include <wtf/MainThread.h>
 #include <wtf/Threading.h>
 
 #undef LOG
@@ -106,17 +107,54 @@ void WebWorkerImpl::postMessageToWorkerContext(const WebString& message) {
 void WebWorkerImpl::workerObjectDestroyed() {
 }
 
+void WebWorkerImpl::DispatchTaskToMainThread(
+    PassRefPtr<WebCore::ScriptExecutionContext::Task> task) {
+  return WTF::callOnMainThread(InvokeTaskMethod, task.releaseRef());
+}
+
+void WebWorkerImpl::InvokeTaskMethod(void* param) {
+  WebCore::ScriptExecutionContext::Task* task =
+      static_cast<WebCore::ScriptExecutionContext::Task*>(param);
+  task->performTask(NULL);
+  task->deref();
+}
+
 // WorkerObjectProxy -----------------------------------------------------------
 
 void WebWorkerImpl::postMessageToWorkerObject(const WebCore::String& message) {
-  client_->postMessageToWorkerObject(webkit_glue::StringToWebString(message));
+  DispatchTaskToMainThread(WebCore::createCallbackTask(
+      &PostMessageTask,
+      this,
+      message));
+}
+
+void WebWorkerImpl::PostMessageTask(
+    WebCore::ScriptExecutionContext* context,
+    WebWorkerImpl* this_ptr,
+    WebCore::String message) {
+  this_ptr->client_->postMessageToWorkerObject(
+      webkit_glue::StringToWebString(message));
 }
 
 void WebWorkerImpl::postExceptionToWorkerObject(
     const WebCore::String& error_message,
     int line_number,
     const WebCore::String& source_url) {
-  client_->postExceptionToWorkerObject(
+  DispatchTaskToMainThread(WebCore::createCallbackTask(
+      &PostExceptionTask,
+      this,
+      error_message,
+      line_number,
+      source_url));
+}
+
+void WebWorkerImpl::PostExceptionTask(
+      WebCore::ScriptExecutionContext* context,
+      WebWorkerImpl* this_ptr,
+      const WebCore::String& error_message,
+      int line_number,
+      const WebCore::String& source_url) {
+  this_ptr->client_->postExceptionToWorkerObject(
       webkit_glue::StringToWebString(error_message),
       line_number,
       webkit_glue::StringToWebString(source_url));
@@ -129,28 +167,76 @@ void WebWorkerImpl::postConsoleMessageToWorkerObject(
     const WebCore::String& message,
     int line_number,
     const WebCore::String& source_url) {
-  client_->postConsoleMessageToWorkerObject(
+  DispatchTaskToMainThread(WebCore::createCallbackTask(
+      &PostConsoleMessageTask,
+      this,
       static_cast<int>(destination),
       static_cast<int>(source),
       static_cast<int>(level),
+      message,
+      line_number,
+      source_url));
+}
+
+void WebWorkerImpl::PostConsoleMessageTask(
+    WebCore::ScriptExecutionContext* context,
+    WebWorkerImpl* this_ptr,
+    int destination,
+    int source,
+    int level,
+    const WebCore::String& message,
+    int line_number,
+    const WebCore::String& source_url) {
+  this_ptr->client_->postConsoleMessageToWorkerObject(
+      destination,
+      source,
+      level,
       webkit_glue::StringToWebString(message),
       line_number,
       webkit_glue::StringToWebString(source_url));
 }
 
 void WebWorkerImpl::confirmMessageFromWorkerObject(bool has_pending_activity) {
-  client_->confirmMessageFromWorkerObject(has_pending_activity);
+  DispatchTaskToMainThread(WebCore::createCallbackTask(
+      &ConfirmMessageTask,
+      this,
+      has_pending_activity));
+}
+
+void WebWorkerImpl::ConfirmMessageTask(
+    WebCore::ScriptExecutionContext* context,
+    WebWorkerImpl* this_ptr,
+    bool has_pending_activity) {
+  this_ptr->client_->confirmMessageFromWorkerObject(has_pending_activity);
 }
 
 void WebWorkerImpl::reportPendingActivity(bool has_pending_activity) {
-  client_->reportPendingActivity(has_pending_activity);
+  DispatchTaskToMainThread(WebCore::createCallbackTask(
+      &ReportPendingActivityTask,
+      this,
+      has_pending_activity));
+}
+
+void WebWorkerImpl::ReportPendingActivityTask(
+    WebCore::ScriptExecutionContext* context,
+    WebWorkerImpl* this_ptr,
+    bool has_pending_activity) {
+  this_ptr->client_->reportPendingActivity(has_pending_activity);
 }
 
 void WebWorkerImpl::workerContextDestroyed() {
-  client_->workerContextDestroyed();
+  DispatchTaskToMainThread(WebCore::createCallbackTask(
+      &WorkerContextDestroyedTask,
+      this));
+}
+
+void WebWorkerImpl::WorkerContextDestroyedTask(
+    WebCore::ScriptExecutionContext* context,
+    WebWorkerImpl* this_ptr) {
+  this_ptr->client_->workerContextDestroyed();
 
   // The lifetime of this proxy is controlled by the worker context.
-  delete this;
+  delete this_ptr;
 }
 
 #else
diff --git a/webkit/glue/webworker_impl.h b/webkit/glue/webworker_impl.h
index 8ae78c6..bfdd26a 100644
--- a/webkit/glue/webworker_impl.h
+++ b/webkit/glue/webworker_impl.h
@@ -29,7 +29,6 @@ class WebWorkerImpl: public WebCore::WorkerObjectProxy,
                      public WebKit::WebWorker {
  public:
   explicit WebWorkerImpl(WebKit::WebWorkerClient* client);
-  virtual ~WebWorkerImpl();
 
   // WebCore::WorkerObjectProxy methods:
   virtual void postMessageToWorkerObject(const WebCore::String& message);
@@ -56,12 +55,54 @@ class WebWorkerImpl: public WebCore::WorkerObjectProxy,
   virtual void postMessageToWorkerContext(const WebKit::WebString& message);
   virtual void workerObjectDestroyed();
 
+  // Executes the given task on the main thread.
+  static void DispatchTaskToMainThread(
+      PassRefPtr<WebCore::ScriptExecutionContext::Task> task);
+
  private:
+  virtual ~WebWorkerImpl();
+
+  // Tasks that are run on the worker thread.
   static void PostMessageToWorkerContextTask(
       WebCore::ScriptExecutionContext* context,
       WebWorkerImpl* this_ptr,
       const WebCore::String& message);
 
+  // Function used to invoke tasks on the main thread.
+  static void InvokeTaskMethod(void* param);
+
+  // Tasks that are run on the main thread.
+  static void PostMessageTask(
+      WebCore::ScriptExecutionContext* context,
+      WebWorkerImpl* this_ptr,
+      WebCore::String message);
+  static void PostExceptionTask(
+      WebCore::ScriptExecutionContext* context,
+      WebWorkerImpl* this_ptr,
+      const WebCore::String& error_message,
+      int line_number,
+      const WebCore::String& source_url);
+  static void PostConsoleMessageTask(
+      WebCore::ScriptExecutionContext* context,
+      WebWorkerImpl* this_ptr,
+      int destination,
+      int source,
+      int level,
+      const WebCore::String& message,
+      int line_number,
+      const WebCore::String& source_url);
+  static void ConfirmMessageTask(
+      WebCore::ScriptExecutionContext* context,
+      WebWorkerImpl* this_ptr,
+      bool has_pending_activity);
+  static void ReportPendingActivityTask(
+      WebCore::ScriptExecutionContext* context,
+      WebWorkerImpl* this_ptr,
+      bool has_pending_activity);
+  static void WorkerContextDestroyedTask(
+      WebCore::ScriptExecutionContext* context,
+      WebWorkerImpl* this_ptr);
+
   WebKit::WebWorkerClient* client_;
   WTF::RefPtr<WebCore::WorkerThread> worker_thread_;
 
diff --git a/webkit/tools/test_shell/test_webworker_helper.cc b/webkit/tools/test_shell/test_webworker_helper.cc
index 6cf309c..20a8d4e 100644
--- a/webkit/tools/test_shell/test_webworker_helper.cc
+++ b/webkit/tools/test_shell/test_webworker_helper.cc
@@ -19,33 +19,34 @@
 #include "base/file_util.h"
 #include "base/path_service.h"
 #include "third_party/WebKit/WebKit/chromium/public/WebKit.h"
+#include "third_party/WebKit/WebKit/chromium/public/WebKitClient.h"
 
 using WebKit::WebWorker;
 using WebKit::WebWorkerClient;
 
+static TestWebWorkerHelper* g_helper;
+
 WebWorker* TestWebWorkerHelper::CreateWebWorker(WebWorkerClient* client) {
-  TestWebWorkerHelper* loader = new TestWebWorkerHelper();
-  return loader->CreateWebWorker_(client, loader);
+  if (!g_helper)
+    g_helper = new TestWebWorkerHelper();
+  g_helper->worker_count_++;
+  return g_helper->CreateWebWorker_(client, g_helper);
 }
 
 TestWebWorkerHelper::TestWebWorkerHelper() :
 #if defined(OS_WIN)
       module_(NULL),
 #endif
-      CreateWebWorker_(NULL) {
+      CreateWebWorker_(NULL),
+      worker_count_(0) {
   Load();
 }
 
 TestWebWorkerHelper::~TestWebWorkerHelper() {
 }
 
-bool TestWebWorkerHelper::IsMainThread() const {
-  return WTF::isMainThread();
-}
-
-void TestWebWorkerHelper::DispatchToMainThread(WTF::MainThreadFunction* func,
-                                               void* context) {
-  return WTF::callOnMainThread(func, context);
+void TestWebWorkerHelper::DispatchToMainThread(void (*func)()) {
+  WebKit::webKitClient()->callOnMainThread(func);
 }
 
 void TestWebWorkerHelper::Load() {
@@ -84,13 +85,22 @@ void TestWebWorkerHelper::Load() {
 }
 
 void TestWebWorkerHelper::Unload() {
+  DCHECK(worker_count_);
+  worker_count_--;
   // Since this is called from DLL, delay the unloading until it can be
   // invoked from EXE.
   return WTF::callOnMainThread(UnloadHelper, this);
 }
 
+WebKit::WebString TestWebWorkerHelper::DuplicateString(
+    const WebKit::WebString& string) {
+  return WebKit::WebString(string.data(), string.length());
+}
+
 void TestWebWorkerHelper::UnloadHelper(void* param) {
   TestWebWorkerHelper* this_ptr = static_cast<TestWebWorkerHelper*>(param);
+  if (this_ptr->worker_count_)
+    return;
 
 #if defined(OS_WIN)
   if (this_ptr->module_) {
@@ -108,4 +118,5 @@ void TestWebWorkerHelper::UnloadHelper(void* param) {
 #endif
 
   delete this_ptr;
+  g_helper = NULL;
 }
diff --git a/webkit/tools/test_shell/test_webworker_helper.h b/webkit/tools/test_shell/test_webworker_helper.h
index 6770678..951950b 100644
--- a/webkit/tools/test_shell/test_webworker_helper.h
+++ b/webkit/tools/test_shell/test_webworker_helper.h
@@ -12,12 +12,14 @@
 
 #include "base/basictypes.h"
 #include "base/port.h"
+#include "third_party/WebKit/WebKit/chromium/public/WebString.h"
 
 #include <wtf/MainThread.h>
 
 class TestWebWorkerHelper;
 
 namespace WebKit {
+class WebKitClient;
 class WebWorker;
 class WebWorkerClient;
 }
@@ -32,14 +34,14 @@ class TestWebWorkerHelper {
   static WebKit::WebWorker* CreateWebWorker(WebKit::WebWorkerClient* client);
 
   TestWebWorkerHelper();
-  ~TestWebWorkerHelper();
 
-  virtual bool IsMainThread() const;
-  virtual void DispatchToMainThread(WTF::MainThreadFunction* func,
-                                    void* context);
+  virtual void DispatchToMainThread(void (*func)());
   virtual void Unload();
 
+  virtual WebKit::WebString DuplicateString(const WebKit::WebString& string);
+
  private:
+  ~TestWebWorkerHelper();
   void Load();
   static void UnloadHelper(void* param);
 
@@ -51,6 +53,7 @@ class TestWebWorkerHelper {
 #endif
 
   CreateWebWorkerFunc CreateWebWorker_;
+  int worker_count_;
 
   DISALLOW_COPY_AND_ASSIGN(TestWebWorkerHelper);
 };
diff --git a/webkit/tools/test_shell/test_worker/test_webworker.cc b/webkit/tools/test_shell/test_worker/test_webworker.cc
index 6b073b5..29733b4 100644
--- a/webkit/tools/test_shell/test_worker/test_webworker.cc
+++ b/webkit/tools/test_shell/test_worker/test_webworker.cc
@@ -19,21 +19,6 @@ using WebKit::WebURL;
 using WebKit::WebWorker;
 using WebKit::WebWorkerClient;
 
-// We use this class to pass a WebString between threads.  This works by
-// copying to a string16 which is itself safe to pass between threads.
-// TODO(jam): Remove this once all worker callbacks happen on the main thread.
-class ThreadSafeWebString {
- public:
-  explicit ThreadSafeWebString(const WebString& str)
-      : str_(str) {
-  }
-  operator WebString() const {
-    return str_;
-  }
- private:
-  string16 str_;
-};
-
 TestWebWorker::TestWebWorker(WebWorkerClient* client,
                              TestWebWorkerHelper* webworker_helper)
     : webworkerclient_delegate_(client),
@@ -81,32 +66,22 @@ void TestWebWorker::workerObjectDestroyed() {
 }
 
 void TestWebWorker::postMessageToWorkerObject(const WebString& message) {
-  if (webworker_helper_->IsMainThread()) {
-    if (webworkerclient_delegate_)
-      webworkerclient_delegate_->postMessageToWorkerObject(message);
-  } else {
-    webworker_helper_->DispatchToMainThread(
-        InvokeMainThreadMethod, NewRunnableMethod(
-            this, &TestWebWorker::postMessageToWorkerObject,
-            ThreadSafeWebString(message)));
-  }
+  if (!webworkerclient_delegate_)
+    return;
+  // The string was created in the dll's memory space as a result of a postTask.
+  // If we pass it to test shell's memory space, it'll cause problems when GC
+  // occurs.  So duplicate it from the test shell's memory space first.
+  webworkerclient_delegate_->postMessageToWorkerObject(
+      webworker_helper_->DuplicateString(message));
 }
 
 void TestWebWorker::postExceptionToWorkerObject(const WebString& error_message,
                                                 int line_number,
                                                 const WebString& source_url) {
-  if (webworker_helper_->IsMainThread()) {
-    if (webworkerclient_delegate_)
-      webworkerclient_delegate_->postExceptionToWorkerObject(error_message,
-                                                             line_number,
-                                                             source_url);
-  } else {
-    webworker_helper_->DispatchToMainThread(
-        InvokeMainThreadMethod, NewRunnableMethod(
-            this, &TestWebWorker::postExceptionToWorkerObject,
-            ThreadSafeWebString(error_message), line_number,
-            ThreadSafeWebString(source_url)));
-  }
+  if (webworkerclient_delegate_)
+    webworkerclient_delegate_->postExceptionToWorkerObject(error_message,
+                                                           line_number,
+                                                           source_url);
 }
 
 void TestWebWorker::postConsoleMessageToWorkerObject(
@@ -116,63 +91,28 @@ void TestWebWorker::postConsoleMessageToWorkerObject(
     const WebString& message,
     int line_number,
     const WebString& source_url) {
-  if (webworker_helper_->IsMainThread()) {
-    if (webworkerclient_delegate_)
-      webworkerclient_delegate_->postConsoleMessageToWorkerObject(
-          destination_id, source_id, message_level, message, line_number,
-          source_url);
-  } else {
-    webworker_helper_->DispatchToMainThread(
-        InvokeMainThreadMethod, NewRunnableMethod(
-            this, &TestWebWorker::postConsoleMessageToWorkerObject,
-            destination_id, source_id, message_level,
-            ThreadSafeWebString(message), line_number,
-            ThreadSafeWebString(source_url)));
-  }
+  if (webworkerclient_delegate_)
+    webworkerclient_delegate_->postConsoleMessageToWorkerObject(
+        destination_id, source_id, message_level, message, line_number,
+        source_url);
 }
 
 void TestWebWorker::confirmMessageFromWorkerObject(bool has_pending_activity) {
-  if (webworker_helper_->IsMainThread()) {
-    if (webworkerclient_delegate_)
-      webworkerclient_delegate_->confirmMessageFromWorkerObject(
-          has_pending_activity);
-  } else {
-    webworker_helper_->DispatchToMainThread(
-        InvokeMainThreadMethod, NewRunnableMethod(
-            this, &TestWebWorker::confirmMessageFromWorkerObject,
-            has_pending_activity));
-  }
+  if (webworkerclient_delegate_)
+    webworkerclient_delegate_->confirmMessageFromWorkerObject(
+        has_pending_activity);
 }
 
 void TestWebWorker::reportPendingActivity(bool has_pending_activity) {
-  if (webworker_helper_->IsMainThread()) {
-    if (webworkerclient_delegate_)
-      webworkerclient_delegate_->reportPendingActivity(has_pending_activity);
-  } else {
-    webworker_helper_->DispatchToMainThread(
-        InvokeMainThreadMethod, NewRunnableMethod(
-            this, &TestWebWorker::reportPendingActivity,
-            has_pending_activity));
-  }
+  if (webworkerclient_delegate_)
+    webworkerclient_delegate_->reportPendingActivity(has_pending_activity);
 }
 
 void TestWebWorker::workerContextDestroyed() {
-  if (webworker_helper_->IsMainThread()) {
-    if (webworkerclient_delegate_)
-      webworkerclient_delegate_->workerContextDestroyed();
-    Release();    // Releases the reference held for worker context object.
-  } else {
-    webworker_impl_ = NULL;
-    webworker_helper_->DispatchToMainThread(
-        InvokeMainThreadMethod, NewRunnableMethod(
-            this, &TestWebWorker::workerContextDestroyed));
-  }
-}
-
-void TestWebWorker::InvokeMainThreadMethod(void* param) {
-  Task* task = static_cast<Task*>(param);
-  task->Run();
-  delete task;
+  webworker_impl_ = NULL;
+  if (webworkerclient_delegate_)
+    webworkerclient_delegate_->workerContextDestroyed();
+  Release();    // Releases the reference held for worker context object.
 }
 
 #endif
diff --git a/webkit/tools/test_shell/test_worker/test_worker_main.cc b/webkit/tools/test_shell/test_worker/test_worker_main.cc
index 6695a11..9273d35 100644
--- a/webkit/tools/test_shell/test_worker/test_worker_main.cc
+++ b/webkit/tools/test_shell/test_worker/test_worker_main.cc
@@ -29,6 +29,9 @@ static base::AtExitManager global_at_exit_manager;
 // Stub WebKit Client.
 class WorkerWebKitClientImpl : public webkit_glue::WebKitClientImpl {
  public:
+  explicit WorkerWebKitClientImpl(TestWebWorkerHelper* helper)
+      : helper_(helper) { }
+
   // WebKitClient methods:
   virtual WebKit::WebClipboard* clipboard() {
     NOTREACHED();
@@ -76,6 +79,13 @@ class WorkerWebKitClientImpl : public webkit_glue::WebKitClientImpl {
     NOTREACHED();
     return WebKit::WebString();
   }
+
+  virtual void callOnMainThread(void (*func)()) {
+    helper_->DispatchToMainThread(func);
+  }
+
+ private:
+  TestWebWorkerHelper* helper_;
 };
 
 // WebKit client used in DLL.
@@ -89,7 +99,7 @@ extern "C" {
 WebWorker* API_CALL CreateWebWorker(WebWorkerClient* webworker_client,
                                     TestWebWorkerHelper* webworker_helper) {
   if (!WebKit::webKitClient()) {
-    webkit_client.reset(new WorkerWebKitClientImpl());
+    webkit_client.reset(new WorkerWebKitClientImpl(webworker_helper));
     WebKit::initialize(webkit_client.get());
   }