Refactor WebThreadImpl, WebSubThread, and WebMainLoop.

WebThreadImpl is now merely a scoped object that binds a provided
SingleThreadTaskRunner to a WebThread::ID. It no longer subclasses
base::Thread. That functionality has been moved to WebSubThread, which is
effectively only used for the IO thread now.

This is the //web implementation of
https://chromium-review.googlesource.com/c/chromium/src/+/980793. See the
description of that CL for more details.

BUG=826465

Change-Id: I636051b154c4156bc32517d649081131f9bdd14d
Reviewed-on: https://chromium-review.googlesource.com/c/1383140
Commit-Queue: Rohit Rao <[email protected]>
Reviewed-by: Gabriel Charette <[email protected]>
Reviewed-by: Sylvain Defresne <[email protected]>
Cr-Commit-Position: refs/heads/master@{#618761}

Author: Rohit Rao

base/threading/thread_restrictions.h

@@ -234,6 +234,10 @@ namespace vr {
 class VrShell;
 }
 
+namespace web {
+class WebSubThread;
+}
+
 namespace webrtc {
 class DesktopConfigurationMonitor;
 }
@@ -317,6 +321,7 @@ class BASE_EXPORT ScopedAllowBlocking {
   friend class mojo::CoreLibraryInitializer;
   friend class resource_coordinator::TabManagerDelegate;  // crbug.com/778703
   friend class ui::MaterialDesignController;
+  friend class web::WebSubThread;
   friend class StackSamplingProfiler;
 
   ScopedAllowBlocking() EMPTY_BODY_IF_DCHECK_IS_OFF;

ios/components/io_thread/DEPS

@@ -7,4 +7,8 @@ include_rules = [
   "+components/version_info",
   "+ios/web/public",
   "+net",
+
+  # Temporarily allow WebThreadImpl until TaskExecutor functionality is moved
+  # into a new file in ios/web/public.
+  "+ios/web/web_thread_impl.h"
 ]

ios/components/io_thread/ios_io_thread_unittest.mm

@@ -4,16 +4,16 @@
 
 #include "ios/components/io_thread/ios_io_thread.h"
 
-#include "base/message_loop/message_loop.h"
-#include "base/run_loop.h"
-#include "components/prefs/pref_registry_simple.h"
-#include "components/prefs/pref_service.h"
-#include "components/prefs/pref_service_factory.h"
-#include "components/prefs/testing_pref_store.h"
+#include <memory>
+
+#include "base/test/scoped_task_environment.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/proxy_config/pref_proxy_config_tracker_impl.h"
-#include "ios/web/public/test/test_web_thread_bundle.h"
-#include "net/test/url_request/url_request_failed_job.h"
-#include "net/url_request/url_request_filter.h"
+#import "ios/web/public/test/fakes/test_web_client.h"
+#include "ios/web/public/test/scoped_testing_web_client.h"
+#include "ios/web/public/test/test_web_thread.h"
+#include "ios/web/web_thread_impl.h"
+#include "net/base/network_delegate.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/gtest_mac.h"
 #include "testing/platform_test.h"
@@ -42,32 +42,43 @@
 
 class IOSIOThreadTest : public PlatformTest {
  public:
-  IOSIOThreadTest() : thread_bundle_(web::TestWebThreadBundle::IO_MAINLOOP) {
-    net::URLRequestFailedJob::AddUrlHandler();
+  IOSIOThreadTest() : web_client_(std::make_unique<web::TestWebClient>()) {
+    web::WebThreadImpl::CreateTaskExecutor();
+
+    ui_thread_ = std::make_unique<web::TestWebThread>(
+        web::WebThread::UI, scoped_task_environment_.GetMainThreadTaskRunner());
   }
 
   ~IOSIOThreadTest() override {
-    net::URLRequestFilter::GetInstance()->ClearHandlers();
+    web::WebThreadImpl::ResetTaskExecutorForTesting();
   }
 
- private:
-  web::TestWebThreadBundle thread_bundle_;
+ protected:
+  base::test::ScopedTaskEnvironment scoped_task_environment_;
+  web::ScopedTestingWebClient web_client_;
+  std::unique_ptr<web::TestWebThread> ui_thread_;
 };
 
 // Tests the creation of an IOSIOThread and verifies that it returns a system
 // url request context.
 TEST_F(IOSIOThreadTest, AssertSystemUrlRequestContext) {
-  PrefServiceFactory pref_service_factory;
-  pref_service_factory.set_user_prefs(base::MakeRefCounted<TestingPrefStore>());
+  std::unique_ptr<TestingPrefServiceSimple> pref_service(
+      std::make_unique<TestingPrefServiceSimple>());
+  PrefProxyConfigTrackerImpl::RegisterPrefs(pref_service->registry());
 
-  scoped_refptr<PrefRegistrySimple> pref_registry = new PrefRegistrySimple;
-  PrefProxyConfigTrackerImpl::RegisterPrefs(pref_registry.get());
+  // Create the IO thread but do not register it yet.
+  std::unique_ptr<web::TestWebThread> io_thread(
+      std::make_unique<web::TestWebThread>(web::WebThread::IO));
+  io_thread->StartIOThreadUnregistered();
 
-  std::unique_ptr<PrefService> pref_service(
-      pref_service_factory.Create(pref_registry.get()));
-
-  std::unique_ptr<TestIOThread> test_io_thread(
+  // Create the TestIOThread before the IO thread is registered.
+  std::unique_ptr<TestIOThread> ios_io_thread(
       new TestIOThread(pref_service.get(), nullptr));
+  io_thread->RegisterAsWebThread();
+
+  ASSERT_TRUE(ios_io_thread->system_url_request_context_getter());
 
-  ASSERT_TRUE(test_io_thread->system_url_request_context_getter());
+  // Explicitly destroy the IO thread so that it is unregistered before the
+  // TestIOThread is destroyed.
+  io_thread.reset();
 }

ios/web/app/web_main_loop.h

@@ -83,7 +83,7 @@ class WebMainLoop {
   // This must get destroyed after other threads that are created in parts_.
   std::unique_ptr<WebThreadImpl> main_thread_;
 
-  // Members initialized in |RunMainMessageLoopParts()| ------------------------
+  // Members initialized in |CreateThreads()| ------------------------
   std::unique_ptr<WebSubThread> io_thread_;
 
   // Members initialized in |WebThreadsStarted()| --------------------------

ios/web/app/web_main_loop.mm

@@ -130,7 +130,9 @@
   base::Thread::Options io_message_loop_options;
   io_message_loop_options.message_loop_type = base::MessageLoop::TYPE_IO;
   io_thread_ = std::make_unique<WebSubThread>(WebThread::IO);
-  io_thread_->StartWithOptions(io_message_loop_options);
+  if (!io_thread_->StartWithOptions(io_message_loop_options))
+    LOG(FATAL) << "Failed to start WebThread::IO";
+  io_thread_->RegisterAsWebThread();
 
   // Only start IO thread above as this is the only WebThread besides UI (which
   // is the main thread).
@@ -196,8 +198,10 @@
   base::PlatformThread::SetName("CrWebMain");
 
   // Register the main thread by instantiating it, but don't call any methods.
+  DCHECK(base::ThreadTaskRunnerHandle::IsSet());
   main_thread_.reset(new WebThreadImpl(
-      WebThread::UI, ios_global_state::GetMainThreadMessageLoop()));
+      WebThread::UI,
+      ios_global_state::GetMainThreadMessageLoop()->task_runner()));
 }
 
 int WebMainLoop::WebThreadsStarted() {

ios/web/public/test/test_web_thread.h

@@ -12,18 +12,29 @@
 
 namespace base {
 class MessageLoop;
+class Thread;
 }
 
 namespace web {
 
-class TestWebThreadImpl;
+class WebSubThread;
+class WebThreadImpl;
 
 // DEPRECATED: use TestWebThreadBundle instead.
 // A WebThread for unit tests; this lets unit tests outside of web create
 // WebThread instances.
 class TestWebThread {
  public:
+  // Constructs a TestWebThread with a |real_thread_| and starts it (with a
+  // MessageLoopForIO if |identifier == WebThread::IO|).
   explicit TestWebThread(WebThread::ID identifier);
+
+  // Constructs a TestWebThread "running" on |thread_runner| (no
+  // |real_thread_|).
+  TestWebThread(WebThread::ID identifier,
+                scoped_refptr<base::SingleThreadTaskRunner> thread_runner);
+
+  // Constructs a TestWebThread based on |message_loop| (no |real_thread_|).
   TestWebThread(WebThread::ID identifier, base::MessageLoop* message_loop);
 
   ~TestWebThread();
@@ -33,22 +44,31 @@ class TestWebThread {
   // WebThread, do no provide the full Thread interface.
 
   // Starts the thread with a generic message loop.
-  bool Start();
+  void Start();
 
   // Starts the thread with an IOThread message loop.
-  bool StartIOThread();
+  void StartIOThread();
+
+  // Together these are the same as StartIOThread(). They can be called in
+  // phases to test binding WebThread::IO after its underlying thread was
+  // started.
+  void StartIOThreadUnregistered();
+  void RegisterAsWebThread();
 
   // Stops the thread.
   void Stop();
 
-  // Returns true if the thread is running.
-  bool IsRunning();
-
  private:
-  std::unique_ptr<TestWebThreadImpl> impl_;
-
   const WebThread::ID identifier_;
 
+  // A real thread which represents |identifier_| when constructor #1 is used
+  // (null otherwise).
+  std::unique_ptr<WebSubThread> real_thread_;
+
+  // Binds |identifier_| to |message_loop| when constructor #2 is used (null
+  // otherwise).
+  std::unique_ptr<WebThreadImpl> fake_thread_;
+
   DISALLOW_COPY_AND_ASSIGN(TestWebThread);
 };
 

ios/web/public/web_thread.h

@@ -95,9 +95,9 @@ class WebThread {
   // Only one delegate may be registered at a time. Delegates may be
   // unregistered by providing a nullptr pointer.
   //
-  // If the caller unregisters a delegate before CleanUp has been
-  // called, it must perform its own locking to ensure the delegate is
-  // not deleted while unregistering.
+  // The delegate can only be registered through this call before
+  // WebThreadImpl(WebThread::IO) is created and unregistered after
+  // it was destroyed and its underlying thread shutdown.
   static void SetIOThreadDelegate(WebThreadDelegate* delegate);
 
   // Returns an appropriate error message for when DCHECK_CURRENTLY_ON() fails.

ios/web/public/web_thread_delegate.h

@@ -13,14 +13,17 @@ namespace web {
 // If registered as such, it will schedule to run Init() before the
 // message loop begins, and receive a CleanUp() call right after the message
 // loop ends (and before the WebThread has done its own clean-up).
+
+// A delegate for //web embedders to perform extra initialization/cleanup on
+// WebThread::IO.
 class WebThreadDelegate {
  public:
   virtual ~WebThreadDelegate() {}
 
-  // Called prior to starting the message loop
+  // Called prior to completing initialization of WebThread::IO.
   virtual void Init() = 0;
 
-  // Called just after the message loop ends.
+  // Called during teardown of WebThread::IO.
   virtual void CleanUp() = 0;
 };
 

ios/web/test/test_web_thread.cc

@@ -6,41 +6,36 @@
 
 #include "base/macros.h"
 #include "base/message_loop/message_loop.h"
+#include "ios/web/web_sub_thread.h"
 #include "ios/web/web_thread_impl.h"
 
 namespace web {
 
-class TestWebThreadImpl : public WebThreadImpl {
- public:
-  TestWebThreadImpl(WebThread::ID identifier) : WebThreadImpl(identifier) {}
-
-  TestWebThreadImpl(WebThread::ID identifier, base::MessageLoop* message_loop)
-      : WebThreadImpl(identifier, message_loop) {}
-
-  ~TestWebThreadImpl() override { Stop(); }
-
- private:
-  DISALLOW_COPY_AND_ASSIGN(TestWebThreadImpl);
-};
-
 TestWebThread::TestWebThread(WebThread::ID identifier)
-    : impl_(new TestWebThreadImpl(identifier)), identifier_(identifier) {}
+    : identifier_(identifier),
+      real_thread_(std::make_unique<WebSubThread>(identifier_)) {
+  real_thread_->AllowBlockingForTesting();
+}
+
+TestWebThread::TestWebThread(
+    WebThread::ID identifier,
+    scoped_refptr<base::SingleThreadTaskRunner> thread_runner)
+    : identifier_(identifier),
+      fake_thread_(new WebThreadImpl(identifier_, thread_runner)) {}
 
 TestWebThread::TestWebThread(WebThread::ID identifier,
                              base::MessageLoop* message_loop)
-    : impl_(new TestWebThreadImpl(identifier, message_loop)),
-      identifier_(identifier) {}
+    : TestWebThread(identifier, message_loop->task_runner()) {}
 
 TestWebThread::~TestWebThread() {
   // The upcoming WebThreadImpl::ResetGlobalsForTesting() call requires that
-  // |impl_| have triggered the shutdown phase for its WebThread::ID. This
-  // either happens when the thread is stopped (if real) or destroyed (when fake
-  // -- i.e. using an externally provided MessageLoop).
-  impl_.reset();
+  // |identifier_| completed its shutdown phase.
+  real_thread_.reset();
+  fake_thread_.reset();
 
-  // Resets WebThreadImpl's globals so that |impl_| is no longer bound to
-  // |identifier_|. This is fine since the underlying MessageLoop has already
-  // been flushed and deleted in Stop(). In the case of an externally provided
+  // Resets WebThreadImpl's globals so that |identifier_| is no longer
+  // bound. This is fine since the underlying MessageLoop has already been
+  // flushed and deleted above. In the case of an externally provided
   // MessageLoop however, this means that TaskRunners obtained through
   // |WebThreadImpl::GetTaskRunnerForThread(identifier_)| will no longer
   // recognize their WebThreadImpl for RunsTasksInCurrentSequence(). This
@@ -54,22 +49,29 @@ TestWebThread::~TestWebThread() {
   WebThreadImpl::ResetGlobalsForTesting(identifier_);
 }
 
-bool TestWebThread::Start() {
-  return impl_->Start();
+void TestWebThread::Start() {
+  CHECK(real_thread_->Start());
+  RegisterAsWebThread();
+}
+
+void TestWebThread::StartIOThread() {
+  StartIOThreadUnregistered();
+  RegisterAsWebThread();
 }
 
-bool TestWebThread::StartIOThread() {
+void TestWebThread::StartIOThreadUnregistered() {
   base::Thread::Options options;
   options.message_loop_type = base::MessageLoop::TYPE_IO;
-  return impl_->StartWithOptions(options);
+  CHECK(real_thread_->StartWithOptions(options));
 }
 
-void TestWebThread::Stop() {
-  impl_->Stop();
+void TestWebThread::RegisterAsWebThread() {
+  real_thread_->RegisterAsWebThread();
 }
 
-bool TestWebThread::IsRunning() {
-  return impl_->IsRunning();
+void TestWebThread::Stop() {
+  if (real_thread_)
+    real_thread_->Stop();
 }
 
 }  // namespace web