Components: write test for "check for updates" dead observer crash

R=dpapad@chromium.org

Bug: 1038846
Change-Id: I72d5b46a6132d0e033a1236af57153cf6a7b603b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1987134Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Commit-Queue: Dan Beam <dbeam@chromium.org>
Auto-Submit: Dan Beam <dbeam@chromium.org>
Cr-Commit-Position: refs/heads/master@{#728659}

chrome/browser/ui/webui/components/components_handler.cc

@@ -6,12 +6,15 @@
 
 #include "base/bind.h"
 #include "base/values.h"
-#include "chrome/browser/browser_process.h"
 #include "chrome/grit/generated_resources.h"
 #include "components/update_client/crx_update_item.h"
 #include "ui/base/l10n/l10n_util.h"
 
-ComponentsHandler::ComponentsHandler() = default;
+ComponentsHandler::ComponentsHandler(
+    component_updater::ComponentUpdateService* component_updater)
+    : component_updater_(component_updater) {
+  DCHECK(component_updater_);
+}
 ComponentsHandler::~ComponentsHandler() = default;
 
 void ComponentsHandler::RegisterMessages() {
@@ -26,7 +29,7 @@ void ComponentsHandler::RegisterMessages() {
 }
 
 void ComponentsHandler::OnJavascriptAllowed() {
-  observer_.Add(g_browser_process->component_updater());
+  observer_.Add(component_updater_);
 }
 
 void ComponentsHandler::OnJavascriptDisallowed() {
@@ -67,9 +70,8 @@ void ComponentsHandler::OnEvent(Events event, const std::string& id) {
   parameters.SetString("event", ComponentEventToString(event));
   if (!id.empty()) {
     if (event == Events::COMPONENT_UPDATED) {
-      auto* component_updater = g_browser_process->component_updater();
       update_client::CrxUpdateItem item;
-      if (component_updater->GetComponentDetails(id, &item) && item.component)
+      if (component_updater_->GetComponentDetails(id, &item) && item.component)
         parameters.SetString("version", item.component->version.GetString());
     }
     parameters.SetString("id", id);
@@ -133,29 +135,21 @@ base::string16 ComponentsHandler::ServiceStatusToString(
   return l10n_util::GetStringUTF16(IDS_COMPONENTS_UNKNOWN);
 }
 
-// static
 void ComponentsHandler::OnDemandUpdate(const std::string& component_id) {
-  component_updater::ComponentUpdateService* cus =
-      g_browser_process->component_updater();
-  if (cus) {  // TODO(dbeam): can this return nullptr if called from UI thread?
-    cus->GetOnDemandUpdater().OnDemandUpdate(
-        component_id, component_updater::OnDemandUpdater::Priority::FOREGROUND,
-        component_updater::Callback());
-  }
+  component_updater_->GetOnDemandUpdater().OnDemandUpdate(
+      component_id, component_updater::OnDemandUpdater::Priority::FOREGROUND,
+      component_updater::Callback());
 }
 
-// static
 std::unique_ptr<base::ListValue> ComponentsHandler::LoadComponents() {
-  component_updater::ComponentUpdateService* cus =
-      g_browser_process->component_updater();
   std::vector<std::string> component_ids;
-  component_ids = cus->GetComponentIDs();
+  component_ids = component_updater_->GetComponentIDs();
 
   // Construct DictionaryValues to return to UI.
   auto component_list = std::make_unique<base::ListValue>();
   for (size_t j = 0; j < component_ids.size(); ++j) {
     update_client::CrxUpdateItem item;
-    if (cus->GetComponentDetails(component_ids[j], &item)) {
+    if (component_updater_->GetComponentDetails(component_ids[j], &item)) {
       auto component_entry = std::make_unique<base::DictionaryValue>();
       component_entry->SetString("id", component_ids[j]);
       component_entry->SetString("status", ServiceStatusToString(item.state));

chrome/browser/ui/webui/components/components_handler.h

@@ -22,7 +22,8 @@ class ListValue;
 class ComponentsHandler : public content::WebUIMessageHandler,
                           public component_updater::ServiceObserver {
  public:
-  ComponentsHandler();
+  ComponentsHandler(
+      component_updater::ComponentUpdateService* component_updater);
   ComponentsHandler(const ComponentsHandler&) = delete;
   ComponentsHandler& operator=(const ComponentsHandler&) = delete;
   ~ComponentsHandler() override;
@@ -45,8 +46,12 @@ class ComponentsHandler : public content::WebUIMessageHandler,
   static base::string16 ComponentEventToString(Events event);
   static base::string16 ServiceStatusToString(
       update_client::ComponentState state);
-  static std::unique_ptr<base::ListValue> LoadComponents();
-  static void OnDemandUpdate(const std::string& component_id);
+
+  std::unique_ptr<base::ListValue> LoadComponents();
+  void OnDemandUpdate(const std::string& component_id);
+
+  // Weak pointer; injected for testing.
+  component_updater::ComponentUpdateService* const component_updater_;
 
   ScopedObserver<component_updater::ComponentUpdateService,
                  component_updater::ComponentUpdateService::Observer>

chrome/browser/ui/webui/components/components_handler_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/webui/components/components_handler.h"
+
+#include "components/component_updater/mock_component_updater_service.h"
+#include "content/public/test/test_web_ui.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+class TestComponentsHandler : public ComponentsHandler {
+ public:
+  TestComponentsHandler(
+      component_updater::ComponentUpdateService* component_update_service)
+      : ComponentsHandler(component_update_service) {
+    set_web_ui(&test_web_ui_);
+  }
+
+ private:
+  content::TestWebUI test_web_ui_;
+};
+
+TEST(ComponentsHandlerTest, RemovesObserver) {
+  testing::NiceMock<component_updater::MockComponentUpdateService> mock_service;
+  EXPECT_CALL(mock_service, AddObserver(testing::_)).Times(1);
+  EXPECT_CALL(mock_service, RemoveObserver(testing::_)).Times(1);
+
+  {
+    TestComponentsHandler handler(&mock_service);
+    base::ListValue args;
+    args.AppendString("unused");
+    handler.HandleRequestComponentsData(&args);
+  }
+}

chrome/browser/ui/webui/components/components_ui.cc

@@ -77,7 +77,8 @@ content::WebUIDataSource* CreateComponentsUIHTMLSource(Profile* profile) {
 ///////////////////////////////////////////////////////////////////////////////
 
 ComponentsUI::ComponentsUI(content::WebUI* web_ui) : WebUIController(web_ui) {
-  web_ui->AddMessageHandler(std::make_unique<ComponentsHandler>());
+  web_ui->AddMessageHandler(std::make_unique<ComponentsHandler>(
+      g_browser_process->component_updater()));
 
   // Set up the chrome://components/ source.
   Profile* profile = Profile::FromWebUI(web_ui);

chrome/test/BUILD.gn

@@ -4054,6 +4054,7 @@ test("unit_tests") {
       "../browser/ui/toolbar/toolbar_actions_model_unittest.cc",
       "../browser/ui/web_applications/app_browser_controller_unittest.cc",
       "../browser/ui/web_applications/web_app_launch_utils_unittest.cc",
+      "../browser/ui/webui/components/components_handler_unittest.cc",
       "../browser/ui/webui/downloads/downloads_dom_handler_unittest.cc",
       "../browser/ui/webui/downloads/downloads_list_tracker_unittest.cc",
       "../browser/ui/webui/downloads/mock_downloads_page.cc",