Fix URLLoaderFactory use-after-free in NetworkPortalDetectorImpl

NetworkPortalDetectorImpl uses a raw pointer to the
SystemNetworkContextManager's URLLoaderFactory, which will be invalid
if mojo pipe runs into any issues. This CL makes it use a
SahredURLLoaderFactory instead, which will handle failures
automatically.

Bug: 936625
Change-Id: I2ea71ef6cc46440f66349729e16cc2558ef145b1
Reviewed-on: https://chromium-review.googlesource.com/c/1495748Reviewed-by: John Abd-El-Malek <jam@chromium.org>
Reviewed-by: Alexander Alekseev <alemate@chromium.org>
Commit-Queue: Robbie McElrath <rmcelrath@chromium.org>
Cr-Commit-Position: refs/heads/master@{#636645}

chrome/browser/chromeos/chrome_browser_main_chromeos.cc

@@ -224,9 +224,7 @@ void InitializeNetworkPortalDetector() {
         new NetworkPortalDetectorStub());
   } else {
     network_portal_detector::SetNetworkPortalDetector(
-        new NetworkPortalDetectorImpl(
-            g_browser_process->system_network_context_manager()
-                ->GetURLLoaderFactory()));
+        new NetworkPortalDetectorImpl());
   }
 }
 

chrome/browser/chromeos/net/network_portal_detector_impl.cc

@@ -13,7 +13,9 @@
 #include "base/metrics/histogram_macros.h"
 #include "base/single_thread_task_runner.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/chrome_notification_types.h"
+#include "chrome/browser/net/system_network_context_manager.h"
 #include "chromeos/constants/chromeos_switches.h"
 #include "chromeos/dbus/dbus_thread_manager.h"
 #include "chromeos/dbus/shill_profile_client.h"
@@ -24,6 +26,7 @@
 #include "content/public/browser/notification_service.h"
 #include "net/http/http_status_code.h"
 #include "net/traffic_annotation/network_traffic_annotation.h"
+#include "services/network/public/cpp/shared_url_loader_factory.h"
 #include "third_party/cros_system_api/dbus/service_constants.h"
 
 using captive_portal::CaptivePortalDetector;
@@ -203,12 +206,21 @@ constexpr base::TimeDelta
     NetworkPortalDetectorImpl::kDelaySinceShillPortalForUMA;
 
 NetworkPortalDetectorImpl::NetworkPortalDetectorImpl(
-    network::mojom::URLLoaderFactory* loader_factory)
+    network::mojom::URLLoaderFactory* loader_factory_for_testing)
     : strategy_(PortalDetectorStrategy::CreateById(
           PortalDetectorStrategy::STRATEGY_ID_LOGIN_SCREEN,
           this)),
       weak_factory_(this) {
   NET_LOG(EVENT) << "NetworkPortalDetectorImpl::NetworkPortalDetectorImpl()";
+  network::mojom::URLLoaderFactory* loader_factory;
+  if (loader_factory_for_testing) {
+    loader_factory = loader_factory_for_testing;
+  } else {
+    shared_url_loader_factory_ =
+        g_browser_process->system_network_context_manager()
+            ->GetSharedURLLoaderFactory();
+    loader_factory = shared_url_loader_factory_.get();
+  }
   captive_portal_detector_.reset(new CaptivePortalDetector(loader_factory));
 
   portal_test_url_ = GURL(CaptivePortalDetector::kDefaultURL);

chrome/browser/chromeos/net/network_portal_detector_impl.h

@@ -13,6 +13,7 @@
 #include "base/compiler_specific.h"
 #include "base/macros.h"
 #include "base/memory/ref_counted.h"
+#include "base/memory/scoped_refptr.h"
 #include "base/memory/weak_ptr.h"
 #include "base/observer_list.h"
 #include "base/sequence_checker.h"
@@ -34,10 +35,11 @@ class Value;
 }
 
 namespace network {
+class SharedURLLoaderFactory;
 namespace mojom {
 class URLLoaderFactory;
 }
-}
+}  // namespace network
 
 namespace chromeos {
 
@@ -57,7 +59,7 @@ class NetworkPortalDetectorImpl : public NetworkPortalDetector,
       base::TimeDelta::FromSeconds(60);
 
   explicit NetworkPortalDetectorImpl(
-      network::mojom::URLLoaderFactory* loader_factory);
+      network::mojom::URLLoaderFactory* loader_factory_for_testing = nullptr);
   ~NetworkPortalDetectorImpl() override;
 
   // Set the URL to be tested for portal state.
@@ -225,6 +227,9 @@ class NetworkPortalDetectorImpl : public NetworkPortalDetector,
   base::CancelableClosure attempt_task_;
   base::CancelableClosure attempt_timeout_;
 
+  // Reference to a SharedURLLoaderFactory used to detect portals.
+  scoped_refptr<network::SharedURLLoaderFactory> shared_url_loader_factory_;
+
   // URL that returns a 204 response code when connected to the Internet. Used
   // by tests.
   GURL portal_test_url_;

chrome/browser/ui/webui/chromeos/login/gaia_screen_handler.cc

@@ -315,9 +315,7 @@ void GaiaScreenHandler::MaybePreloadAuthExtension() {
   VLOG(1) << "MaybePreloadAuthExtension";
 
   if (!network_portal_detector_) {
-    NetworkPortalDetectorImpl* detector = new NetworkPortalDetectorImpl(
-        g_browser_process->system_network_context_manager()
-            ->GetURLLoaderFactory());
+    NetworkPortalDetectorImpl* detector = new NetworkPortalDetectorImpl();
     detector->set_portal_test_url(GURL(kRestrictiveProxyURL));
     network_portal_detector_.reset(detector);
     network_portal_detector_->AddObserver(this);