ServiceWorker: Run functions in the browsertest on appropriate thread

content/browser/service_worker/service_worker_browsertest.cc

 // Copyright 2014 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include <map>
 #include <memory>
+#include <mutex>
 #include <utility>
 
 #include "base/bind.h"
 #include "base/callback.h"
 #include "base/command_line.h"
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
 #include "base/memory/ref_counted.h"
 #include "base/run_loop.h"
 #include "base/single_thread_task_runner.h"
@@ skipping 355 matching lines @@
  private:
   scoped_refptr<ServiceWorkerContextWrapper> wrapper_;
 };
 
 class ConsoleListener : public EmbeddedWorkerInstance::Listener {
  public:
   void OnReportConsoleMessage(int source_identifier,
                               int message_level,
                               const base::string16& message,
                               int line_number,
                               const GURL& source_url) override {

[nhiroki, 2016/08/02 09:15:43]

Can you add DCHECK_CURRENTLY_ON(BrowserThread::IO)?

[shimazu, 2016/08/02 23:34:49]

Done.

+    std::lock_guard<std::mutex> lock(mutex_);
     messages_.push_back(message);
     if (!quit_.is_null() && messages_.size() == expected_message_count_) {
       BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, quit_);
       quit_.Reset();
     }

[nhiroki, 2016/08/02 09:15:43]

How about running these operations on the UI thread instead of using a lock?

void OnReportConsoleMessageOnUI(const base::string16& message) {
  DCHECK_CURRENTLY_ON(BrowserThread::UI);
  messages_.push_back(message);
  if (!quit_.is_null() && messages_.size() == expected_message_count_) {
    base::Closure quit = quit_;
    quit_.Reset();
    quit.Run();
  }
}

// In OnReportConsoleMessage()
BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, this,
&ConsoleListener::OnReportConsoleMessageOnUI);

[nhiroki, 2016/08/02 09:16:32]

BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, this,
&ConsoleListener::OnReportConsoleMessageOnUI, message);

[shimazu, 2016/08/02 23:34:49]

Done.

   }
 
   void WaitForConsoleMessages(size_t expected_message_count) {
-    if (messages_.size() >= expected_message_count)
-      return;
+    base::RunLoop console_run_loop;
+    {
+      std::lock_guard<std::mutex> lock(mutex_);
+      if (messages_.size() >= expected_message_count)
+        return;
 
-    expected_message_count_ = expected_message_count;
-    base::RunLoop console_run_loop;
-    quit_ = console_run_loop.QuitClosure();
+      expected_message_count_ = expected_message_count;
+      quit_ = console_run_loop.QuitClosure();
+    }
+
     console_run_loop.Run();
 
-    ASSERT_EQ(messages_.size(), expected_message_count);
+    {
+      std::lock_guard<std::mutex> lock(mutex_);
+      ASSERT_EQ(messages_.size(), expected_message_count);
+    }
   }
 
   bool OnMessageReceived(const IPC::Message& message) override { return false; }
   const std::vector<base::string16>& messages() const { return messages_; }
 
  private:
   std::vector<base::string16> messages_;
+  std::mutex mutex_;
   size_t expected_message_count_;
   base::Closure quit_;

[nhiroki, 2016/08/02 09:15:42]

We may want to add a comment like "// These must be accessed on the IO thread.".

[shimazu, 2016/08/02 23:34:49]

Done.

 };
 
 class ServiceWorkerVersionBrowserTest : public ServiceWorkerBrowserTest {
  public:
   using self = ServiceWorkerVersionBrowserTest;
 
   ~ServiceWorkerVersionBrowserTest() override {}
 
   void TearDownOnIOThread() override {
     registration_ = NULL;
@@ skipping 501 matching lines @@
   ActivateTestHelper("/service_worker/worker_activate_rejected.js",
                      SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED);
 }
 
 IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest,
                        InstallWithWaitUntil_Rejected) {
   InstallTestHelper("/service_worker/worker_install_rejected.js",
                     SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED);
 }
 
