ActivationStateComputingThrottle unit tests

components/subresource_filter/content/browser/activation_state_computing_navigation_throttle_unittest.cc

+// Copyright 2017 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 "components/subresource_filter/content/browser/activation_state_computing_navigation_throttle.h"
+
+#include <memory>
+#include <utility>
+
+#include "base/callback.h"
+#include "base/memory/ptr_util.h"
+#include "base/optional.h"
+#include "base/run_loop.h"
+#include "base/test/test_simple_task_runner.h"
+#include "components/subresource_filter/content/browser/async_document_subresource_filter.h"
+#include "components/subresource_filter/content/browser/async_document_subresource_filter_test_utils.h"
+#include "components/subresource_filter/core/common/activation_level.h"
+#include "components/subresource_filter/core/common/activation_state.h"
+#include "components/subresource_filter/core/common/proto/rules.pb.h"
+#include "components/subresource_filter/core/common/test_ruleset_creator.h"
+#include "components/subresource_filter/core/common/test_ruleset_utils.h"
+#include "content/public/browser/navigation_handle.h"
+#include "content/public/browser/web_contents_observer.h"
+#include "content/public/common/referrer.h"

[engedy, 2017/03/10 10:56:57]

nit: Unused?

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+#include "content/public/test/navigation_simulator.h"
+#include "content/public/test/test_renderer_host.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace subresource_filter {
+
+class ActivationStateComputingNavigationThrottleTest
+    : public content::RenderViewHostTestHarness,
+      public content::WebContentsObserver {
+ public:
+  ActivationStateComputingNavigationThrottleTest() {}
+  ~ActivationStateComputingNavigationThrottleTest() override {}
+
+  void SetUp() override {
+    content::RenderViewHostTestHarness::SetUp();
+    NavigateAndCommit(GURL("https://example.first"));
+    InitializeRuleset();
+    Observe(RenderViewHostTestHarness::web_contents());
+  }
+
+  void TearDown() override {
+    async_filter_.reset();
+    ruleset_handle_.reset();
+    dealer_handle_.reset();
+    RunUntilIdle();
+    content::RenderViewHostTestHarness::TearDown();
+  }
+
+  // content::WebContentsObserver:

[engedy, 2017/03/10 10:56:57]

nit: Could you please move these further down into a protected: section?

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+  void DidStartNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    std::unique_ptr<ActivationStateComputingNavigationThrottle> throttle =
+        navigation_handle->IsInMainFrame()
+            ? ActivationStateComputingNavigationThrottle::CreateForMainFrame(
+                  navigation_handle)
+            : ActivationStateComputingNavigationThrottle::CreateForSubframe(
+                  navigation_handle, ruleset_handle_.get(),
+                  parent_activation_state_.value());
+    test_throttle_ = throttle.get();
+    navigation_handle->RegisterThrottleForTesting(std::move(throttle));
+  }
+
+  void ReadyToCommitNavigation(
+      content::NavigationHandle* navigation_handle) override {
+    if (!test_throttle_)
+      return;
+    ASSERT_EQ(navigation_handle, test_throttle_->navigation_handle());
+    last_activation_state_ = test_throttle_->GetActivationState();
+    if (last_activation_state_.value().activation_level ==
+        ActivationLevel::DISABLED) {
+      EXPECT_FALSE(test_throttle_->ReleaseFilter());
+    } else {
+      EXPECT_TRUE(test_throttle_->ReleaseFilter());
+    }
+    test_throttle_ = nullptr;
+  }
+
+  void InitializeRuleset() {
+    std::vector<proto::UrlRule> rules;
+    rules.push_back(testing::CreateWhitelistRuleForDocument(
+        "whitelist.com", proto::ACTIVATION_TYPE_DOCUMENT, {"parent.com"}));

[engedy, 2017/03/10 10:56:57]

nit: Let's call these whitelisted.com and allow-child-to-be-whitelisted.com

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+    ASSERT_NO_FATAL_FAILURE(test_ruleset_creator_.CreateRulesetWithRules(
+        rules, &test_ruleset_pair_));
+
+    // Make the blocking task runner run on the current task runner for the
+    // tests, to ensure that the NavigationSimulator properly runs all necessary
+    // tasks while waiting for throttle checks to finish.
+    dealer_handle_ = base::MakeUnique<VerifiedRulesetDealer::Handle>(
+        base::MessageLoop::current()->task_runner());
+    dealer_handle_->SetRulesetFile(
+        testing::TestRuleset::Open(test_ruleset_pair_.indexed));
+    ruleset_handle_ =
+        base::MakeUnique<VerifiedRuleset::Handle>(dealer_handle_.get());
+  }
+  void InitializeDocumentSubresourceFilter(const GURL& document_url) {
+    NavigateAndCommit(document_url);

[engedy, 2017/03/10 10:56:57]

Apart from this line, this function has no effect (i.e. |async_filter_| never
used).

I am fine with solutions on both end of the spectrum:
 -- Let the tests for subframe activation always use a hardcoded parent
activation state, in which case this method can be replaced with just calling
NavigateAndCommit.
 -- For these tests, trigger a real calculation of ActivationState in the
parent, which is then somehow implicitly read by
CreateSubframeAndInitTestNavigation. This might make some propagation tests a
lot harder, though.

[Charlie Harrison, 2017/03/10 17:12:47]

Oops, sorry about that. I've reworked the tests to do your second idea, by
persisting both the last activation state calculation, as well as the
last committed render frame host.

+
+    testing::TestActivationStateCallbackReceiver activation_state;
+    async_filter_ = base::MakeUnique<AsyncDocumentSubresourceFilter>(
+        ruleset_handle_.get(),
+        AsyncDocumentSubresourceFilter::InitializationParams(
+            document_url, ActivationLevel::ENABLED,
+            false /* measure_performance */),
+        activation_state.GetCallback(), base::OnceClosure());
+    RunUntilIdle();
+    activation_state.ExpectReceivedOnce(
+        ActivationState(ActivationLevel::ENABLED));
+  }
+
+  void RunUntilIdle() { base::RunLoop().RunUntilIdle(); }
+
+  void CreateTestNavigationForMainFrame(const GURL& first_url) {
+    navigation_simulator_ =
+        content::NavigationSimulator::CreateRendererInitiated(first_url,
+                                                              main_rfh());
+  }
+
+  void CreateSubframeAndInitTestNavigation(
+      const GURL& first_url,
+      const ActivationState& parent_activation_state) {
+    parent_activation_state_ = parent_activation_state;
+    content::RenderFrameHost* navigating_subframe =
+        content::RenderFrameHostTester::For(main_rfh())
+            ->AppendChild("subframe");
+    navigation_simulator_ =
+        content::NavigationSimulator::CreateRendererInitiated(
+            first_url, navigating_subframe);
+  }
+
+  void SimulateStartAndExpectToProceed() {
+    ASSERT_TRUE(navigation_simulator_);
+    navigation_simulator_->Start();
+    EXPECT_EQ(content::NavigationThrottle::PROCEED,
+              navigation_simulator_->GetLastThrottleCheckResult());
+  }
+
+  void SimulateRedirectAndExpectToProceed(const GURL& new_url) {
+    navigation_simulator_->Redirect(new_url);
+    EXPECT_EQ(content::NavigationThrottle::PROCEED,
+              navigation_simulator_->GetLastThrottleCheckResult());
+  }
+
+  void SimulateCommitAndExpectToProceed() {
+    navigation_simulator_->Commit();
+    EXPECT_EQ(content::NavigationThrottle::PROCEED,
+              navigation_simulator_->GetLastThrottleCheckResult());
+  }
+
+  void NotifyPageActivation(ActivationState state) {
+    test_throttle_->NotifyPageActivationWithRuleset(
+        state.activation_level == ActivationLevel::DISABLED
+            ? nullptr
+            : ruleset_handle_.get(),
+        state);
+  }
+
+  const ActivationState& last_activation_state() {
+    return last_activation_state_.value();

[engedy, 2017/03/10 10:56:57]

nit: This essentially EXPECT's that las_ will have a value, but DCHECK if the
expectation is not met, which is impolite as it will bring down the entire
unittest. How about:

EXPECT_TRUE(las.has_value());
return las.value_or(...);

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+  }
+
+ private:
+  testing::TestRulesetCreator test_ruleset_creator_;
+  testing::TestRulesetPair test_ruleset_pair_;
+
+  std::unique_ptr<VerifiedRulesetDealer::Handle> dealer_handle_;
+  std::unique_ptr<VerifiedRuleset::Handle> ruleset_handle_;
+
+  std::unique_ptr<AsyncDocumentSubresourceFilter> async_filter_;
+
+  std::unique_ptr<content::NavigationSimulator> navigation_simulator_;
+
+  // Owned by the current navigation.
+  ActivationStateComputingNavigationThrottle* test_throttle_;
+  base::Optional<ActivationState> last_activation_state_;
+  base::Optional<ActivationState> parent_activation_state_;
+
+  DISALLOW_COPY_AND_ASSIGN(ActivationStateComputingNavigationThrottleTest);

