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

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

 // Copyright 2015 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_clear_browsing_data_handler.h"
 
 #include <stddef.h>
 
 #include "base/macros.h"
 #include "base/memory/ptr_util.h"
@@ skipping 91 matching lines @@
 void ClearBrowsingDataHandler::OnJavascriptAllowed() {
   PrefService* prefs = profile_->GetPrefs();
   profile_pref_registrar_.Init(prefs);
   profile_pref_registrar_.Add(
       prefs::kAllowDeletingBrowserHistory,
       base::Bind(&ClearBrowsingDataHandler::OnBrowsingHistoryPrefChanged,
                  base::Unretained(this)));
 
   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(
     const base::ListValue* args) {
   DCHECK(!task_observer_);
 
   PrefService* prefs = profile_->GetPrefs();
 
   int site_data_mask = BrowsingDataRemover::REMOVE_SITE_DATA;
   // Don't try to clear LSO data if it's not supported.
@@ skipping 124 matching lines @@
       base::FundamentalValue(
           profile_->GetPrefs()->GetBoolean(
               prefs::kAllowDeletingBrowserHistory)));
 }
 
 void ClearBrowsingDataHandler::HandleInitialize(const base::ListValue* args) {
   AllowJavascript();
   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();
 
   ResolveJavascriptCallback(
       *callback_id,
       *base::Value::CreateNullValue() /* Promise<void> */);
 }
 
 void ClearBrowsingDataHandler::OnStateChanged() {
   CallJavascriptFunction(
@@ skipping 52 matching lines @@
 void ClearBrowsingDataHandler::UpdateCounterText(
     std::unique_ptr<browsing_data::BrowsingDataCounter::Result> result) {
   CallJavascriptFunction(
       "cr.webUIListenerCallback",
       base::StringValue("update-counter-text"),
       base::StringValue(result->source()->GetPrefName()),
       base::StringValue(GetChromeCounterTextFromResult(result.get())));
 }
 
 }  // namespace settings