-// Has errors under TSan. See https://crbug.com/631323.
-#if defined(THREAD_SANITIZER)
-#define MAYBE_InstallWithWaitUntil_RejectConsoleMessage \
-  DISABLED_InstallWithWaitUntil_RejectConsoleMessage
-#else
-#define MAYBE_InstallWithWaitUntil_RejectConsoleMessage \
-  InstallWithWaitUntil_RejectConsoleMessage
-#endif
 IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest,
-                       MAYBE_InstallWithWaitUntil_RejectConsoleMessage) {
+                       InstallWithWaitUntil_RejectConsoleMessage) {
   RunOnIOThread(base::Bind(&self::SetUpRegistrationOnIOThread, this,
                            "/service_worker/worker_install_rejected.js"));
 
   ConsoleListener console_listener;
-  version_->embedded_worker()->AddListener(&console_listener);
+  RunOnIOThread(base::Bind(&EmbeddedWorkerInstance::AddListener,
+                           base::Unretained(version_->embedded_worker()),
+                           &console_listener));
 
   // Dispatch install on a worker.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
   base::RunLoop install_run_loop;
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(&self::InstallOnIOThread, this,
                                      install_run_loop.QuitClosure(), &status));
   install_run_loop.Run();
   ASSERT_EQ(SERVICE_WORKER_ERROR_EVENT_WAITUNTIL_REJECTED, status);
 
   const base::string16 expected =
       base::ASCIIToUTF16("Rejecting oninstall event");
   console_listener.WaitForConsoleMessages(1);
   ASSERT_NE(base::string16::npos,
             console_listener.messages()[0].find(expected));
-  version_->embedded_worker()->RemoveListener(&console_listener);
+  RunOnIOThread(base::Bind(&EmbeddedWorkerInstance::RemoveListener,
+                           base::Unretained(version_->embedded_worker()),
+                           &console_listener));
 }
 
 class WaitForLoaded : public EmbeddedWorkerInstance::Listener {
  public:
   explicit WaitForLoaded(const base::Closure& quit) : quit_(quit) {}
 
   void OnThreadStarted() override {

[nhiroki, 2016/08/02 09:15:43]

DCHECK_CURRENTLY_ON(BrowserThread::IO)

[shimazu, 2016/08/02 23:34:49]

Done.

     BrowserThread::PostTask(BrowserThread::UI, FROM_HERE, quit_);
   }
   bool OnMessageReceived(const IPC::Message& message) override { return false; }
 
  private:
   base::Closure quit_;
 };
 
-// Has errors under TSan. See https://crbug.com/631323.
-#if defined(THREAD_SANITIZER)
-#define MAYBE_TimeoutStartingWorker DISABLED_TimeoutStartingWorker
-#else
-#define MAYBE_TimeoutStartingWorker TimeoutStartingWorker
-#endif
-IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest,
-                       MAYBE_TimeoutStartingWorker) {
+IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest, TimeoutStartingWorker) {
   RunOnIOThread(base::Bind(&self::SetUpRegistrationOnIOThread, this,
                            "/service_worker/while_true_worker.js"));
 
   // Start a worker, waiting until the script is loaded.
   ServiceWorkerStatusCode status = SERVICE_WORKER_ERROR_FAILED;
   base::RunLoop start_run_loop;
   base::RunLoop load_run_loop;
   WaitForLoaded wait_for_load(load_run_loop.QuitClosure());
-  version_->embedded_worker()->AddListener(&wait_for_load);
+  RunOnIOThread(base::Bind(&EmbeddedWorkerInstance::AddListener,
+                           base::Unretained(version_->embedded_worker()),
+                           &wait_for_load));
   BrowserThread::PostTask(BrowserThread::IO, FROM_HERE,
                           base::Bind(&self::StartOnIOThread, this,
                                      start_run_loop.QuitClosure(), &status));
   load_run_loop.Run();
-  version_->embedded_worker()->RemoveListener(&wait_for_load);
+  RunOnIOThread(base::Bind(&EmbeddedWorkerInstance::RemoveListener,
+                           base::Unretained(version_->embedded_worker()),
+                           &wait_for_load));
 
   // The script has loaded but start has not completed yet.
   ASSERT_EQ(SERVICE_WORKER_ERROR_FAILED, status);
   EXPECT_EQ(EmbeddedWorkerStatus::STARTING, version_->running_status());
 
   // Simulate execution timeout. Use a delay to prevent killing the worker
   // before it's started execution.
   EXPECT_TRUE(version_->timeout_timer_.IsRunning());
   RunOnIOThreadWithDelay(base::Bind(&self::TimeoutWorkerOnIOThread, this),
                          base::TimeDelta::FromMilliseconds(100));
@@ skipping 76 matching lines @@
 
   FetchOnRegisteredWorker(&result, &response2, &blob_data_handle);
   ASSERT_EQ(SERVICE_WORKER_FETCH_EVENT_RESULT_RESPONSE, result);
   EXPECT_EQ(200, response2.status_code);
   EXPECT_EQ("OK", response2.status_text);
   EXPECT_EQ(response1.response_time, response2.response_time);
   EXPECT_TRUE(response2.is_in_cache_storage);
   EXPECT_EQ("cache_name", response2.cache_storage_cache_name);
 }
 
