Simplify the WebKit thread model. It's now created/destroyed on the UI thread (before/after the IO thread is started/stopped). The WebKit thread is created lazily as needed (while on the IO thread).TEST=noneBUG=none

Review URL: http://codereview.chromium.org/149238

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

5 files changed, 59 insertions, 123 deletions

diff --git a/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc b/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc
index cbbf9ff..1ea105c 100644
--- a/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc
+++ b/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.cc
@@ -16,17 +16,12 @@ DOMStorageDispatcherHost::DOMStorageDispatcherHost(
       webkit_thread_(webkit_thread),
       message_sender_(message_sender) {
   DCHECK(webkit_context_.get());
-  DCHECK(webkit_thread_.get());
+  DCHECK(webkit_thread_);
   DCHECK(message_sender_);
 }
 
 DOMStorageDispatcherHost::~DOMStorageDispatcherHost() {
-  DCHECK(!message_sender_);
-}
-
-void DOMStorageDispatcherHost::Shutdown() {
-  DCHECK(IsOnIOThread());
-  AutoLock lock(message_sender_lock_);
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
   message_sender_ = NULL;
 }
 
@@ -37,34 +32,21 @@ bool DOMStorageDispatcherHost::OnMessageReceived(const IPC::Message& msg) {
 }
 
 void DOMStorageDispatcherHost::Send(IPC::Message* message) {
-  if (IsOnIOThread()) {
-    if (message_sender_)
-      message_sender_->Send(message);
-    else
-      delete message;
-  }
-
-  // If message_sender_ is NULL, the IO thread has either gone away
-  // or will do so soon.  By holding this lock until we finish posting to the
-  // thread, we block the IO thread from completely shutting down benieth us.
-  AutoLock lock(message_sender_lock_);
   if (!message_sender_) {
     delete message;
     return;
   }
 
+  if (ChromeThread::CurrentlyOn(ChromeThread::IO)) {
+    message_sender_->Send(message);
+    return;
+  }
+
+  // The IO thread can't dissapear while the WebKit thread is still running.
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::WEBKIT));
   MessageLoop* io_loop = ChromeThread::GetMessageLoop(ChromeThread::IO);
   CancelableTask* task = NewRunnableMethod(this,
                                            &DOMStorageDispatcherHost::Send,
                                            message);
   io_loop->PostTask(FROM_HERE, task);
 }
-
-bool DOMStorageDispatcherHost::IsOnIOThread() const {
-  MessageLoop* io_loop = ChromeThread::GetMessageLoop(ChromeThread::IO);
-  return MessageLoop::current() == io_loop;
-}
-
-bool DOMStorageDispatcherHost::IsOnWebKitThread() const {
-  return MessageLoop::current() == webkit_thread_->GetMessageLoop();
-}
diff --git a/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h b/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h
index a7b815f..a495d97 100644
--- a/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h
+++ b/chrome/browser/in_process_webkit/dom_storage_dispatcher_host.h
@@ -6,7 +6,6 @@
 #define CHROME_BROWSER_IN_PROCESS_WEBKIT_DOM_STORAGE_DISPATCHER_HOST_H_
 
 #include "base/ref_counted.h"
-#include "base/thread.h"
 #include "chrome/common/ipc_message.h"
 
 class WebKitContext;
@@ -15,7 +14,7 @@ class WebKitThread;
 // This class handles the logistics of DOM Storage within the browser process.
 // It mostly ferries information between IPCs and the WebKit implementations,
 // but it also handles some special cases like when renderer processes die.
