DNR: De-flake and re-enable some disabled tests.

This CL attempts to remove some possible sources of flakiness in the
declarative net request browsertests by removing all calls to
content::RunAllTasksUntilIdle() and replacing them with more
deterministic waiting logic. Also re-enable the tests disabled in
r758084.

Also, enforce that RulesetManager only has a single test observer at a
time.

BUG=1069665

Change-Id: I29c646d85322195894d2786fbf01fb7c10842cb1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2146335
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#759468}

chrome/browser/extensions/api/declarative_net_request/rule_indexing_unittest.cc

@@ -925,9 +925,9 @@ TEST_P(MultipleRulesetsIndexingTest, EnabledRulesCount) {
   AddRuleset(CreateRuleset("2.json", 200, 20, false));
   AddRuleset(CreateRuleset("3.json", 300, 30, true));
 
-  RulesetCountWaiter waiter(manager());
+  RulesetManagerObserver ruleset_waiter(manager());
   LoadAndExpectSuccess();
-  waiter.WaitForRulesetCount(1);
+  ruleset_waiter.WaitForExtensionsWithRulesetsCount(1);
 
   // Only the first and third rulesets should be enabled.
   CompositeMatcher* composite_matcher =
@@ -957,7 +957,7 @@ TEST_P(MultipleRulesetsIndexingTest, StaticRuleCountExceeded) {
   // Enabled on load.
   AddRuleset(CreateRuleset("4.json", 30, 0, true));
 
-  RulesetCountWaiter waiter(manager());
+  RulesetManagerObserver ruleset_waiter(manager());
   extension_loader()->set_ignore_manifest_warnings(true);
 
   LoadAndExpectSuccess();
@@ -978,7 +978,7 @@ TEST_P(MultipleRulesetsIndexingTest, StaticRuleCountExceeded) {
             Field(&InstallWarning::message, kEnabledRuleCountExceeded)));
   }
 
-  waiter.WaitForRulesetCount(1);
+  ruleset_waiter.WaitForExtensionsWithRulesetsCount(1);
 
   CompositeMatcher* composite_matcher =
       manager()->GetMatcherForExtension(extension_id);
@@ -1006,7 +1006,7 @@ TEST_P(MultipleRulesetsIndexingTest, RegexRuleCountExceeded) {
   // Enabled on load.
   AddRuleset(CreateRuleset("4.json", 20, 20, true));
 
-  RulesetCountWaiter waiter(manager());
+  RulesetManagerObserver ruleset_waiter(manager());
   extension_loader()->set_ignore_manifest_warnings(true);
 
   LoadAndExpectSuccess();
@@ -1019,7 +1019,7 @@ TEST_P(MultipleRulesetsIndexingTest, RegexRuleCountExceeded) {
                                            kEnabledRegexRuleCountExceeded)));
   }
 
-  waiter.WaitForRulesetCount(1);
+  ruleset_waiter.WaitForExtensionsWithRulesetsCount(1);
 
   CompositeMatcher* composite_matcher =
       manager()->GetMatcherForExtension(extension()->id());

extensions/browser/api/declarative_net_request/ruleset_manager.cc

@@ -218,6 +218,8 @@ void RulesetManager::OnDidFinishNavigation(content::RenderFrameHost* host) {
 
 void RulesetManager::SetObserverForTest(TestObserver* observer) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  DCHECK(!(test_observer_ && observer))
+      << "Multiple test observers are not supported";
   test_observer_ = observer;
 }
 

extensions/browser/api/declarative_net_request/test_utils.cc

@@ -15,6 +15,7 @@
 #include "extensions/browser/api/declarative_net_request/indexed_rule.h"
 #include "extensions/browser/api/declarative_net_request/ruleset_matcher.h"
 #include "extensions/browser/api/declarative_net_request/ruleset_source.h"
+#include "extensions/browser/api/web_request/web_request_info.h"
 #include "extensions/browser/extension_prefs.h"
 #include "extensions/common/api/declarative_net_request/test_utils.h"
 #include "extensions/common/extension.h"
@@ -336,16 +337,23 @@ bool EqualsForTesting(const dnr_api::ModifyHeaderInfo& lhs,
   return lhs.operation == rhs.operation && lhs.header == rhs.header;
 }
 
