Support the Clear-Site-Data header on resource requests

content/browser/browsing_data/clear_site_data_throttle_unittest.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 <memory>
 
 #include "base/command_line.h"
+#include "base/message_loop/message_loop.h"
+#include "base/test/scoped_command_line.h"
+#include "content/common/net/url_request_service_worker_data.h"
 #include "content/public/common/content_switches.h"
+#include "net/base/load_flags.h"
+#include "net/url_request/url_request_test_util.h"
 #include "testing/gmock/include/gmock/gmock.h"
 #include "testing/gtest/include/gtest/gtest.h"
 
 namespace content {
 
 class ClearSiteDataThrottleTest : public testing::Test {
  public:
-  void SetUp() override {
-    base::CommandLine::ForCurrentProcess()->AppendSwitch(
-        switches::kEnableExperimentalWebPlatformFeatures);
-    throttle_ = ClearSiteDataThrottle::CreateThrottleForNavigation(nullptr);
-  }
+  void SetUp() override { throttle_.reset(new ClearSiteDataThrottle(nullptr)); }
 
   ClearSiteDataThrottle* GetThrottle() {
     return static_cast<ClearSiteDataThrottle*>(throttle_.get());
   }
 
  private:
-  std::unique_ptr<NavigationThrottle> throttle_;
+  base::MessageLoop message_loop_;
+  std::unique_ptr<ResourceThrottle> throttle_;
 };
 
+TEST_F(ClearSiteDataThrottleTest, CreateThrottleForRequest) {
+  // Enable experimental features.
+  std::unique_ptr<base::test::ScopedCommandLine> command_line(
+      new base::test::ScopedCommandLine());
+  command_line->GetProcessCommandLine()->AppendSwitch(
+      switches::kEnableExperimentalWebPlatformFeatures);
+
+  // Create a URL request.
+  GURL url("https://www.example.com");
+  net::TestURLRequestContext context;
+  std::unique_ptr<net::URLRequest> request(
+      context.CreateRequest(url, net::DEFAULT_PRIORITY, nullptr));
+
+  // We will not create the throttle for an empty ResourceRequestInfo.
+  EXPECT_FALSE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+
+  // We can create the throttle for a valid ResourceRequestInfo.
+  ResourceRequestInfo::AllocateForTesting(request.get(), RESOURCE_TYPE_IMAGE,
+                                          nullptr, 0, 0, 0, false, true, true,
+                                          true, false);
+  EXPECT_TRUE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+
+  // But not if experimental web features are disabled again.
+  request->SetLoadFlags(net::LOAD_NORMAL);
+  command_line.reset();
+  EXPECT_FALSE(ClearSiteDataThrottle::CreateThrottleForRequest(request.get()));
+}
+
 TEST_F(ClearSiteDataThrottleTest, ParseHeader) {

[mmenke, 2016/10/21 15:16:21]

We don't have any tests of the deferring behavior, which I think we should have.
 We could have a browser test that makes sure we clear cookies before following
a redirect, but that could flakily pass, while a well-written unit test would
not.

If we go with a browser test, I'd suggest a redirect test (Something we don't
have but need somewhere).  Set a cookie manually, request a resource which has
the clear header and redirects us to another site, which sets another cookie. 
Then make sure the first cookie was cleared, wasn't set with the request, and
the second one is set at the end of the test.

[mmenke, 2016/10/21 15:16:21]

I suggest making all these tests more integration-y.  De-friend the individual
tests, create a mock ResourceController that checks if a particular cookie /
whatever has been / has not been cleared on Resume, and then call the throttle's
WillStartRequest / WillRedirectRequest methods, as in production code, check if
we're deferred or not (as expected), and if deferred, wait for the resume call. 
That more fully checks all expected behaviors.

If the BrowsingDataRemover can't easily be used in unit tests, then just make a
virtual method that posts the task to call ClearSiteDataOnUIThread, then
subclass it, and not go through ClearSiteDataThrottle::CreateThrottleForRequest
for most of these tests.

The browser tests do a reasonable job, but think we could have better coverage
here, WDYT?  Not sure about mocking out stuff like console messages, either. 
Could be it's just too much work.

I'm not going to push too much on this, given the good coverage we're getting
from your browser tests.

[msramek, 2016/10/31 19:23:36]

I disassembled ClearSiteDataThrottle a bit to make it better mockable, added a
few tests for error messages and ClearSiteDataThrottleTest.DeferAndResume to
test deferring.

Regarding the the set-cookie-and-delete-cookie test, neither the unittest or
browsertest can reasonably do that, as the call goes through
ContentBrowserClient out to the browser. We could implement a mock cookie
storage / MockContentBrowserClient, but at that point we're just testing the
mock implementation, which is as good as testing EXPECT_CALL.

There is one problem where I got stuck, maybe you can help me there. I wanted to
write the test about deferring in the following way:

Create a request, create a throttle, then keep modifying the request (e.g.
redirect it somewhere) and EXPECT_CALLs on the throttle. The problem is that I
don't know how to reasonably modify the request - all interesting methods are
non-public. URLRequestJob is a friend that has access to them, but subclasses of
URLRequestJob are not friends.

We could for example run the request against an EmbeddedTestServer, but I
already have this kind of stuff in the browsertest, and don't want to copy over
the whole infrastructure.

So instead, I just always create a new request and then just manually run
WillRedirectRequest() or WillProcessResponse() and see what happens. This mostly
achieves what I want, but it's not elegant and the test could still pass if we
replaced URLRequest::url() with URLRequest::original_url(). Fortunately, we test
that in many other places.

[mmenke, 2016/11/08 18:23:59]

Hrm...That seems a little weird.  From a layering standpoint, this is part of a
web API, so doesn't seem like it should be dependent on Chrome to implement it,
just from a correctness standpoint.  Or, alternatively, content-shell should.

Beyond that, blimp, headless, views, cast, and Android WebView all fail to
implement the requisite method.  Do you have any plans here?  Is the plan to
support this feature on none of these?
Why do you want to modify the request?  You can register a request handler that
repeatedly returns redirects and varying headers.

   struct TestCase {
     const char* header;
     bool cookies;
     bool storage;
     bool cache;
   } test_cases[] = {
       // One data type.
       {"{ \"types\": [\"cookies\"] }", true, false, false},
       {"{ \"types\": [\"storage\"] }", false, true, false},
       {"{ \"types\": [\"cache\"] }", false, false, true},
@@ skipping 55 matching lines @@
       {"\"some text\"", "Expecting a JSON dictionary with a 'types' field.\n"},
       {"{ \"field\" : {} }",
        "Expecting a JSON dictionary with a 'types' field.\n"},
       {"{ \"types\" : [ \"passwords\" ] }",
        "Invalid type: \"passwords\".\n"
        "No valid types specified in the 'types' field.\n"},
       {"{ \"types\" : [ [ \"list in a list\" ] ] }",
        "Invalid type: [\"list in a list\"].\n"
        "No valid types specified in the 'types' field.\n"},
       {"{ \"types\" : [ \"кукис\", \"сторидж\", \"кэш\" ]",
        "Must only contain ASCII characters.\n"}};

[mmenke, 2016/10/21 15:16:21]

How hard would it be to write integration tests that make sure these actually
get to the console, or unit tests that make sure these are sent to the method
that makes sure these actually get to the console?

[msramek, 2016/10/31 19:23:36]

The call goes to RenderFrameHostImpl. So far, I can see that

a) RenderFrameHost is abstract, but there isn't any testing subclass anywhere
which is suspicious
b) RenderFrameHostImpl is not mockable
c) RenderFrameImpl is in the renderer

I'm not sure what's the best approach. Given that
- they're not super important
- everything around console messages is tested except that they actually make it
to the console, and that part is easy to verify visually
- this CL has already grown quite a bit

...I would prefer to leave this for another time :)

 
   for (const TestCase& test_case : test_cases) {
     SCOPED_TRACE(test_case.header);
 
     bool actual_cookies;
     bool actual_storage;
     bool actual_cache;
 
     std::vector<ClearSiteDataThrottle::ConsoleMessage> messages;
 
     EXPECT_FALSE(GetThrottle()->ParseHeader(test_case.header, &actual_cookies,
                                             &actual_storage, &actual_cache,
                                             &messages));
 
     std::string multiline_message;
     for (const auto& message : messages) {
       EXPECT_EQ(CONSOLE_MESSAGE_LEVEL_ERROR, message.level);
       multiline_message += message.text + "\n";
     }
 
     EXPECT_EQ(test_case.console_message, multiline_message);
   }
 }
 
 }  // namespace content