Use WeakPtr when getting ports in SerialChooserController

As of r626853 SerialChooserController no longer owners its own
connection to the SerialPortManager. Since this connection can now
outlive the controller instance it is not safe to bind an raw pointer
to |this|. This patch adds a WeakPtrFactory so that callbacks arriving
after the controller has been destroyed are safely discarded.

Bug: 952709
Change-Id: I1994b0aa3e7514bb200c304e70accce4b17e0b9b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1606588Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Commit-Queue: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#659725}

chrome/browser/ui/serial/serial_chooser_controller.cc

@@ -38,7 +38,7 @@ SerialChooserController::SerialChooserController(
   DCHECK(chooser_context_);
 
   chooser_context_->GetPortManager()->GetDevices(base::BindOnce(
-      &SerialChooserController::OnGetDevices, base::Unretained(this)));
+      &SerialChooserController::OnGetDevices, weak_factory_.GetWeakPtr()));
 }
 
 SerialChooserController::~SerialChooserController() {

chrome/browser/ui/serial/serial_chooser_controller.h

@@ -25,7 +25,7 @@ class SerialChooserContext;
 
 // SerialChooserController provides data for the Serial API permission prompt.
 // It is owned by ChooserBubbleDelegate.
-class SerialChooserController : public ChooserController {
+class SerialChooserController final : public ChooserController {
  public:
   SerialChooserController(
       content::RenderFrameHost* render_frame_host,
@@ -56,6 +56,8 @@ class SerialChooserController : public ChooserController {
   base::WeakPtr<SerialChooserContext> chooser_context_;
   std::vector<device::mojom::SerialPortInfoPtr> ports_;
 
+  base::WeakPtrFactory<SerialChooserController> weak_factory_{this};
+
   DISALLOW_COPY_AND_ASSIGN(SerialChooserController);
 };
 

chrome/browser/ui/serial/serial_chooser_controller_unittest.cc

+// Copyright 2019 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 "chrome/browser/ui/serial/serial_chooser_controller.h"
+
+#include "base/test/bind_test_util.h"
+#include "chrome/browser/serial/serial_chooser_context.h"
+#include "chrome/browser/serial/serial_chooser_context_factory.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "chrome/test/base/testing_profile.h"
+#include "mojo/public/cpp/bindings/interface_request.h"
+#include "services/device/public/cpp/test/fake_serial_port_manager.h"
+#include "services/device/public/mojom/serial.mojom.h"
+#include "testing/gmock/include/gmock/gmock.h"
+#include "testing/gtest/include/gtest/gtest.h"
+#include "third_party/blink/public/mojom/serial/serial.mojom.h"
+
+class SerialChooserControllerTest : public ChromeRenderViewHostTestHarness {
+ public:
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+
+    device::mojom::SerialPortManagerPtr port_manager_ptr;
+    port_manager_.AddBinding(mojo::MakeRequest(&port_manager_ptr));
+    SerialChooserContextFactory::GetForProfile(profile())
+        ->SetPortManagerForTesting(std::move(port_manager_ptr));
+  }
+
+ private:
+  device::FakeSerialPortManager port_manager_;
+};
+
+TEST_F(SerialChooserControllerTest, GetPortsLateResponse) {
+  std::vector<blink::mojom::SerialPortFilterPtr> filters;
+
+  bool callback_run = false;
+  auto callback = base::BindLambdaForTesting(
+      [&](device::mojom::SerialPortInfoPtr port_info) {
+        EXPECT_FALSE(port_info);
+        callback_run = true;
+      });
+
+  auto controller = std::make_unique<SerialChooserController>(
+      main_rfh(), std::move(filters), std::move(callback));
+  controller.reset();
+
+  // Allow any tasks posted by |controller| to run, such as asynchronous
+  // requests to the Device Service to get the list of available serial ports.
+  // These should be safely discarded since |controller| was destroyed.
+  base::RunLoop().RunUntilIdle();
+
+  // Even if |controller| is destroyed without user interaction the callback
+  // should be run.
+  EXPECT_TRUE(callback_run);
+}

chrome/test/BUILD.gn

@@ -3499,6 +3499,7 @@ test("unit_tests") {
       "../browser/ui/search/search_ipc_router_policy_unittest.cc",
       "../browser/ui/search/search_ipc_router_unittest.cc",
       "../browser/ui/search/search_tab_helper_unittest.cc",
+      "../browser/ui/serial/serial_chooser_controller_unittest.cc",
       "../browser/ui/tab_contents/tab_contents_iterator_unittest.cc",
       "../browser/ui/tabs/pinned_tab_codec_unittest.cc",
       "../browser/ui/tabs/pinned_tab_service_unittest.cc",
@@ -3595,6 +3596,7 @@ test("unit_tests") {
       "//components/send_tab_to_self:test_support",
       "//components/signin/core/browser:signin_buildflags",
       "//components/sync:test_support",
+      "//services/device/public/cpp/test:test_support",
       "//services/metrics/public/cpp:ukm_builders",
       "//third_party/libaddressinput",
       "//ui/native_theme:test_support",

services/device/public/cpp/test/fake_serial_port_manager.cc

@@ -88,6 +88,11 @@ FakeSerialPortManager::FakeSerialPortManager() = default;
 
 FakeSerialPortManager::~FakeSerialPortManager() = default;
 
+void FakeSerialPortManager::AddBinding(
+    mojom::SerialPortManagerRequest request) {
+  bindings_.AddBinding(this, std::move(request));
+}
+
 void FakeSerialPortManager::AddPort(mojom::SerialPortInfoPtr port) {
   base::UnguessableToken token = port->token;
   ports_[token] = std::move(port);

services/device/public/cpp/test/fake_serial_port_manager.h

@@ -9,6 +9,7 @@
 
 #include "base/macros.h"
 #include "base/unguessable_token.h"
+#include "mojo/public/cpp/bindings/binding_set.h"
 #include "services/device/public/mojom/serial.mojom.h"
 
 namespace device {
@@ -18,6 +19,7 @@ class FakeSerialPortManager : public mojom::SerialPortManager {
   FakeSerialPortManager();
   ~FakeSerialPortManager() override;
 
+  void AddBinding(mojom::SerialPortManagerRequest request);
   void AddPort(mojom::SerialPortInfoPtr port);
 
   // mojom::SerialPortManager
@@ -28,6 +30,7 @@ class FakeSerialPortManager : public mojom::SerialPortManager {
 
  private:
   std::map<base::UnguessableToken, mojom::SerialPortInfoPtr> ports_;
+  mojo::BindingSet<mojom::SerialPortManager> bindings_;
 
   DISALLOW_COPY_AND_ASSIGN(FakeSerialPortManager);
 };