-// THIS CLASS MUST NOT BE DESTROYED ON THE WEBKIT THREAD (for now).
+// THIS CLASS MUST NOT BE DESTROYED ON THE WEBKIT THREAD.
 class DOMStorageDispatcherHost :
     public base::RefCountedThreadSafe<DOMStorageDispatcherHost> {
  public:
@@ -23,10 +22,6 @@ class DOMStorageDispatcherHost :
   DOMStorageDispatcherHost(IPC::Message::Sender* message_sender,
                            WebKitContext*, WebKitThread*);
 
-  // Only call Shutdown from the IO thread.  Shutdown warns us that we're going
-  // to go away soon and tells us not to send anything else to the IO thread.
-  void Shutdown();
-
   // Only call from IO thread.
   bool OnMessageReceived(const IPC::Message& message);
 
@@ -38,18 +33,13 @@ class DOMStorageDispatcherHost :
   friend class base::RefCountedThreadSafe<DOMStorageDispatcherHost>;
   ~DOMStorageDispatcherHost();
 
-  // Obviously can be called from any thread.
-  bool IsOnIOThread() const;
-  bool IsOnWebKitThread() const;
-
-  // Are immutable and are always valid throughout the lifetime of the object.
+  // Data shared between renderer processes with the same profile.
   scoped_refptr<WebKitContext> webkit_context_;
-  scoped_refptr<WebKitThread> webkit_thread_;
 
-  // We keep the message_sender_ pointer for sending messages.  All access
-  // to the message_sender_ (and the IO thread in general) should be done under
-  // this lock and only if message_sender_ is non-NULL.
-  Lock message_sender_lock_;
+  // ResourceDispatcherHost takes care of destruction.  Immutable.
+  WebKitThread* webkit_thread_;
+
+  // Only set on the IO thread.
   IPC::Message::Sender* message_sender_;
 
   DISALLOW_IMPLICIT_CONSTRUCTORS(DOMStorageDispatcherHost);
diff --git a/chrome/browser/in_process_webkit/webkit_thread.cc b/chrome/browser/in_process_webkit/webkit_thread.cc
index 55772c6..19cf65b 100644
--- a/chrome/browser/in_process_webkit/webkit_thread.cc
+++ b/chrome/browser/in_process_webkit/webkit_thread.cc
@@ -4,21 +4,23 @@
 
 #include "chrome/browser/in_process_webkit/webkit_thread.h"
 
+#include "base/command_line.h"
 #include "chrome/browser/in_process_webkit/browser_webkitclient_impl.h"
+#include "chrome/common/chrome_switches.h"
 #include "webkit/api/public/WebKit.h"
 
-base::LazyInstance<Lock> WebKitThread::global_webkit_lock_(
-    base::LINKER_INITIALIZED);
-int WebKitThread::global_webkit_ref_count_ = 0;
-WebKitThread::InternalWebKitThread* WebKitThread::global_webkit_thread_ = NULL;
+// This happens on the UI thread before the IO thread has been shut down.
+WebKitThread::WebKitThread() {
+  // The thread is started lazily by InitializeThread() on the IO thread.
+}
 
-WebKitThread::WebKitThread()
-    : cached_webkit_thread_(NULL) {
-  // The thread is started lazily by InitializeThread().
+// This happens on the UI thread after the IO thread has been shut down.
+WebKitThread::~WebKitThread() {
+  DCHECK(!ChromeThread::CurrentlyOn(ChromeThread::WEBKIT));
 }
 
 WebKitThread::InternalWebKitThread::InternalWebKitThread()
-    : base::Thread("WebKit"),
+    : ChromeThread(ChromeThread::WEBKIT),
       webkit_client_(NULL) {
 }
 
@@ -28,42 +30,24 @@ void WebKitThread::InternalWebKitThread::Init() {
   DCHECK(webkit_client_);
   WebKit::initialize(webkit_client_);
   // Don't do anything heavyweight here since this can block the IO thread from
-  // executing (since InitializeThread() is often called on the IO thread).
+  // executing (since InitializeThread() is called on the IO thread).
 }
 
 void WebKitThread::InternalWebKitThread::CleanUp() {
+  // Don't do anything heavyweight here since this can block the IO thread from
+  // executing (since the thread is shutdown from the IO thread).
   DCHECK(webkit_client_);
   WebKit::shutdown();
   delete webkit_client_;
-  webkit_client_ = NULL;
 }
 
-WebKitThread::~WebKitThread() {
-  AutoLock lock(global_webkit_lock_.Get());
-  if (cached_webkit_thread_) {
-    DCHECK(global_webkit_ref_count_ > 0);
-    if (--global_webkit_ref_count_ == 0) {
-      // TODO(jorlow): Make this safe.
-      DCHECK(MessageLoop::current() != global_webkit_thread_->message_loop());
-      global_webkit_thread_->Stop();
-      delete global_webkit_thread_;
-      global_webkit_thread_ = NULL;
-    }
-  }
-}
+MessageLoop* WebKitThread::InitializeThread() {
+  if (CommandLine::ForCurrentProcess()->HasSwitch(switches::kSingleProcess))
+    return NULL;
 
-void WebKitThread::InitializeThread() {
-  AutoLock lock(global_webkit_lock_.Get());
-  if (!cached_webkit_thread_) {
-    if (!global_webkit_thread_) {
-      global_webkit_thread_ = new InternalWebKitThread;
-      DCHECK(global_webkit_thread_);
-      bool started = global_webkit_thread_->Start();
-      DCHECK(started);
-    }
-    ++global_webkit_ref_count_;
-    // The cached version can be accessed outside of global_webkit_lock_.
-    cached_webkit_thread_ = global_webkit_thread_;
-  }
-  DCHECK(cached_webkit_thread_->IsRunning());
+  DCHECK(!webkit_thread_.get());
+  webkit_thread_.reset(new InternalWebKitThread);
+  bool started = webkit_thread_->Start();
+  DCHECK(started);
+  return webkit_thread_->message_loop();
 }
diff --git a/chrome/browser/in_process_webkit/webkit_thread.h b/chrome/browser/in_process_webkit/webkit_thread.h
index 518fa66..49ad7ac 100644
--- a/chrome/browser/in_process_webkit/webkit_thread.h
+++ b/chrome/browser/in_process_webkit/webkit_thread.h
@@ -10,36 +10,34 @@
 #include "base/logging.h"
 #include "base/ref_counted.h"
 #include "base/thread.h"
+#include "chrome/browser/chrome_thread.h"
 
 class BrowserWebKitClientImpl;
 
 // This is an object that represents WebKit's "main" thread within the browser
-// process.  You can create as many instances of this class as you'd like;
-// they'll all point to the same thread and you're guaranteed they'll
-// initialize in a thread-safe way, though WebKitThread instances should
-// probably be shared when it's easy to do so.  The first time you call
-// GetMessageLoop() or EnsureWebKitInitialized() the thread will be created
-// and WebKit initialized.  When the last instance of WebKitThread is
-// destroyed, WebKit is shut down and the thread is stopped.
-// THIS CLASS MUST NOT BE DEREFED TO 0 ON THE WEBKIT THREAD (for now).
-class WebKitThread : public base::RefCountedThreadSafe<WebKitThread> {
+// process.  It should be instantiated and destroyed on the UI thread
+// before/after the IO thread is created/destroyed.  All other usage should be
+// on the IO thread.  If the browser is being run in --single-process mode, a
+// thread will never be spun up, and GetMessageLoop() will always return NULL.
+class WebKitThread {
  public:
+  // Called from the UI thread.
   WebKitThread();
+  ~WebKitThread();
 
+  // Returns the message loop for the WebKit thread unless we're in
+  // --single-processuntil mode, in which case it'll return NULL.  Only call
+  // from the IO thread.  Only do fast-path work here.
   MessageLoop* GetMessageLoop() {
-    if (!cached_webkit_thread_)
-      InitializeThread();
-    return cached_webkit_thread_->message_loop();
-  }
-
-  void EnsureWebKitInitialized() {
-    if (!cached_webkit_thread_)
-      InitializeThread();
+    DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
+    if (!webkit_thread_.get())
+      return InitializeThread();
+    return webkit_thread_->message_loop();
   }
 
  private:
   // Must be private so that we can carefully control its lifetime.
-  class InternalWebKitThread : public base::Thread {
+  class InternalWebKitThread : public ChromeThread {
    public:
     InternalWebKitThread();
     virtual ~InternalWebKitThread() { }
@@ -52,22 +50,13 @@ class WebKitThread : public base::RefCountedThreadSafe<WebKitThread> {
     BrowserWebKitClientImpl* webkit_client_;
   };
 
-  friend class base::RefCountedThreadSafe<WebKitThread>;
-  ~WebKitThread();
-
-  void InitializeThread();
-
-  // If this is set, then this object has incremented the global WebKit ref
-  // count and will shutdown the thread if it sees the ref count go to 0.
-  // It's assumed that once this is non-NULL, the pointer will be valid until
-  // destruction.
-  InternalWebKitThread* cached_webkit_thread_;
+  // Returns the WebKit thread's message loop or NULL if we're in
+  // --single-process mode.  Do slow-path initialization work here.
+  MessageLoop* InitializeThread();
 
-  // If there are multiple WebKitThread object (should only be possible in
-  // unittests at the moment), make sure they all share one real thread.
-  static base::LazyInstance<Lock> global_webkit_lock_;
-  static int global_webkit_ref_count_;
-  static InternalWebKitThread* global_webkit_thread_;
+  // Pointer to the actual WebKitThread.  NULL if not yet started.  Only modify
+  // from the IO thread while the WebKit thread is not running.
+  scoped_ptr<InternalWebKitThread> webkit_thread_;
 
   DISALLOW_COPY_AND_ASSIGN(WebKitThread);
 };
diff --git a/chrome/browser/in_process_webkit/webkit_thread_unittest.cc b/chrome/browser/in_process_webkit/webkit_thread_unittest.cc
index f66a26c..1f19963 100644
--- a/chrome/browser/in_process_webkit/webkit_thread_unittest.cc
+++ b/chrome/browser/in_process_webkit/webkit_thread_unittest.cc
@@ -5,13 +5,4 @@
 #include "chrome/browser/in_process_webkit/webkit_thread.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
-// This is important because if there are 2 different message loops, we must
-// have 2 different WebKit threads which would be very bad.
-TEST(WebKitThreadTest, TwoThreadsShareMessageLoopTest) {
-  scoped_refptr<WebKitThread> thread_a = new WebKitThread;
-  scoped_refptr<WebKitThread> thread_b = new WebKitThread;
-  MessageLoop* loop_a = thread_a->GetMessageLoop();
-  MessageLoop* loop_b = thread_b->GetMessageLoop();
-  ASSERT_FALSE(loop_a == NULL);
-  ASSERT_EQ(loop_a, loop_b);
-}
+// TODO(jorlow): Write some tests. http://crbug.com/16155