[Remoting Mobile] Fix crashes related to mojo URL loader refactoring

We have found a few crashes on Chromoting mobile clients that are
related to a refactoring to replace URLFetcher with ChromiumUrlRequest:

* Apps crash at launch because mojo core is not initialized.
* TransitionalURLLoaderFactoryOwner and the SharedURLLoaderFactory it
  creates seem to be thread unsafe, so the app crashes because of some
  thread checker failures.

These issues are fixed by:

* Calling mojo::core::Init() in the client runtime.
* Make url_loader_factory() only being called on the network thread and
  guard it with DCHECK.

Bug: 878919
Change-Id: I3827b5175546aa9845b53e95cbd913043028b59e
Reviewed-on: https://chromium-review.googlesource.com/1196026
Reviewed-by: Ken Rockot <rockot@chromium.org>
Reviewed-by: Antonio Gomes <tonikitoo@igalia.com>
Reviewed-by: Joe Downing <joedow@chromium.org>
Commit-Queue: Yuwei Huang <yuweih@chromium.org>
Cr-Commit-Position: refs/heads/master@{#587754}

remoting/base/telemetry_log_writer.cc

@@ -18,20 +18,25 @@ constexpr char kAuthorizationHeaderPrefix[] = "Authorization:Bearer ";
 
 TelemetryLogWriter::TelemetryLogWriter(
     const std::string& telemetry_base_url,
-    std::unique_ptr<UrlRequestFactory> request_factory,
     std::unique_ptr<OAuthTokenGetter> token_getter)
     : telemetry_base_url_(telemetry_base_url),
-      request_factory_(std::move(request_factory)),
       token_getter_(std::move(token_getter)) {
   DETACH_FROM_THREAD(thread_checker_);
-  DCHECK(request_factory_);
   DCHECK(token_getter_);
 }
 
 TelemetryLogWriter::~TelemetryLogWriter() {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
 }
 
+void TelemetryLogWriter::Init(
+    std::unique_ptr<UrlRequestFactory> request_factory) {
+  DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
+  DCHECK(!request_factory_);
+  DCHECK(request_factory);
+  request_factory_ = std::move(request_factory);
+}
+
 void TelemetryLogWriter::Log(const ChromotingEvent& entry) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   pending_entries_.push_back(entry);
@@ -72,6 +77,7 @@ void TelemetryLogWriter::PostJsonToServer(const std::string& json,
                                           const std::string& access_token) {
   DCHECK_CALLED_ON_VALID_THREAD(thread_checker_);
   DCHECK(!request_);
+  DCHECK(request_factory_);
   net::NetworkTrafficAnnotationTag traffic_annotation =
       net::DefineNetworkTrafficAnnotation("CRD_telemetry_log", R"(
         semantics {

remoting/base/telemetry_log_writer.h

@@ -29,14 +29,15 @@ namespace remoting {
 class TelemetryLogWriter : public ChromotingEventLogWriter {
  public:
   TelemetryLogWriter(const std::string& telemetry_base_url,
-                     std::unique_ptr<UrlRequestFactory> request_factory,
                      std::unique_ptr<OAuthTokenGetter> token_getter);
 
+  ~TelemetryLogWriter() override;
+
+  void Init(std::unique_ptr<UrlRequestFactory> request_factory);
+
   // Push the log entry to the pending list and send out all the pending logs.
   void Log(const ChromotingEvent& entry) override;
 
-  ~TelemetryLogWriter() override;
-
  private:
   void SendPendingEntries();
   void PostJsonToServer(const std::string& json,

remoting/base/telemetry_log_writer_unittest.cc

@@ -103,13 +103,14 @@ class FakeUrlRequestFactory : public UrlRequestFactory {
 class TelemetryLogWriterTest : public testing::Test {
  public:
   TelemetryLogWriterTest()
-      : request_factory_(new FakeUrlRequestFactory()),
-        log_writer_(
+      : log_writer_(
             "",
-            base::WrapUnique(request_factory_),
             std::make_unique<FakeOAuthTokenGetter>(OAuthTokenGetter::SUCCESS,
                                                    "email",
                                                    kFakeAccessToken)) {
+    auto request_factory = std::make_unique<FakeUrlRequestFactory>();
+    request_factory_ = request_factory.get();
+    log_writer_.Init(std::move(request_factory));
     success_result_.success = true;
     success_result_.status = 200;
     success_result_.response_body = "{}";
@@ -133,7 +134,7 @@ class TelemetryLogWriterTest : public testing::Test {
   UrlRequest::Result success_result_;
   UrlRequest::Result unauth_result_;
 
-  FakeUrlRequestFactory* request_factory_;  // For peeking. No ownership.
+  FakeUrlRequestFactory* request_factory_;  // No ownership.
   TelemetryLogWriter log_writer_;
 
  private:

remoting/client/BUILD.gn

@@ -61,6 +61,7 @@ static_library("client") {
       "oauth_token_getter_proxy.h",
     ]
     deps += [
+      "//mojo/core/embedder",
       "//remoting/client/input",
       "//remoting/client/ui",
       "//services/network/public/mojom",

remoting/client/DEPS

@@ -1,6 +1,7 @@
 include_rules = [
   "+ppapi",
   "+jingle/glue",
+  "+mojo/core/embedder",
   "+net",
   "+ui/events/keycodes/dom",
 

remoting/client/chromoting_client_runtime.cc

@@ -12,6 +12,7 @@
 #include "base/message_loop/message_loop_current.h"
 #include "base/task/task_scheduler/task_scheduler.h"
 #include "build/build_config.h"
+#include "mojo/core/embedder/embedder.h"
 #include "remoting/base/chromium_url_request.h"
 #include "remoting/base/telemetry_log_writer.h"
 #include "remoting/base/url_request_context_getter.h"
@@ -58,10 +59,8 @@ ChromotingClientRuntime::ChromotingClientRuntime() {
   display_task_runner_ = AutoThread::Create("native_disp", ui_task_runner_);
   network_task_runner_ = AutoThread::CreateWithType(
       "native_net", ui_task_runner_, base::MessageLoop::TYPE_IO);
-  url_requester_ = new URLRequestContextGetter(network_task_runner_);
-  url_loader_factory_owner_ =
-      std::make_unique<network::TransitionalURLLoaderFactoryOwner>(
-          url_requester_);
+
+  mojo::core::Init();
 }
 
 ChromotingClientRuntime::~ChromotingClientRuntime() {
@@ -86,8 +85,11 @@ void ChromotingClientRuntime::Init(
   delegate_ = delegate;
   log_writer_ = std::make_unique<TelemetryLogWriter>(
       kTelemetryBaseUrl,
-      std::make_unique<ChromiumUrlRequestFactory>(url_loader_factory()),
       CreateOAuthTokenGetter());
+  network_task_runner()->PostTask(
+      FROM_HERE,
+      base::BindOnce(&ChromotingClientRuntime::InitializeOnNetworkThread,
+                     base::Unretained(this)));
 }
 
 std::unique_ptr<OAuthTokenGetter>
@@ -98,7 +100,18 @@ ChromotingClientRuntime::CreateOAuthTokenGetter() {
 
 scoped_refptr<network::SharedURLLoaderFactory>
 ChromotingClientRuntime::url_loader_factory() {
+  DCHECK(network_task_runner()->BelongsToCurrentThread());
   return url_loader_factory_owner_->GetURLLoaderFactory();
 }
 
+void ChromotingClientRuntime::InitializeOnNetworkThread() {
+  DCHECK(network_task_runner()->BelongsToCurrentThread());
+  url_requester_ = new URLRequestContextGetter(network_task_runner_);
+  url_loader_factory_owner_ =
+      std::make_unique<network::TransitionalURLLoaderFactoryOwner>(
+          url_requester_);
+  log_writer_->Init(
+      std::make_unique<ChromiumUrlRequestFactory>(url_loader_factory()));
+}
+
 }  // namespace remoting

remoting/client/chromoting_client_runtime.h

@@ -77,6 +77,7 @@ class ChromotingClientRuntime {
     return url_requester_;
   }
 
+  // Must be called from the network thread.
   scoped_refptr<network::SharedURLLoaderFactory> url_loader_factory();
 
   ChromotingEventLogWriter* log_writer() { return log_writer_.get(); }
@@ -85,6 +86,10 @@ class ChromotingClientRuntime {
   ChromotingClientRuntime();
   virtual ~ChromotingClientRuntime();
 
+  // Initializes URL loader factory owner, log writer, and other resources on
+  // the network thread.
+  void InitializeOnNetworkThread();
+
   // Chromium code's connection to the app message loop. Once created the
   // MessageLoop will live for the life of the program.
   std::unique_ptr<base::MessageLoopForUI> ui_loop_;