Fix Crashes when Searching or deleting cookies in chrome://settings/siteData

The underlying root cause for the bugs is that the javascript callbacks are not called for the handler cases where the cookies tree model is recreated.

Also made some misc minor fixes and substitution of deprecated calls.

Fixed: 795611,877082,1011734,845245
Change-Id: If94f701008dac9e0ddde3a85e7953b9b45f75934
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1851229
Commit-Queue: S. Ganesh <ganesh@chromium.org>
Reviewed-by: Dan Beam <dbeam@chromium.org>
Reviewed-by: Demetrios Papadopoulos <dpapad@chromium.org>
Cr-Commit-Position: refs/heads/master@{#704972}

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

@@ -132,6 +132,7 @@ void CookiesViewHandler::OnJavascriptAllowed() {
 
 void CookiesViewHandler::OnJavascriptDisallowed() {
   callback_weak_ptr_factory_.InvalidateWeakPtrs();
+  request_.Clear();
 }
 
 void CookiesViewHandler::RegisterMessages() {
@@ -231,7 +232,7 @@ void CookiesViewHandler::TreeModelBeginBatch(CookiesTreeModel* model) {
 void CookiesViewHandler::TreeModelEndBatch(CookiesTreeModel* model) {
   DCHECK(batch_update_);
   batch_update_ = false;
-  if (IsJavascriptAllowed()) {
+
   if (request_.should_send_list) {
     SendLocalDataList(model->GetRoot());
   } else if (!request_.callback_id_.empty()) {
@@ -239,7 +240,6 @@ void CookiesViewHandler::TreeModelEndBatch(CookiesTreeModel* model) {
                               (base::Value()));
     request_.Clear();
   }
-  }
 }
 
 void CookiesViewHandler::EnsureCookiesTreeModelCreated() {
@@ -255,14 +255,18 @@ void CookiesViewHandler::RecreateCookiesTreeModel() {
   filter_.clear();
   sorted_sites_.clear();
   EnsureCookiesTreeModelCreated();
+
+  CHECK(!request_.callback_id_.empty());
+  ResolveJavascriptCallback(base::Value(request_.callback_id_),
+                            (base::Value()));
+  request_.Clear();
 }
 
 void CookiesViewHandler::HandleGetCookieDetails(const base::ListValue* args) {
   CHECK(request_.callback_id_.empty());
-  CHECK_EQ(2U, args->GetSize());
-  CHECK(args->GetString(0, &request_.callback_id_));
-  std::string site;
-  CHECK(args->GetString(1, &site));
+  CHECK_EQ(2U, args->GetList().size());
+  request_.callback_id_ = args->GetList()[0].GetString();
+  std::string site = args->GetList()[1].GetString();
 
   AllowJavascript();
   const CookieTreeNode* node = model_util_->GetTreeNodeFromTitle(
@@ -279,11 +283,10 @@ void CookiesViewHandler::HandleGetCookieDetails(const base::ListValue* args) {
 
 void CookiesViewHandler::HandleGetNumCookiesString(
     const base::ListValue* args) {
-  CHECK_EQ(2U, args->GetSize());
+  CHECK_EQ(2U, args->GetList().size());
   std::string callback_id;
-  CHECK(args->GetString(0, &callback_id));
-  int num_cookies;
-  CHECK(args->GetInteger(1, &num_cookies));
+  callback_id = args->GetList()[0].GetString();
+  int num_cookies = args->GetList()[1].GetInt();
 
   AllowJavascript();
   const base::string16 string =
@@ -296,10 +299,9 @@ void CookiesViewHandler::HandleGetNumCookiesString(
 
 void CookiesViewHandler::HandleGetDisplayList(const base::ListValue* args) {
   CHECK(request_.callback_id_.empty());
-  CHECK_EQ(2U, args->GetSize());
-  CHECK(args->GetString(0, &request_.callback_id_));
-  base::string16 filter;
-  CHECK(args->GetString(1, &filter));
+  CHECK_EQ(2U, args->GetList().size());
+  request_.callback_id_ = args->GetList()[0].GetString();
+  base::string16 filter = base::UTF8ToUTF16(args->GetList()[1].GetString());
 
   AllowJavascript();
   request_.should_send_list = true;
@@ -315,8 +317,8 @@ void CookiesViewHandler::HandleGetDisplayList(const base::ListValue* args) {
 
 void CookiesViewHandler::HandleReloadCookies(const base::ListValue* args) {
   CHECK(request_.callback_id_.empty());
-  CHECK_EQ(1U, args->GetSize());
-  CHECK(args->GetString(0, &request_.callback_id_));
+  CHECK_EQ(1U, args->GetList().size());
+  request_.callback_id_ = args->GetList()[0].GetString();
 
   AllowJavascript();
   RecreateCookiesTreeModel();
@@ -324,8 +326,8 @@ void CookiesViewHandler::HandleReloadCookies(const base::ListValue* args) {
 
 void CookiesViewHandler::HandleRemoveAll(const base::ListValue* args) {
   CHECK(request_.callback_id_.empty());
-  CHECK_EQ(1U, args->GetSize());
-  CHECK(args->GetString(0, &request_.callback_id_));
+  CHECK_EQ(1U, args->GetList().size());
+  request_.callback_id_ = args->GetList()[0].GetString();
 
   AllowJavascript();
   cookies_tree_model_->DeleteAllStoredObjects();
@@ -333,8 +335,7 @@ void CookiesViewHandler::HandleRemoveAll(const base::ListValue* args) {
 }
 
 void CookiesViewHandler::HandleRemove(const base::ListValue* args) {
-  std::string node_path;
-  CHECK(args->GetString(0, &node_path));
+  std::string node_path = args->GetList()[0].GetString();
 
   AllowJavascript();
   const CookieTreeNode* node = model_util_->GetTreeNodeFromPath(
@@ -347,8 +348,8 @@ void CookiesViewHandler::HandleRemove(const base::ListValue* args) {
 
 void CookiesViewHandler::HandleRemoveThirdParty(const base::ListValue* args) {
   CHECK(request_.callback_id_.empty());
-  CHECK_EQ(1U, args->GetSize());
-  CHECK(args->GetString(0, &request_.callback_id_));
+  CHECK_EQ(1U, args->GetList().size());
+  request_.callback_id_ = args->GetList()[0].GetString();
 
   AllowJavascript();
   Profile* profile = Profile::FromWebUI(web_ui());
@@ -360,7 +361,7 @@ void CookiesViewHandler::HandleRemoveThirdParty(const base::ListValue* args) {
 }
 
 void CookiesViewHandler::HandleRemoveShownItems(const base::ListValue* args) {
-  CHECK_EQ(0U, args->GetSize());
+  CHECK_EQ(0U, args->GetList().size());
 
   AllowJavascript();
   CookieTreeNode* parent = cookies_tree_model_->GetRoot();
@@ -369,10 +370,9 @@ void CookiesViewHandler::HandleRemoveShownItems(const base::ListValue* args) {
 }
 
 void CookiesViewHandler::HandleRemoveItem(const base::ListValue* args) {
-  CHECK_EQ(1U, args->GetSize());
+  CHECK_EQ(1U, args->GetList().size());
   CHECK(request_.callback_id_.empty());
-  base::string16 site;
-  CHECK(args->GetString(0, &site));
+  base::string16 site = base::UTF8ToUTF16(args->GetList()[0].GetString());
 
   AllowJavascript();
   CookieTreeNode* parent = cookies_tree_model_->GetRoot();

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

@@ -46,6 +46,10 @@ class CookiesViewHandler : public SettingsPageUIHandler,
   void TreeModelEndBatch(CookiesTreeModel* model) override;
 
  private:
+  friend class CookiesViewHandlerTest;
+  FRIEND_TEST_ALL_PREFIXES(CookiesViewHandlerTest,
+                           HandleReloadCookiesAndGetDisplayList);
+
   // Creates the CookiesTreeModel if necessary.
   void EnsureCookiesTreeModelCreated();
 

chrome/browser/ui/webui/settings/settings_cookies_view_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/settings/settings_cookies_view_handler.h"
+
+#include <memory>
+#include <string>
+
+#include "base/values.h"
+#include "chrome/test/base/chrome_render_view_host_test_harness.h"
+#include "content/public/test/test_web_ui.h"
+
+namespace settings {
+
+class CookiesViewHandlerTest : public ChromeRenderViewHostTestHarness {
+ public:
+  void SetUp() override {
+    ChromeRenderViewHostTestHarness::SetUp();
+
+    web_ui_ = std::make_unique<content::TestWebUI>();
+    web_ui_->set_web_contents(web_contents());
+    handler_ = std::make_unique<CookiesViewHandler>();
+    handler_->set_web_ui(web_ui());
+    handler_->AllowJavascript();
+    web_ui_->ClearTrackedCalls();
+  }
+
+  void TearDown() override {
+    handler_->set_web_ui(nullptr);
+    handler_.reset();
+    web_ui_.reset();
+
+    ChromeRenderViewHostTestHarness::TearDown();
+  }
+
+  content::TestWebUI* web_ui() { return web_ui_.get(); }
+  CookiesViewHandler* handler() { return handler_.get(); }
+
+ private:
+  std::unique_ptr<content::TestWebUI> web_ui_;
+  std::unique_ptr<CookiesViewHandler> handler_;
+};
+
+// This unit test checks that the javascript callbacks are called correctly for
+// the reloadCookies and the getDisplayList handler cases. It also makes sure
+// that CHECKs for request_.callback_id_.empty() do not fire when multiple
+// handlers are called in sequence.
+TEST_F(CookiesViewHandlerTest, HandleReloadCookiesAndGetDisplayList) {
+  const std::string reload_callback_id("localData.reload_0");
+  const std::string get_display_list_callback_id("localData.getDisplayList_1");
+
+  base::ListValue reload_args;
+  reload_args.AppendString(reload_callback_id);
+  handler()->HandleReloadCookies(&reload_args);
+  EXPECT_EQ(1U, web_ui()->call_data().size());
+
+  base::ListValue get_display_list_args;
+  get_display_list_args.AppendString(reload_callback_id);
+  get_display_list_args.AppendString(std::string());
+  handler()->HandleGetDisplayList(&get_display_list_args);
+  EXPECT_EQ(2U, web_ui()->call_data().size());
+}
+
+}  // namespace settings

chrome/test/BUILD.gn

@@ -3887,6 +3887,7 @@ test("unit_tests") {
       "../browser/ui/webui/settings/people_handler_unittest.cc",
       "../browser/ui/webui/settings/profile_info_handler_unittest.cc",
       "../browser/ui/webui/settings/reset_settings_handler_unittest.cc",
+      "../browser/ui/webui/settings/settings_cookies_view_handler_unittest.cc",
       "../browser/ui/webui/settings/settings_manage_profile_handler_unittest.cc",
       "../browser/ui/webui/settings/site_settings_handler_unittest.cc",
       "../browser/ui/webui/settings_utils_unittest.cc",