MD Settings: Fix WebUI lifecycle issues in Clear Browsing Data handler.

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

Index: chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
diff --git a/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc b/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
index b5a014ce17b338385c5a9383c5050c41938b2194..a06e2955938bdcb7c5217b543197058dbcd897e4 100644
--- a/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
+++ b/chrome/browser/ui/webui/settings/settings_clear_browsing_data_handler.cc
@@ -109,12 +109,19 @@ void ClearBrowsingDataHandler::OnJavascriptAllowed() {
 
   if (sync_service_)
     sync_service_observer_.Add(sync_service_);
+
+  DCHECK(counters_.empty());

[Dan Beam, 2016/11/29 06:18:58]

nit: I suppose this is OK, but why can't we just .clear() here as well?

[tommycli, 2016/11/30 15:59:35]

Clearing would be also harmless, but clearing tells devs "this list might have
something in it", whereas this dcheck tells devs "this list should definitely be
empty".

+  for (const std::string& pref : kCounterPrefs) {
+    AddCounter(
+        BrowsingDataCounterFactory::GetForProfileAndPref(profile_, pref));
+  }
 }
 
 void ClearBrowsingDataHandler::OnJavascriptDisallowed() {
   profile_pref_registrar_.RemoveAll();
   sync_service_observer_.RemoveAll();
   task_observer_.reset();
+  counters_.clear();

[tommycli, 2016/11/29 01:08:12]

This was the missing line that caused the crashes.

 }
 
 void ClearBrowsingDataHandler::HandleClearBrowsingData(
@@ -259,14 +266,6 @@ void ClearBrowsingDataHandler::HandleInitialize(const base::ListValue* args) {
   const base::Value* callback_id;
   CHECK(args->Get(0, &callback_id));
 
-  task_observer_.reset();

[tommycli, 2016/11/29 01:08:12]

This was not necessary right? Reading the code -- it appears that this can only
be set by the HandleClearBrowsingData handler, which should come after the
HandleInitialize handler.

After that, any refreshes or navigations away will trigger
OnJavascriptDisallowed right?

[Dan Beam, 2016/11/29 06:18:58]

in theory, though a renderer crash might affect this.

related:
https://cs.chromium.org/chromium/src/chrome/browser/ui/webui/md_downloads/md_...

[tommycli, 2016/11/30 15:59:35]

I see. I restored it and added a comment.

-  counters_.clear();
-
-  for (const std::string& pref : kCounterPrefs) {
-    AddCounter(
-        BrowsingDataCounterFactory::GetForProfileAndPref(profile_, pref));
-  }
-
   OnStateChanged();
   RefreshHistoryNotice();