[engedy, 2017/03/10 10:56:57]

#include "base/macros.h"

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+};
+
+// Main frame tests
+TEST_F(ActivationStateComputingNavigationThrottleTest, ActivateMainFrame) {

[engedy, 2017/03/10 11:53:41]

optional nit: If we had two test fixtures,
ActivationStateComputingThrottle{Main|Sub}FrameTest, we would have an easier
time naming the tests with the usual Scenario_ExpectatedBehavior pattern. 

[Charlie Harrison, 2017/03/10 17:12:47]

Done. I also rewrote some of the tests to fit the pattern better. Note that the
fixtures need to be basically the same, so I made them both empty subclasses of
the main one.

+  CreateTestNavigationForMainFrame(GURL("http://example.test/activate.html"));
+  SimulateStartAndExpectToProceed();
+
+  NotifyPageActivation(ActivationState(ActivationLevel::ENABLED));

[engedy, 2017/03/10 10:56:57]

nit for follow-up CL: Ultimately we should have a version of this test where the
notification arrives before WillStartRequest. This is an edge case that might
happen if we have the Safe Browsing result cached. Currently it looks like this
might be hard to simulate.

[Charlie Harrison, 2017/03/10 17:12:47]

Would this be before WillStartRequest but after DidStartNavigation? Let's do
this in the ThrottleManager where we can have the mock page state activation
throttle notify us early.

[engedy, 2017/03/10 18:33:42]

Exactly, the overall navigation will be in the WillStartRequest state, but
WillStartRequest will not have been called on this throttle yet. The solution
you suggested sounds great.

+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_EQ(ActivationLevel::ENABLED, state.activation_level);
+  EXPECT_FALSE(state.filtering_disabled_for_document);
+}
+
+TEST_F(ActivationStateComputingNavigationThrottleTest,
+       DoNotActivateMainFrameForInactivePage) {
+  CreateTestNavigationForMainFrame(GURL("http://example.test/inactivate.html"));

[engedy, 2017/03/10 10:56:57]

nit: Let's remove the paths in all these URL, their presence suggests they play
a role, which is confusing.

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+  SimulateStartAndExpectToProceed();
+  SimulateRedirectAndExpectToProceed(
+      GURL("http://example.test/inactivate.html?v=1"));
+
+  // Never send NotifyPageActivation.
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_EQ(ActivationLevel::DISABLED, state.activation_level);
+}
+
+TEST_F(ActivationStateComputingNavigationThrottleTest,
+       DoNotActivateMainFrameForInactivePage2) {
+  CreateTestNavigationForMainFrame(GURL("http://example.test/inactivate.html"));
+  SimulateStartAndExpectToProceed();
+  SimulateRedirectAndExpectToProceed(
+      GURL("http://example.test/inactivate.html?v=1"));
+
+  // Notify that the page level state is explicitly disabled. Should be

[engedy, 2017/03/10 10:56:57]

nit: s/level state/(level) activation/

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+  // equivalent to not sending the message at all.
+  NotifyPageActivation(ActivationState(ActivationLevel::DISABLED));
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_EQ(ActivationLevel::DISABLED, state.activation_level);
+}
+
+TEST_F(ActivationStateComputingNavigationThrottleTest,

[engedy, 2017/03/10 10:56:57]

Let's also have a test where the main frame is actually whitelisted (no domain
filter option).