-RulesetCountWaiter::RulesetCountWaiter(RulesetManager* manager)
+RulesetManagerObserver::RulesetManagerObserver(RulesetManager* manager)
     : manager_(manager), current_count_(manager_->GetMatcherCountForTest()) {
   manager_->SetObserverForTest(this);
 }
 
-RulesetCountWaiter::~RulesetCountWaiter() {
+RulesetManagerObserver::~RulesetManagerObserver() {
   manager_->SetObserverForTest(nullptr);
 }
 
-void RulesetCountWaiter::WaitForRulesetCount(size_t count) {
+std::vector<GURL> RulesetManagerObserver::GetAndResetRequestSeen() {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  std::vector<GURL> seen_requests;
+  std::swap(seen_requests, observed_requests_);
+  return seen_requests;
+}
+
+void RulesetManagerObserver::WaitForExtensionsWithRulesetsCount(size_t count) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   ASSERT_FALSE(expected_count_);
   if (current_count_ == count)
@@ -356,7 +364,7 @@ void RulesetCountWaiter::WaitForRulesetCount(size_t count) {
   run_loop_->Run();
 }
 
-void RulesetCountWaiter::OnRulesetCountChanged(size_t count) {
+void RulesetManagerObserver::OnRulesetCountChanged(size_t count) {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
   current_count_ = count;
   if (expected_count_ != count)
@@ -368,5 +376,11 @@ void RulesetCountWaiter::OnRulesetCountChanged(size_t count) {
   expected_count_.reset();
 }
 
+void RulesetManagerObserver::OnEvaluateRequest(const WebRequestInfo& request,
+                                               bool is_incognito_context) {
+  DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
+  observed_requests_.push_back(request.url);
+}
+
 }  // namespace declarative_net_request
 }  // namespace extensions

extensions/browser/api/declarative_net_request/test_utils.h

@@ -86,30 +86,36 @@ bool EqualsForTesting(
     const api::declarative_net_request::ModifyHeaderInfo& lhs,
     const api::declarative_net_request::ModifyHeaderInfo& rhs);
 
-// Used to wait till the number of extension rulesets (CompositeMatchers)
-// managed by the RulesetManager reach a certain count. This is a multi-use
-// observer i.e. WaitForRulesetCount can be called multiple times per lifetime
+// Test observer for RulesetManager. This is a multi-use observer i.e.
+// WaitForExtensionsWithRulesetsCount can be called multiple times per lifetime
 // of an observer.
-class RulesetCountWaiter : public RulesetManager::TestObserver {
+class RulesetManagerObserver : public RulesetManager::TestObserver {
  public:
-  explicit RulesetCountWaiter(RulesetManager* manager);
-  RulesetCountWaiter(const RulesetCountWaiter&) = delete;
-  RulesetCountWaiter& operator=(const RulesetCountWaiter&) = delete;
-  ~RulesetCountWaiter() override;
+  explicit RulesetManagerObserver(RulesetManager* manager);
+  RulesetManagerObserver(const RulesetManagerObserver&) = delete;
+  RulesetManagerObserver& operator=(const RulesetManagerObserver&) = delete;
+  ~RulesetManagerObserver() override;
+
+  // Returns the requests seen by RulesetManager since the last call to this
+  // function.
+  std::vector<GURL> GetAndResetRequestSeen();
 
   // Waits for the number of rulesets to change to |count|. Note |count| is the
   // number of extensions with rulesets or the number of active
   // CompositeMatchers.
-  void WaitForRulesetCount(size_t count);
+  void WaitForExtensionsWithRulesetsCount(size_t count);
 
  private:
   // RulesetManager::TestObserver implementation.
   void OnRulesetCountChanged(size_t count) override;
+  void OnEvaluateRequest(const WebRequestInfo& request,
+                         bool is_incognito_context) override;
 
   RulesetManager* const manager_;
   size_t current_count_ = 0;
   base::Optional<size_t> expected_count_;
   std::unique_ptr<base::RunLoop> run_loop_;
+  std::vector<GURL> observed_requests_;
   SEQUENCE_CHECKER(sequence_checker_);
 };