Simplify the WebKit thread model. It's now created/destroyed on the UI thread...

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

chrome/browser/chrome_thread.cc

@@ -9,6 +9,7 @@ static const char* chrome_thread_names[ChromeThread::ID_COUNT] = {
   "Chrome_IOThread",  // IO
   "Chrome_FileThread",  // FILE
   "Chrome_DBThread",  // DB
+  "Chrome_WebKitThread",  // WEBKIT
   "Chrome_HistoryThread",  // HISTORY
 #if defined(OS_LINUX)
   "Chrome_Background_X11Thread",  // BACKGROUND_X11
@@ -21,6 +22,7 @@ ChromeThread* ChromeThread::chrome_threads_[ID_COUNT] = {
   NULL,  // IO
   NULL,  // FILE
   NULL,  // DB
+  NULL,  // WEBKIT
   NULL,  // HISTORY
 #if defined(OS_LINUX)
   NULL,  // BACKGROUND_X11
@@ -51,3 +53,13 @@ MessageLoop* ChromeThread::GetMessageLoop(ID identifier) {
 
   return NULL;
 }
+
+// static
+bool ChromeThread::CurrentlyOn(ID identifier) {
+  // MessageLoop::current() will return NULL if none is running.  This is often
+  // true when running under unit tests.  This behavior actually works out
+  // pretty convienently (as is mentioned in the header file comment), but it's
+  // worth noting here.
+  MessageLoop* message_loop = GetMessageLoop(identifier);
+  return MessageLoop::current() == message_loop;
+}

chrome/browser/chrome_thread.h

@@ -38,6 +38,10 @@ class ChromeThread : public base::Thread {
     // This is the thread that interacts with the database.
     DB,
 
+    // This is the "main" thread for WebKit within the browser process when
+    // NOT in --single-process mode.
+    WEBKIT,
+
     // This is the thread that interacts with the history database.
     HISTORY,
 
@@ -68,6 +72,16 @@ class ChromeThread : public base::Thread {
   //
   static MessageLoop* GetMessageLoop(ID identifier);
 
+  // Callable on any thread.  Returns whether you're currently on a particular
+  // thread.
+  //
+  // WARNING:
+  //   When running under unit-tests, this will return true if you're on the
+  //   main thread and the thread ID you pass in isn't running.  This is
+  //   normally the correct behavior because you want to ignore these asserts
+  //   unless you've specifically spun up the threads, but be mindful of it.
+  static bool CurrentlyOn(ID identifier);
+
  private:
   // The identifier of this thread.  Only one thread can exist with a given
   // identifier at a given time.

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

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

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

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

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

chrome/browser/renderer_host/resource_dispatcher_host.h

@@ -292,7 +292,7 @@ class ResourceDispatcherHost : public URLRequest::Delegate {
   }
 
   WebKitThread* webkit_thread() const {
-    return webkit_thread_;
+    return webkit_thread_.get();
   }
 
   MessageLoop* ui_loop() const { return ui_loop_; }
@@ -529,7 +529,8 @@ class ResourceDispatcherHost : public URLRequest::Delegate {
 
   scoped_refptr<SafeBrowsingService> safe_browsing_;
 
-  scoped_refptr<WebKitThread> webkit_thread_;
+  // We own the WebKit thread and see to its destruction.
+  scoped_ptr<WebKitThread> webkit_thread_;
 
   // Request ID for browser initiated requests. request_ids generated by
   // child processes are counted up from 0, while browser created requests

chrome/browser/renderer_host/resource_message_filter.cc

@@ -175,10 +175,7 @@ ResourceMessageFilter::ResourceMessageFilter(
 
 ResourceMessageFilter::~ResourceMessageFilter() {
   // This function should be called on the IO thread.
-  DCHECK(MessageLoop::current() ==
-         ChromeThread::GetMessageLoop(ChromeThread::IO));
-
-  dom_storage_dispatcher_host_->Shutdown();
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   // Let interested observers know we are being deleted.
   NotificationService::current()->Notify(
@@ -452,8 +449,7 @@ void ResourceMessageFilter::OnGetDataDir(std::wstring* data_dir) {
 
 void ResourceMessageFilter::OnPluginMessage(const FilePath& plugin_path,
                                             const std::vector<uint8>& data) {
-  DCHECK(MessageLoop::current() ==
-         ChromeThread::GetMessageLoop(ChromeThread::IO));
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   ChromePluginLib *chrome_plugin = ChromePluginLib::Find(plugin_path);
   if (chrome_plugin) {
@@ -466,8 +462,7 @@ void ResourceMessageFilter::OnPluginMessage(const FilePath& plugin_path,
 void ResourceMessageFilter::OnPluginSyncMessage(const FilePath& plugin_path,
                                                 const std::vector<uint8>& data,
                                                 std::vector<uint8> *retval) {
-  DCHECK(MessageLoop::current() ==
-         ChromeThread::GetMessageLoop(ChromeThread::IO));
+  DCHECK(ChromeThread::CurrentlyOn(ChromeThread::IO));
 
   ChromePluginLib *chrome_plugin = ChromePluginLib::Find(plugin_path);
   if (chrome_plugin) {