[engedy, 2017/03/10 10:56:57]

In addition to (or instead of) this test, let's have one that verifies that a
subframe won't be whitelisted if it is embedded into
"disallows-child-to-be-whitelisted.com".

[Charlie Harrison, 2017/03/10 17:12:46]

Done.

[Charlie Harrison, 2017/03/10 17:12:46]

Done.

+       ActivateMainFrameWhitelistedSubframe) {

[engedy, 2017/03/10 10:56:57]

nit:
s/ActivateMainFrameWhitelistedSubframe/MainFrame_ActivationBecauseWhitelistNotApplies/

[Charlie Harrison, 2017/03/10 17:12:46]

Done.

+  InitializeDocumentSubresourceFilter(GURL("http://parent.com/"));
+  // Not really a child frame, so whitelist should not apply.
+  CreateTestNavigationForMainFrame(GURL("http://whitelist.com/"));
+  SimulateStartAndExpectToProceed();
+
+  // Main frames will usually have their "parent" activation state set right
+  // before WillProcessResponse.
+  NotifyPageActivation(ActivationState(ActivationLevel::ENABLED));
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_FALSE(state.filtering_disabled_for_document);
+  EXPECT_EQ(ActivationLevel::ENABLED, state.activation_level);
+}
+
+// Subframe tests
+TEST_F(ActivationStateComputingNavigationThrottleTest, ActivateSubframe) {
+  InitializeDocumentSubresourceFilter(GURL("http://example.test"));
+  CreateSubframeAndInitTestNavigation(
+      GURL("http://example.test/activate.html"),
+      ActivationState(ActivationLevel::ENABLED));
+  SimulateStartAndExpectToProceed();
+  SimulateRedirectAndExpectToProceed(
+      GURL("http://example.test/activate.html?v=1"));
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_EQ(ActivationLevel::ENABLED, state.activation_level);
+  EXPECT_FALSE(state.filtering_disabled_for_document);
+}
+
+TEST_F(ActivationStateComputingNavigationThrottleTest,
+       DoNotActivateWhitelistedSubframe) {

[engedy, 2017/03/10 10:56:57]

We should also a version to verify that proto::ACTIVATION_TYPE_GENERICBLOCK
translates into ActivationState::generic_blocking_rules_disabled = true.

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+  InitializeDocumentSubresourceFilter(GURL("http://parent.com/"));
+  CreateSubframeAndInitTestNavigation(
+      GURL("http://whitelist.com/"), ActivationState(ActivationLevel::ENABLED));
+  SimulateStartAndExpectToProceed();
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_TRUE(state.filtering_disabled_for_document);
+  EXPECT_EQ(ActivationLevel::ENABLED, state.activation_level);
+}
+
+TEST_F(ActivationStateComputingNavigationThrottleTest,
+       EnsureDryRunIsPropagated) {

[engedy, 2017/03/10 10:56:57]

We should also have a version to verify that page activation with DRYRUN
translates into main frame activation with DRYRUN.

[Charlie Harrison, 2017/03/10 17:12:47]

This test now verifies that with the refactoring I did w/ page activation.

+  InitializeDocumentSubresourceFilter(GURL("http://example.test"));
+  CreateSubframeAndInitTestNavigation(GURL("http://example.test/activate.html"),
+                                      ActivationState(ActivationLevel::DRYRUN));
+  SimulateStartAndExpectToProceed();
+  SimulateRedirectAndExpectToProceed(
+      GURL("http://example.test/activate.html?v=1"));
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_EQ(ActivationLevel::DRYRUN, state.activation_level);

[engedy, 2017/03/10 10:56:57]

nit:

EXPECT_FALSE(state.filtering_disabled_for_document);
EXPECT_FALSE(state.generic_blocking_rules_disabled);

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+}
+
+TEST_F(ActivationStateComputingNavigationThrottleTest,
+       DisabledStatePropagatedDocument) {
+  InitializeDocumentSubresourceFilter(GURL("http://example.test"));
+  ActivationState parent_activation(ActivationLevel::ENABLED);
+  parent_activation.filtering_disabled_for_document = true;
+  CreateSubframeAndInitTestNavigation(GURL("http://example.test/activate.html"),
+                                      parent_activation);
+  SimulateStartAndExpectToProceed();
+  SimulateRedirectAndExpectToProceed(
+      GURL("http://example.test/activate.html?v=1"));
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_EQ(ActivationLevel::ENABLED, state.activation_level);
+  EXPECT_TRUE(state.filtering_disabled_for_document);
+  EXPECT_FALSE(state.generic_blocking_rules_disabled);
+}
+
+TEST_F(ActivationStateComputingNavigationThrottleTest,
+       DisabledStatePropagatedGeneric) {

[engedy, 2017/03/10 10:56:57]

We should have a test (a new one or extend an existing) to verify that:

(state.generic_blocking_rules_disabled = 1 for parent) + (completely whitelisted
child) = (state.filtering_disabled_for_document = 1 for child)

[Charlie Harrison, 2017/03/10 17:12:47]

Done.

+  InitializeDocumentSubresourceFilter(GURL("http://example.test"));
+  ActivationState parent_activation(ActivationLevel::ENABLED);
+  parent_activation.generic_blocking_rules_disabled = true;
+  CreateSubframeAndInitTestNavigation(GURL("http://example.test/activate.html"),
+                                      parent_activation);
+  SimulateStartAndExpectToProceed();
+  SimulateRedirectAndExpectToProceed(
+      GURL("http://example.test/activate.html?v=1"));
+  SimulateCommitAndExpectToProceed();
+
+  ActivationState state = last_activation_state();
+  EXPECT_EQ(ActivationLevel::ENABLED, state.activation_level);
+  EXPECT_FALSE(state.filtering_disabled_for_document);
+  EXPECT_TRUE(state.generic_blocking_rules_disabled);
+}
+
+}  // namespace subresource_filter