-// Has errors under TSan. See https://crbug.com/631323.
-#if defined(THREAD_SANITIZER)
-#define MAYBE_FetchEvent_respondWithRejection \
-  DISABLED_FetchEvent_respondWithRejection
-#else
-#define MAYBE_FetchEvent_respondWithRejection FetchEvent_respondWithRejection
-#endif
 IN_PROC_BROWSER_TEST_F(ServiceWorkerVersionBrowserTest,
-                       MAYBE_FetchEvent_respondWithRejection) {
+                       FetchEvent_respondWithRejection) {
   ServiceWorkerFetchEventResult result;
   ServiceWorkerResponse response;
   std::unique_ptr<storage::BlobDataHandle> blob_data_handle;
 
   RunOnIOThread(base::Bind(&self::SetUpRegistrationOnIOThread, this,
                            "/service_worker/fetch_event_rejected.js"));
 
   ConsoleListener console_listener;
-  version_->embedded_worker()->AddListener(&console_listener);
+  RunOnIOThread(base::Bind(&EmbeddedWorkerInstance::AddListener,
+                           base::Unretained(version_->embedded_worker()),
+                           &console_listener));
 
   FetchOnRegisteredWorker(&result, &response, &blob_data_handle);
   const base::string16 expected1 = base::ASCIIToUTF16(
       "resulted in a network error response: the promise was rejected.");
   const base::string16 expected2 =
       base::ASCIIToUTF16("Uncaught (in promise) Rejecting respondWith promise");
   console_listener.WaitForConsoleMessages(2);
   ASSERT_NE(base::string16::npos,
             console_listener.messages()[0].find(expected1));
   ASSERT_EQ(0u, console_listener.messages()[1].find(expected2));
-  version_->embedded_worker()->RemoveListener(&console_listener);
+  RunOnIOThread(base::Bind(&EmbeddedWorkerInstance::RemoveListener,
+                           base::Unretained(version_->embedded_worker()),
+                           &console_listener));
 
   ASSERT_EQ(SERVICE_WORKER_FETCH_EVENT_RESULT_RESPONSE, result);
   EXPECT_EQ(0, response.status_code);
 
   ASSERT_FALSE(blob_data_handle);
 }
 
 class MockContentBrowserClient : public TestContentBrowserClient {
  public:
   MockContentBrowserClient()
@@ skipping 852 matching lines @@
 
 IN_PROC_BROWSER_TEST_F(ServiceWorkerDisableWebSecurityTest, UpdateNoCrash) {
   const char kPageUrl[] = "/service_worker/disable_web_security_update.html";
   const char kScopeUrl[] = "/service_worker/scope/";
   const char kWorkerUrl[] = "/service_worker/fetch_event_blob.js";
   RegisterServiceWorkerOnCrossOriginServer(kScopeUrl, kWorkerUrl);
   RunTestWithCrossOriginURL(kPageUrl, kScopeUrl);
 }
 
 }  // namespace content