Safety check: fixed browser crash due to a WebUI update check callback after...

Safety check: fixed browser crash due to a WebUI update check callback after OnJavascriptDisallowed()

Bug: 1062566, 1015841
Change-Id: I8990125168e05ec9a04ee8b282a46214c48f7de8
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2107548Reviewed-by: Esmael Elmoslimany <aee@chromium.org>
Commit-Queue: Andrey Zaytsev <andzaytsev@google.com>
Auto-Submit: Andrey Zaytsev <andzaytsev@google.com>
Cr-Commit-Position: refs/heads/master@{#751480}

chrome/browser/ui/webui/settings/safety_check_handler.cc

@@ -527,6 +527,9 @@ void SafetyCheckHandler::OnJavascriptDisallowed() {
   // another safety check is started. Otherwise |observed_leak_check_|
   // automatically calls RemoveAll() on destruction.
   observed_leak_check_.RemoveAll();
+  // Destroy the version updater to prevent getting a callback and firing a
+  // WebUI event, which would cause a crash.
+  version_updater_.reset();
 }
 
 void SafetyCheckHandler::RegisterMessages() {

chrome/browser/ui/webui/settings/safety_check_handler.h

@@ -91,6 +91,11 @@ class SafetyCheckHandler
                      extensions::ExtensionPrefs* extension_prefs,
                      extensions::ExtensionServiceInterface* extension_service);
 
+  void SetVersionUpdaterForTesting(
+      std::unique_ptr<VersionUpdater> version_updater) {
+    version_updater_ = std::move(version_updater);
+  }
+
  private:
   // These ensure integers are passed in the correct possitions in the extension
   // check methods.

chrome/browser/ui/webui/settings/safety_check_handler_unittest.cc

@@ -52,6 +52,7 @@ class TestingSafetyCheckHandler : public SafetyCheckHandler {
   using SafetyCheckHandler::AllowJavascript;
   using SafetyCheckHandler::DisallowJavascript;
   using SafetyCheckHandler::set_web_ui;
+  using SafetyCheckHandler::SetVersionUpdaterForTesting;
 
   TestingSafetyCheckHandler(
       std::unique_ptr<VersionUpdater> version_updater,
@@ -66,6 +67,21 @@ class TestingSafetyCheckHandler : public SafetyCheckHandler {
                            extension_service) {}
 };
 
+class TestDestructionVersionUpdater : public TestVersionUpdater {
+ public:
+  ~TestDestructionVersionUpdater() override { destructor_invoked_ = true; }
+
+  void CheckForUpdate(const StatusCallback& callback,
+                      const PromoteCallback&) override {}
+
+  static bool GetDestructorInvoked() { return destructor_invoked_; }
+
+ private:
+  static bool destructor_invoked_;
+};
+
+bool TestDestructionVersionUpdater::destructor_invoked_ = false;
+
 class TestPasswordsDelegate : public extensions::TestPasswordsPrivateDelegate {
  public:
   void SetBulkLeakCheckService(
@@ -408,6 +424,15 @@ TEST_F(SafetyCheckHandlerTest, CheckUpdates_Failed) {
       "Browser update problems and failed updates.</a>");
 }
 
+TEST_F(SafetyCheckHandlerTest, CheckUpdates_DestroyedOnJavascriptDisallowed) {
+  EXPECT_FALSE(TestDestructionVersionUpdater::GetDestructorInvoked());
+  safety_check_->SetVersionUpdaterForTesting(
+      std::make_unique<TestDestructionVersionUpdater>());
+  safety_check_->PerformSafetyCheck();
+  safety_check_->DisallowJavascript();
+  EXPECT_TRUE(TestDestructionVersionUpdater::GetDestructorInvoked());
+}
+
 TEST_F(SafetyCheckHandlerTest, CheckSafeBrowsing_Enabled) {
   Profile::FromWebUI(&test_web_ui_)
       ->GetPrefs()
@@ -520,6 +545,10 @@ TEST_F(SafetyCheckHandlerTest, CheckPasswords_InterruptedAndRefreshed) {
   // The check gets interrupted and the page is refreshed.
   safety_check_->DisallowJavascript();
   safety_check_->AllowJavascript();
+  // Need to set the |TestVersionUpdater| instance again to prevent
+  // |PerformSafetyCheck()| from creating a real |VersionUpdater| instance.
+  safety_check_->SetVersionUpdaterForTesting(
+      std::make_unique<TestVersionUpdater>());
   // Another run of the safety check.
   safety_check_->PerformSafetyCheck();
   test_leak_service_->set_state_and_notify(