Support the Clear-Site-Data header on resource requests

content/browser/browsing_data/clear_site_data_throttle.cc

 // Copyright 2016 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 "content/browser/browsing_data/clear_site_data_throttle.h"
 
 #include "base/command_line.h"
 #include "base/json/json_reader.h"
 #include "base/json/json_string_value_serializer.h"
 #include "base/memory/ptr_util.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
 #include "base/values.h"
-#include "content/browser/frame_host/navigation_handle_impl.h"
+#include "content/browser/service_worker/service_worker_response_info.h"
 #include "content/public/browser/browser_context.h"
-#include "content/public/browser/content_browser_client.h"
-#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/browser_thread.h"
+#include "content/public/browser/browsing_data_filter_builder.h"
+#include "content/public/browser/browsing_data_remover.h"
+#include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
-#include "content/public/common/content_client.h"
 #include "content/public/common/content_switches.h"
 #include "content/public/common/origin_util.h"
+#include "content/public/common/resource_response_info.h"
+#include "net/base/load_flags.h"
+#include "net/base/registry_controlled_domains/registry_controlled_domain.h"
 #include "net/http/http_response_headers.h"
+#include "net/url_request/redirect_info.h"
 #include "url/gurl.h"
 #include "url/origin.h"
 
 namespace content {
 
 namespace {
 
+static const char* kNameForLogging = "ClearSiteDataThrottle";
+
 static const char* kClearSiteDataHeader = "Clear-Site-Data";
 
 static const char* kTypesKey = "types";
 
 // Pretty-printed log output.
 static const char* kConsoleMessagePrefix = "Clear-Site-Data header on '%s': %s";
 static const char* kClearingOneType = "Clearing %s.";
 static const char* kClearingTwoTypes = "Clearing %s and %s.";
 static const char* kClearingThreeTypes = "Clearing %s, %s, and %s.";
 
-// Console logging. Adds a |text| message with |level| to |messages|.
-void ConsoleLog(std::vector<ClearSiteDataThrottle::ConsoleMessage>* messages,
-                const GURL& url,
-                const std::string& text,
-                ConsoleMessageLevel level) {
-  messages->push_back({url, text, level});
-}
-
 bool AreExperimentalFeaturesEnabled() {
   return base::CommandLine::ForCurrentProcess()->HasSwitch(
       switches::kEnableExperimentalWebPlatformFeatures);
 }
 
 // Represents the parameters as a single number to be recorded in a histogram.
 int ParametersMask(bool clear_cookies, bool clear_storage, bool clear_cache) {
   return static_cast<int>(clear_cookies) * (1 << 0) +
          static_cast<int>(clear_storage) * (1 << 1) +
          static_cast<int>(clear_cache) * (1 << 2);
 }
 
-}  // namespace
-
-// static
-std::unique_ptr<NavigationThrottle>
-ClearSiteDataThrottle::CreateThrottleForNavigation(NavigationHandle* handle) {
-  if (AreExperimentalFeaturesEnabled())
-    return base::WrapUnique(new ClearSiteDataThrottle(handle));
-
-  return std::unique_ptr<NavigationThrottle>();
+// A helper function to pass an IO thread callback to a method called on
+// the UI thread.
+void JumpFromUIToIOThread(const base::Closure& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  BrowserThread::PostTask(BrowserThread::IO, FROM_HERE, callback);
 }
 
-ClearSiteDataThrottle::ClearSiteDataThrottle(
-    NavigationHandle* navigation_handle)
-    : NavigationThrottle(navigation_handle),
-      clearing_in_progress_(false),
-      weak_ptr_factory_(this) {}
+// A BrowsingDataRemover::Observer that waits for |count|
+// OnBrowsingDataRemoverDone() callbacks, translates them into
+// one base::Closure, and then destroys itself.
+class ClearSiteDataObserver : public BrowsingDataRemover::Observer {
+ public:
+  explicit ClearSiteDataObserver(BrowsingDataRemover* remover,
+                                 const base::Closure& callback,
+                                 int count)

[mmenke, 2017/05/22 18:35:35]

count seems too ambiguous here.  expected_completion_calls, maybe?  Open to
other names.  Same for the member variable.

Or, better, maybe make this take the three clear_ bools from
ClearSiteDataOnUIThread, and move all of its logic here?  Then you don't have
the somewhat silly OnBrowsingDataRemoverDone() calls in that method, instead,
you just increment count as needed...Or can browsing data remover invoke
callbacks synchronously?  If so, may need to code around that, but still think
it's cleaner to have the logic in here.

[msramek, 2017/05/24 22:59:51]

Done - merged ClearSiteDataOnUIThread() and this Observer into one class.

+      : remover_(remover), callback_(callback), count_(count) {
+    remover_->AddObserver(this);
+  }
 
-ClearSiteDataThrottle::~ClearSiteDataThrottle() {
-  // At the end of the navigation we finally have access to the correct
-  // RenderFrameHost. Output the cached console messages. Prefix each sequence
-  // of messages belonging to the same URL with |kConsoleMessagePrefix|.
+  ~ClearSiteDataObserver() override { remover_->RemoveObserver(this); }
+
+  // BrowsingDataRemover::Observer.
+  void OnBrowsingDataRemoverDone() override {
+    DCHECK(count_);
+    if (--count_)
+      return;
+
+    JumpFromUIToIOThread(callback_);
+    delete this;
+  }
+
+ private:
+  BrowsingDataRemover* remover_;
+  base::Closure callback_;
+  int count_;
+};
+
+// Finds the BrowserContext associated with the request and requests
+// the actual clearing of data for |origin|. The datatypes to be deleted
+// are determined by |clear_cookies|, |clear_storage|, and |clear_cache|.
+// |web_contents_getter| identifies the WebContents from which the request
+// originated. Must be run on the UI thread. The |callback| will be executed
+// on the IO thread.
+void ClearSiteDataOnUIThread(
+    const ResourceRequestInfo::WebContentsGetter& web_contents_getter,
+    url::Origin origin,
+    bool clear_cookies,
+    bool clear_storage,
+    bool clear_cache,
+    const base::Closure& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  WebContents* web_contents = web_contents_getter.Run();
+  if (!web_contents)
+    return;
+
+  BrowsingDataRemover* remover =
+      BrowserContext::GetBrowsingDataRemover(web_contents->GetBrowserContext());
+
+  // ClearSiteDataObserver deletes itself when callbacks from both removal
+  // tasks are received.
+  ClearSiteDataObserver* observer =
+      new ClearSiteDataObserver(remover, callback, 2 /* number of tasks */);
+
+  // Cookies and channel IDs are scoped to
+  // a) eTLD+1 of |origin|'s host if |origin|'s host is a registrable domain
+  //    or a subdomain thereof
+  // b) |origin|'s host exactly if it is an IP address or an internal hostname
+  //    (e.g. "localhost" or "fileserver").
+  // TODO(msramek): What about plugin data?
+  if (clear_cookies) {
+    std::string domain = GetDomainAndRegistry(
+        origin.host(),
+        net::registry_controlled_domains::INCLUDE_PRIVATE_REGISTRIES);
+
+    if (domain.empty())
+      domain = origin.host();  // IP address or internal hostname.
+
+    std::unique_ptr<BrowsingDataFilterBuilder> domain_filter_builder(
+        BrowsingDataFilterBuilder::Create(
+            BrowsingDataFilterBuilder::WHITELIST));
+    domain_filter_builder->AddRegisterableDomain(domain);
+
+    remover->RemoveWithFilterAndReply(
+        base::Time(), base::Time::Max(),
+        BrowsingDataRemover::DATA_TYPE_COOKIES |
+            BrowsingDataRemover::DATA_TYPE_CHANNEL_IDS,
+        BrowsingDataRemover::ORIGIN_TYPE_UNPROTECTED_WEB |
+            BrowsingDataRemover::ORIGIN_TYPE_PROTECTED_WEB,
+        std::move(domain_filter_builder), observer);
+  } else {
+    // The first removal task is a no-op.
+    observer->OnBrowsingDataRemoverDone();
+  }
+
+  // Delete origin-scoped data.
+  int remove_mask = 0;
+  if (clear_storage)
+    remove_mask |= BrowsingDataRemover::DATA_TYPE_DOM_STORAGE;
+  if (clear_cache)
+    remove_mask |= BrowsingDataRemover::DATA_TYPE_CACHE;
+
+  if (remove_mask) {
+    std::unique_ptr<BrowsingDataFilterBuilder> origin_filter_builder(
+        BrowsingDataFilterBuilder::Create(
+            BrowsingDataFilterBuilder::WHITELIST));
+    origin_filter_builder->AddOrigin(origin);
+
+    remover->RemoveWithFilterAndReply(
+        base::Time(), base::Time::Max(), remove_mask,
+        BrowsingDataRemover::ORIGIN_TYPE_UNPROTECTED_WEB |
+            BrowsingDataRemover::ORIGIN_TYPE_PROTECTED_WEB,
+        std::move(origin_filter_builder), observer);
+  } else {
+    // The second removal task is a no-op.
+    observer->OnBrowsingDataRemoverDone();
+  }
+}
+
+// Outputs |messages| to the console of WebContents retrieved from
+// |web_contents_getter|. Must be run on the UI thread.
+void OutputMessagesOnUIThread(
+    const ResourceRequestInfo::WebContentsGetter& web_contents_getter,
+    const std::vector<ClearSiteDataThrottle::ConsoleMessagesDelegate::Message>&
+        messages) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+
+  WebContents* web_contents = web_contents_getter.Run();
+  if (!web_contents)
+    return;
+
+  // Prefix each sequence of messages belonging to the same URL with
+  // |kConsoleMessagePrefix|.
   GURL last_seen_url;
-  for (const ConsoleMessage& message : messages_) {
+  for (const auto& message : messages) {
     if (message.url == last_seen_url) {

[mmenke, 2017/05/22 18:35:35]

Any reason to do these all at the end, instead of on a per-request basis?  i.e.,
just dump messages after each HandleHeader call, and use the current URL (If
there are any messages to pass).

That also makes the "Clearing" in strings not a lie - currently, unless the
request is cancelled while clearing, when we print out "clearing foo", we've
already long since cleared "foo".

[msramek, 2017/05/24 22:59:51]

That's a good point - changed "Clearing..." To "Cleared...".

The reason why we only output this at the end are navigations. When this was a
NavigationThrottle, the most reasonable usage was a navigation redirect chain:

Deletion request -> URL that returns the header -> landing page.

...in which case, the output would never be visible, as the DevTools would only
show messages from the landing page.

Since we support subresource requests with the CL, maybe we don't care about
that case anymore, I'm not sure. Or we could handle it separately.

[mmenke, 2017/05/25 15:19:01]

Hrm....But for subresources, clearing will affect future resource requests, so
delaying it seems incorrect.  I'll defer to you here, but not a fan of the
current logic.

[msramek, 2017/05/30 21:58:44]

But we're not delaying the clearing itself, just the console output?

The clearing actually happens at each redirect. The developer always sees what
was cleared, but only when the request ends, not in real time. This might matter
in some cases, but it's mostly a cosmetic problem.

Perhaps a good solution would be to keep the current behavior if the request
type is navigation, and flush with each redirect if it's a subresource request.

But again - at this point I'm trying to make the header work at all. Pretty
console outputs can be done in a followup CL, especially with the feedback from
developers.

[mmenke, 2017/05/30 22:58:35]

My main concern is subresources - events will be logged in orders other than the
order in which they occurred.  This can be a bit confusing, and I'd rather not
have Internals>network>cookies bugs about this, particularly as no one owns the
label, which means there's a fair chance I'll be the one looking at them,
despite never having owned the cookie system, because no one else looks at such
bugs...  And I'd rather not have to deal with investigating them.  So this is
primarily motivated by a desire to save Chrome developer time (Especially my
own), by making the interface less confusing.  Fact is that errors in this sort
of thing will most often end up either in front of the network team, or perhaps
the DevTools team, so I'm trying to minimize our maintenance burden.

[msramek, 2017/06/01 22:02:27]

Fair enough. For the record, of course, please do route related bugs to me.

I implemented what I proposed in the previous comment - flushing the console
output with each redirect if it's a subresource request. Also added a test for
that.

-      navigation_handle()->GetRenderFrameHost()->AddMessageToConsole(
-          message.level, message.text);
+      web_contents->GetMainFrame()->AddMessageToConsole(message.level,
+                                                        message.text);
     } else {
-      navigation_handle()->GetRenderFrameHost()->AddMessageToConsole(
+      web_contents->GetMainFrame()->AddMessageToConsole(
           message.level,
           base::StringPrintf(kConsoleMessagePrefix, message.url.spec().c_str(),
                              message.text.c_str()));
     }
 
     last_seen_url = message.url;
   }
 }
 
-ClearSiteDataThrottle::ThrottleCheckResult
-ClearSiteDataThrottle::WillStartRequest() {
-  current_url_ = navigation_handle()->GetURL();
-  return PROCEED;
+}  // namespace
+
+////////////////////////////////////////////////////////////////////////////////
+// ConsoleMessagesDelegate
+
+ClearSiteDataThrottle::ConsoleMessagesDelegate::ConsoleMessagesDelegate() {}
+
+ClearSiteDataThrottle::ConsoleMessagesDelegate::~ConsoleMessagesDelegate() {}
+
+void ClearSiteDataThrottle::ConsoleMessagesDelegate::AddMessage(
+    const GURL& url,
+    const std::string& text,
+    ConsoleMessageLevel level) {
+  messages_.push_back({url, text, level});
 }
 
-ClearSiteDataThrottle::ThrottleCheckResult
-ClearSiteDataThrottle::WillRedirectRequest() {
-  // We are processing a redirect from url1 to url2. GetResponseHeaders()
-  // contains headers from url1, but GetURL() is already equal to url2. Handle
-  // the headers before updating the URL, so that |current_url_| corresponds
-  // to the URL that sent the headers.
-  HandleHeader();
-  current_url_ = navigation_handle()->GetURL();
+void ClearSiteDataThrottle::ConsoleMessagesDelegate::OutputMessages(
+    const ResourceRequestInfo::WebContentsGetter& web_contents_getter) {
+  if (messages_.empty())
+    return;
 
-  return clearing_in_progress_ ? DEFER : PROCEED;
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&OutputMessagesOnUIThread, web_contents_getter,
+                 std::move(messages_)));
+
+  messages_.clear();

[mmenke, 2017/05/22 18:35:34]

Is this needed, or is std::move guaranteed to clear the source vector?

[msramek, 2017/05/24 22:59:51]

I'm not sure that std::vector<> is safe to use after move. What I did find is
that "clear()" is always safe to call, and always brings the vector to valid
empty state.

(I think that in practice, it's safe to omit this, but I'd like to play it
safe.)

[mmenke, 2017/05/25 15:19:01]

Googling for an answer, looks like it may not be guaranteed, so this makes
sense.

[msramek, 2017/05/30 21:58:44]

Acknowledged.

 }
 
-ClearSiteDataThrottle::ThrottleCheckResult
-ClearSiteDataThrottle::WillProcessResponse() {
-  HandleHeader();
-  return clearing_in_progress_ ? DEFER : PROCEED;
+////////////////////////////////////////////////////////////////////////////////
+// ClearSiteDataThrottle
+
+// static
+std::unique_ptr<ResourceThrottle>
+ClearSiteDataThrottle::CreateThrottleForRequest(net::URLRequest* request) {
+  // This is an experimental feature.
+  if (!AreExperimentalFeaturesEnabled())
+    return std::unique_ptr<ResourceThrottle>();
+
+  // The throttle has no purpose if the request has no ResourceRequestInfo,
+  // because we won't be able to determine whose data should be deleted.
+  if (!ResourceRequestInfo::ForRequest(request))
+    return std::unique_ptr<ResourceThrottle>();
+
+  return base::WrapUnique(new ClearSiteDataThrottle(
+      request, base::MakeUnique<ConsoleMessagesDelegate>()));
 }
 
-const char* ClearSiteDataThrottle::GetNameForLogging() {
-  return "ClearSiteDataThrottle";
+ClearSiteDataThrottle::ClearSiteDataThrottle(
+    net::URLRequest* request,
+    std::unique_ptr<ConsoleMessagesDelegate> delegate)
+    : request_(request),
+      delegate_(std::move(delegate)),
+      weak_ptr_factory_(this) {
+  DCHECK(request_);
+  DCHECK(delegate_);
 }
 
-void ClearSiteDataThrottle::HandleHeader() {
-  NavigationHandleImpl* handle =
-      static_cast<NavigationHandleImpl*>(navigation_handle());
-  const net::HttpResponseHeaders* headers = handle->GetResponseHeaders();
+ClearSiteDataThrottle::~ClearSiteDataThrottle() {
+  // Output the cached console messages. We output console messages when the
+  // request is finished rather than in real time, since in case of navigations
+  // swapping RenderFrameHost would cause the outputs to disappear.
+  LOG(ERROR) << "*** DEBUG OUTPUT FOR TRYBOTS *** Destructor " << this;
+  const ResourceRequestInfo* info = ResourceRequestInfo::ForRequest(request_);
+  if (info)
+    delegate_->OutputMessages(info->GetWebContentsGetterForRequest());
+}
 
-  if (!headers || !headers->HasHeader(kClearSiteDataHeader))
-    return;
+void ClearSiteDataThrottle::WillRedirectRequest(
+    const net::RedirectInfo& redirect_info,
+    bool* defer) {
+  *defer = HandleHeader();
+}
 
-  // Only accept the header on secure origins.
-  if (!IsOriginSecure(current_url_)) {
-    ConsoleLog(&messages_, current_url_, "Not supported for insecure origins.",
-               CONSOLE_MESSAGE_LEVEL_ERROR);
-    return;
+void ClearSiteDataThrottle::WillProcessResponse(bool* defer) {
+  *defer = HandleHeader();
+}
+
+const char* ClearSiteDataThrottle::GetNameForLogging() const {
+  return kNameForLogging;
+}
+
+bool ClearSiteDataThrottle::HandleHeader() {
+  const net::HttpResponseHeaders* headers = GetResponseHeaders();
+
+  std::string header_value;
+  if (!headers ||
+      !headers->GetNormalizedHeader(kClearSiteDataHeader, &header_value)) {
+    return false;
   }
 
-  std::string header_value;
-  headers->GetNormalizedHeader(kClearSiteDataHeader, &header_value);
+  // Only accept the header on secure non-unique origins.
+  if (!IsOriginSecure(request_->url())) {

[mmenke, 2017/05/22 18:35:35]

Hrm....bizarrely, we don't have a version of IsOriginSecure that takes or
origin, though it does compute the GURL version of origin (As opposed to the
Origin version of an origin) internally.  No action needed, this just makes me
sad.

[msramek, 2017/05/24 22:59:51]

Acknowledged. I'd expect that we can just move it to url::Origin (Mike?).

+    delegate_->AddMessage(request_->url(),
+                          "Not supported for insecure origins.",
+                          CONSOLE_MESSAGE_LEVEL_ERROR);
+    return false;
+  }
+
+  url::Origin origin(request_->url());
+  if (origin.unique()) {
+    delegate_->AddMessage(request_->url(), "Not supported for unique origins.",
+                          CONSOLE_MESSAGE_LEVEL_ERROR);
+    return false;
+  }
+
+  // The LOAD_DO_NOT_SAVE_COOKIES flag prohibits the request from doing any
+  // modification to cookies. Clear-Site-Data applies this restriction to other
+  // datatypes as well.
+  if (request_->load_flags() & net::LOAD_DO_NOT_SAVE_COOKIES) {
+    delegate_->AddMessage(
+        request_->url(),
+        "The request's credentials mode prohibits modifying cookies "
+        "and other local data.",
+        CONSOLE_MESSAGE_LEVEL_ERROR);

[mmenke, 2017/05/22 18:35:34]

Hrm...We don't log this for normal requests with Set-cookie headers and
LOAD_DO_NOT_SAVE_COOKIES, I assume?  Should we log it in this much less common
case, then?

[msramek, 2017/05/24 22:59:51]

We wouldn't log anything to the console, but I think we would display a crossed
cookie icon in the omnibox. We don't have that here, so I wanted to give at
least some feedback to the developer.

I added a TODO to find a proper treatment.

+    return false;
+  }
+
+  // Service workers can handle fetches of third-party resources and inject
+  // arbitrary headers. Ignore responses that came from a service worker,
+  // as supporting Clear-Site-Data would give them the power to delete data from
+  // any website.
+  // See https://w3c.github.io/webappsec-clear-site-data/#service-workers
+  // for more information.
+  const ServiceWorkerResponseInfo* response_info =
+      ServiceWorkerResponseInfo::ForRequest(request_);
+  if (response_info) {
+    ResourceResponseInfo extra_response_info;
+    response_info->GetExtraResponseInfo(&extra_response_info);
+
+    if (extra_response_info.was_fetched_via_service_worker) {
+      delegate_->AddMessage(
+          request_->url(),
+          "Ignoring, as the response came from a service worker.",
+          CONSOLE_MESSAGE_LEVEL_ERROR);
+      return false;
+    }
+  }
 
   bool clear_cookies;
   bool clear_storage;
   bool clear_cache;
 
   if (!ParseHeader(header_value, &clear_cookies, &clear_storage, &clear_cache,

[mmenke, 2017/05/22 18:35:34]

Know this is old code, but can we use a less generic name for it?  It's not
clear from context this is a ClearSideDataThrottle method, and it sounds like a
general method name.

[msramek, 2017/05/24 22:59:51]

How about simply prefixing it with the class name here? It's a static method, so
everyone who would use the method outside this class will have to do so anyway.

And then ClearSiteDataThrottle::ParseClearSiteDataThrottle would look like an
unnecessary duplication.

-                   &messages_)) {
-    return;
+                   delegate_.get(), request_->url())) {
+    return false;
   }
 
+  // If the header is valid, clear the data for this browser context and origin.
+  clearing_started_ = base::TimeTicks::Now();
+
   // Record the call parameters.
   UMA_HISTOGRAM_ENUMERATION(
       "Navigation.ClearSiteData.Parameters",
       ParametersMask(clear_cookies, clear_storage, clear_cache), (1 << 3));
 
-  // If the header is valid, clear the data for this browser context and origin.
-  BrowserContext* browser_context =
-      navigation_handle()->GetWebContents()->GetBrowserContext();
-  url::Origin origin(current_url_);
+  base::WeakPtr<ClearSiteDataThrottle> weak_ptr =
+      weak_ptr_factory_.GetWeakPtr();
 
-  if (origin.unique()) {
-    ConsoleLog(&messages_, current_url_, "Not supported for unique origins.",
-               CONSOLE_MESSAGE_LEVEL_ERROR);
-    return;
-  }
+  // Immediately bind the weak pointer to the current thread (IO). This will
+  // make a potential misuse on the UI thread DCHECK immediately rather than
+  // later when it's correctly used on the IO thread again.
+  weak_ptr.get();

[mmenke, 2017/05/22 18:35:34]

I don't think this is needed - GetWeakPtr already binds the weak ptr yo this
thread, no?  Per spec, the weak ptr can only be dereferenced on the thread
GetWeakPtr is called on:

// Note that GetWeakPtr must be called on one and only one TaskQueue or thread
// and the WeakPtr must only be dereferenced and invalidated on that same
// TaskQueue/thread.

[msramek, 2017/05/24 22:59:51]

But there is also this:

// To ensure correct use, the first time a WeakPtr issued by a WeakPtrFactory
// is dereferenced, the factory and its WeakPtrs become bound to the calling
// thread or current SequencedWorkerPool token, and cannot be dereferenced or
// invalidated on any other task runner.

So it's bound to the thread when dereferenced for the first time.

I have learned this recently when debugging a threading issue - the stack trace
from DCHECK was confusing, because the weak pointer was first dereferenced on
the wrong thread.

 
-  clearing_in_progress_ = true;
-  clearing_started_ = base::TimeTicks::Now();
-  GetContentClient()->browser()->ClearSiteData(
-      browser_context, origin, clear_cookies, clear_storage, clear_cache,
-      base::Bind(&ClearSiteDataThrottle::TaskFinished,
-                 weak_ptr_factory_.GetWeakPtr()));
+  ExecuteClearingTask(
+      origin, clear_cookies, clear_storage, clear_cache,
+      base::Bind(&ClearSiteDataThrottle::TaskFinished, weak_ptr));

[mmenke, 2017/05/22 18:35:34]

base::BindOnce + use std::move everywhere to pass the callback is now preferred,
I think?

[msramek, 2017/05/24 22:59:51]

Done. Yes, I think so too. Changed here and in tests, except where other
classes' interfaces required RepeatedCallback.

+
+  return true;
 }
 
+// static
 bool ClearSiteDataThrottle::ParseHeader(const std::string& header,

[mmenke, 2017/05/22 18:35:34]

This should be in the same order it's declared in in the header file.

[msramek, 2017/05/24 22:59:51]

Done. Reordered.

                                         bool* clear_cookies,
                                         bool* clear_storage,
                                         bool* clear_cache,
-                                        std::vector<ConsoleMessage>* messages) {
+                                        ConsoleMessagesDelegate* delegate,
+                                        const GURL& current_url) {
   if (!base::IsStringASCII(header)) {
-    ConsoleLog(messages, current_url_, "Must only contain ASCII characters.",
-               CONSOLE_MESSAGE_LEVEL_ERROR);
+    delegate->AddMessage(current_url, "Must only contain ASCII characters.",
+                         CONSOLE_MESSAGE_LEVEL_ERROR);
     return false;
   }
 
   std::unique_ptr<base::Value> parsed_header = base::JSONReader::Read(header);
 
   if (!parsed_header) {
-    ConsoleLog(messages, current_url_, "Not a valid JSON.",
-               CONSOLE_MESSAGE_LEVEL_ERROR);
+    delegate->AddMessage(current_url, "Not a valid JSON.",
+                         CONSOLE_MESSAGE_LEVEL_ERROR);
     return false;
   }
 
   const base::DictionaryValue* dictionary = nullptr;
   const base::ListValue* types = nullptr;
   if (!parsed_header->GetAsDictionary(&dictionary) ||
       !dictionary->GetListWithoutPathExpansion(kTypesKey, &types)) {
-    ConsoleLog(messages, current_url_,
-               "Expecting a JSON dictionary with a 'types' field.",
-               CONSOLE_MESSAGE_LEVEL_ERROR);
+    delegate->AddMessage(current_url,
+                         "Expecting a JSON dictionary with a 'types' field.",
+                         CONSOLE_MESSAGE_LEVEL_ERROR);
     return false;
   }
 
   DCHECK(types);
 
   *clear_cookies = false;
   *clear_storage = false;
   *clear_cache = false;
 
   std::vector<std::string> type_names;
   for (const base::Value& value : *types) {
     std::string type;
     value.GetAsString(&type);

[mmenke, 2017/05/22 18:35:35]

if (!value.GetAsString(&type))
  continue;
(With maybe a log message)?

[msramek, 2017/05/24 22:59:51]

This is handled in the "} else {" branch below, together with valid but unknown
strings. We do serialize |type| in the console message, so it should be clear
enough where was the syntax error, regardless of what |type| is.

 
     bool* datatype = nullptr;
 
     if (type == "cookies") {
       datatype = clear_cookies;
     } else if (type == "storage") {
       datatype = clear_storage;
     } else if (type == "cache") {

[mmenke, 2017/05/22 18:35:35]

optional:  Suggest making these 3 constants up top - they're only used once, but
so are kClearingOneType, and think it's nice to make it clear what types are
supported in a list of constants up top.

[msramek, 2017/05/24 22:59:52]

Done.

       datatype = clear_cache;
     } else {
       std::string serialized_type;
       JSONStringValueSerializer serializer(&serialized_type);
       serializer.Serialize(value);
-      ConsoleLog(
-          messages, current_url_,
-          base::StringPrintf("Invalid type: %s.", serialized_type.c_str()),
+      delegate->AddMessage(
+          current_url,
+          base::StringPrintf("Unrecognized type: %s.", serialized_type.c_str()),
           CONSOLE_MESSAGE_LEVEL_ERROR);
       continue;
     }
 
     // Each data type should only be processed once.
     DCHECK(datatype);

[mmenke, 2017/05/22 18:35:34]

This seems wrong - we shouldn't DCHECK if a site sends us bad data, and since
this is just a list, a server can send us a list with duplicate values, no?

[msramek, 2017/05/24 22:59:51]

I moved the comment one line below, where it's hopefully less confusing :)

Earlier in this method, |datatype| is initialized to nullptr. This DCHECK just
verifies that it has a value by now (bool |cookies|, bool |storage|, or bool
|cache|).

Then the "if" on the next line makes sure that we don't process something twice.

     if (*datatype)
       continue;
 
     *datatype = true;
     type_names.push_back(type);
   }
 
   if (!*clear_cookies && !*clear_storage && !*clear_cache) {
-    ConsoleLog(messages, current_url_,
-               "No valid types specified in the 'types' field.",
-               CONSOLE_MESSAGE_LEVEL_ERROR);
+    delegate->AddMessage(current_url,
+                         "No recognized types specified in the 'types' field.",
+                         CONSOLE_MESSAGE_LEVEL_ERROR);
     return false;
   }
 
   // Pretty-print which types are to be cleared.
   std::string output;
   switch (type_names.size()) {
     case 1:
       output = base::StringPrintf(kClearingOneType, type_names[0].c_str());
       break;
     case 2:
       output = base::StringPrintf(kClearingTwoTypes, type_names[0].c_str(),
                                   type_names[1].c_str());
       break;
     case 3:
       output = base::StringPrintf(kClearingThreeTypes, type_names[0].c_str(),
                                   type_names[1].c_str(), type_names[2].c_str());

[mmenke, 2017/05/22 18:35:35]

Since this isn't internationalized (I assume nothing in dev tools is?), suggest
just a loop - it's more general, and adding a case every time another option is
added seems silly, and likely to be something someone forgets to do, resulting
in a regression.

[msramek, 2017/05/24 22:59:52]

Done.

       break;
     default:
       NOTREACHED();
   }
-  ConsoleLog(messages, current_url_, output, CONSOLE_MESSAGE_LEVEL_INFO);
+  delegate->AddMessage(current_url, output, CONSOLE_MESSAGE_LEVEL_INFO);
 
   return true;
 }
 
+const net::HttpResponseHeaders* ClearSiteDataThrottle::GetResponseHeaders()
+    const {
+  return request_->response_headers();
+}
+
+void ClearSiteDataThrottle::ExecuteClearingTask(const url::Origin& origin,
+                                                bool clear_cookies,
+                                                bool clear_storage,
+                                                bool clear_cache,
+                                                const base::Closure& callback) {
+  DCHECK_CURRENTLY_ON(BrowserThread::IO);
+  BrowserThread::PostTask(
+      BrowserThread::UI, FROM_HERE,
+      base::Bind(&ClearSiteDataOnUIThread,
+                 ResourceRequestInfo::ForRequest(request_)
+                     ->GetWebContentsGetterForRequest(),
+                 origin, clear_cookies, clear_storage, clear_cache, callback));
+}
+
 void ClearSiteDataThrottle::TaskFinished() {

[mmenke, 2017/05/22 18:35:34]

DCHECK(!clearing_started_.is_null());

[mmenke, 2017/05/22 18:35:34]

DCHECK_CURRENTLY_ON(BrowserThread::IO);

[msramek, 2017/05/24 22:59:51]

I didn't add this on purpose, because the unittest is single-threaded (only the
actual deletion, which is mocked in the unittest, has these DCHECKs).

I didn't have time to look into this further today, but if you think the DCHECK
should be here, I can add real threads to the unittest in the next iteration.

[msramek, 2017/05/24 22:59:51]

Done.

[msramek, 2017/05/30 21:58:44]

Fixed this in the latest patchset.

-  DCHECK(clearing_in_progress_);
-  clearing_in_progress_ = false;
-
   UMA_HISTOGRAM_CUSTOM_TIMES("Navigation.ClearSiteData.Duration",
                              base::TimeTicks::Now() - clearing_started_,
                              base::TimeDelta::FromMilliseconds(1),
                              base::TimeDelta::FromSeconds(1), 50);
 
-  navigation_handle()->Resume();
+  LOG(ERROR) << "*** DEBUG OUTPUT FOR TRYBOTS *** Defer.";

[mmenke, 2017/05/22 18:35:34]

Remove this?

[msramek, 2017/05/24 22:59:51]

Done. (Here and elsewhere)

I still haven't debugged the red test, but this didn't help. It seems that
TitleWatcher doesn't work correctly on some platforms :/

+  Resume();
 }
 
 }  // namespace content