[DNR] Remove the NONE action type

This CL removes the NONE action type. Instead of returning a vector of
one action with the NONE type, RulesetManager::EvaluateRequest will
instead return an empty vector if no DNR rules match with the request.

Bug: 1004006
Change-Id: I7dc2b7b59540f3d185b9585e530818f92e6201b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1809882Reviewed-by: Karan Bhatia <karandeepb@chromium.org>
Commit-Queue: Kelvin Jiang <kelvinjiang@chromium.org>
Cr-Commit-Position: refs/heads/master@{#698651}

chrome/browser/extensions/api/web_request/web_request_api_unittest.cc

@@ -771,7 +771,7 @@ TEST(ExtensionWebRequestHelpersTest, TestMergeOnBeforeSendHeadersResponses) {
   headers0.MergeFrom(base_headers);
   WebRequestInfoInitParams info_params;
   WebRequestInfo info(std::move(info_params));
-  info.dnr_actions.emplace_back(Action::Type::NONE);
+  info.dnr_actions = std::vector<Action>();
   MergeOnBeforeSendHeadersResponses(info, deltas, &headers0, &ignored_actions,
                                     &ignore1, &ignore2,
                                     &request_headers_modified0);
@@ -882,7 +882,8 @@ TEST(ExtensionWebRequestHelpersTest, TestMergeOnBeforeSendHeadersResponses) {
 
   Action remove_headers_action(Action::Type::REMOVE_HEADERS);
   remove_headers_action.request_headers_to_remove = {"key5"};
-  info.dnr_actions.push_back(std::move(remove_headers_action));
+  info.dnr_actions = std::vector<Action>();
+  info.dnr_actions->push_back(std::move(remove_headers_action));
   MergeOnBeforeSendHeadersResponses(info, deltas, &headers4, &ignored_actions,
                                     &ignore1, &ignore2,
                                     &request_headers_modified4);
@@ -929,7 +930,7 @@ TEST(ExtensionWebRequestHelpersTest,
 
   WebRequestInfoInitParams info_params;
   WebRequestInfo info(std::move(info_params));
-  info.dnr_actions.emplace_back(Action::Type::NONE);
+  info.dnr_actions = std::vector<Action>();
   helpers::IgnoredActions ignored_actions;
   std::set<std::string> removed_headers, set_headers;
   bool request_headers_modified = false;
@@ -1272,7 +1273,7 @@ TEST(ExtensionWebRequestHelpersTest, TestMergeOnHeadersReceivedResponses) {
   WebRequestInfoInitParams info_params;
   info_params.url = GURL(kExampleUrl);
   WebRequestInfo info(std::move(info_params));
-  info.dnr_actions.emplace_back(Action::Type::NONE);
+  info.dnr_actions = std::vector<Action>();
 
   MergeOnHeadersReceivedResponses(info, deltas, base_headers.get(),
                                   &new_headers0, &allowed_unsafe_redirect_url0,
@@ -1354,7 +1355,8 @@ TEST(ExtensionWebRequestHelpersTest, TestMergeOnHeadersReceivedResponses) {
   // request extensions and result in a conflict.
   Action remove_headers_action(Action::Type::REMOVE_HEADERS);
   remove_headers_action.response_headers_to_remove = {"key3"};
-  info.dnr_actions.push_back(std::move(remove_headers_action));
+  info.dnr_actions = std::vector<Action>();
+  info.dnr_actions->push_back(std::move(remove_headers_action));
 
   ignored_actions.clear();
   bool response_headers_modified3 = false;
@@ -1410,7 +1412,7 @@ TEST(ExtensionWebRequestHelpersTest,
   WebRequestInfoInitParams info_params;
   info_params.url = GURL(kExampleUrl);
   WebRequestInfo info(std::move(info_params));
-  info.dnr_actions.emplace_back(Action::Type::NONE);
+  info.dnr_actions = std::vector<Action>();
 
   MergeOnHeadersReceivedResponses(info, deltas, base_headers.get(),
                                   &new_headers1, &allowed_unsafe_redirect_url1,

extensions/browser/api/declarative_net_request/ruleset_manager.cc

@@ -337,12 +337,12 @@ const std::vector<RulesetManager::Action>& RulesetManager::EvaluateRequest(
   // |is_incognito_context| will stay the same for a given |request|. This also
   // assumes that the core state of the WebRequestInfo isn't changed between the
   // different EvaluateRequest invocations.
-  if (request.dnr_actions.empty()) {
+  if (!request.dnr_actions) {
     request.dnr_actions =
         EvaluateRequestInternal(request, is_incognito_context);
   }
 
-  return request.dnr_actions;
+  return *request.dnr_actions;
 }
 
 bool RulesetManager::HasAnyExtraHeadersMatcher() const {
@@ -496,22 +496,18 @@ std::vector<RulesetManager::Action> RulesetManager::EvaluateRequestInternal(
     const WebRequestInfo& request,
     bool is_incognito_context) const {
   DCHECK_CALLED_ON_VALID_SEQUENCE(sequence_checker_);
-  DCHECK(request.dnr_actions.empty());
+  DCHECK(!request.dnr_actions);
 
   std::vector<Action> actions;
 
-  if (!ShouldEvaluateRequest(request)) {
-    actions.emplace_back(Action::Type::NONE);
+  if (!ShouldEvaluateRequest(request))
     return actions;
-  }
 
   if (test_observer_)
     test_observer_->OnEvaluateRequest(request, is_incognito_context);
 
-  if (rulesets_.empty()) {
-    actions.emplace_back(Action::Type::NONE);
+  if (rulesets_.empty())
     return actions;
-  }
 
   SCOPED_UMA_HISTOGRAM_TIMER(
       "Extensions.DeclarativeNetRequest.EvaluateRequestTime.AllExtensions2");
@@ -574,7 +570,6 @@ std::vector<RulesetManager::Action> RulesetManager::EvaluateRequestInternal(
   if (!remove_headers_actions.empty())
     return remove_headers_actions;
 
-  actions.emplace_back(Action::Type::NONE);
   return actions;
 }
 

extensions/browser/api/declarative_net_request/ruleset_manager.h

@@ -41,7 +41,6 @@ class RulesetManager {
  public:
   struct Action {
     enum class Type {
-      NONE,
       // Block the network request.
       BLOCK,
       // Block the network request and collapse the corresponding DOM element.
@@ -57,14 +56,13 @@ class RulesetManager {
     Action(Action&&);
     Action& operator=(Action&&);
 
-    Type type = Type::NONE;
+    Type type = Type::BLOCK;
 
     // Valid iff |type| is |REDIRECT|.
     base::Optional<GURL> redirect_url;
 
-    // The id of the extension the action is attributed to. Valid iff |type| is
-    // not |NONE|.
-    base::Optional<ExtensionId> extension_id;
+    // The id of the extension the action is attributed to.
+    ExtensionId extension_id;
 
     // Valid iff |type| is |REMOVE_HEADERS|. The vectors point to strings of
     // static storage duration.

extensions/browser/api/web_request/web_request_api.cc

@@ -514,8 +514,7 @@ void OnDNRActionMatched(
           ->ruleset_manager()
           ->action_tracker();
 
-  DCHECK(action.extension_id.has_value());
-  action_tracker.OnRuleMatched(action.extension_id.value(), tab_id);
+  action_tracker.OnRuleMatched(action.extension_id, tab_id);
 }
 
 // Helper to remove request headers based on a matched DNR action. Returns
@@ -1044,7 +1043,7 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest(
   DCHECK(should_collapse_initiator);
 
   if (ShouldHideEvent(browser_context, *request)) {
-    request->dnr_actions.emplace_back(Action::Type::NONE);
+    request->dnr_actions = std::vector<Action>();
     return net::OK;
   }
 
@@ -1101,9 +1100,6 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest(
           ->EvaluateRequest(*request, is_incognito_context);
   for (const auto& action : actions) {
     switch (action.type) {
-      case Action::Type::NONE:
-        DCHECK_EQ(1u, actions.size());
-        break;
       case Action::Type::BLOCK:
         ClearPendingCallbacks(*request);
         DCHECK_EQ(1u, actions.size());
@@ -1126,8 +1122,8 @@ int ExtensionWebRequestEventRouter::OnBeforeRequest(
         // Unlike other actions, allow web request extensions to intercept the
         // request here. The headers will be removed during subsequent request
         // stages.
-        DCHECK(std::all_of(request->dnr_actions.begin(),
-                           request->dnr_actions.end(), [](const auto& action) {
+        DCHECK(std::all_of(request->dnr_actions->begin(),
+                           request->dnr_actions->end(), [](const auto& action) {
                              return action.type == Action::Type::REMOVE_HEADERS;
                            }));
         break;
@@ -1163,10 +1159,10 @@ int ExtensionWebRequestEventRouter::OnBeforeSendHeaders(
   // Remove request headers for the Declarative Net Request API. It is given
   // preference over the Web Request API and this also hides the removed headers
   // from extensions using the Web Request API.
-  DCHECK(!request->dnr_actions.empty());
+  DCHECK(request->dnr_actions);
   std::set<std::string> removed_headers;
 
-  for (const auto& action : request->dnr_actions) {
+  for (const auto& action : *request->dnr_actions) {
     bool headers_removed_for_action =
         RemoveRequestHeadersForAction(headers, action, &removed_headers);
 
@@ -1258,9 +1254,10 @@ int ExtensionWebRequestEventRouter::OnHeadersReceived(
   // Handle header removal by the Declarative Net Request API. We filter these
   // headers so that headers removed by Declarative Net Request API are not
   // visible to web request extensions.
-  DCHECK(!request->dnr_actions.empty());
+  DCHECK(request->dnr_actions);
   bool should_remove_headers =
-      request->dnr_actions[0].type == Action::Type::REMOVE_HEADERS &&
+      !request->dnr_actions->empty() &&
+      (*request->dnr_actions)[0].type == Action::Type::REMOVE_HEADERS &&
       original_response_headers;
 
   scoped_refptr<const net::HttpResponseHeaders> filtered_response_headers;
@@ -1274,7 +1271,7 @@ int ExtensionWebRequestEventRouter::OnHeadersReceived(
             original_response_headers->raw_headers());
 
     bool headers_filtered = false;
-    for (const auto& action : request->dnr_actions) {
+    for (const auto& action : *request->dnr_actions) {
       bool headers_filtered_for_action = FilterResponseHeaders(
           mutable_response_headers.get(), action.response_headers_to_remove);
 

extensions/browser/api/web_request/web_request_api_helpers.cc

@@ -328,7 +328,7 @@ static_assert(ValidateHeaderEntries(kResponseHeaderEntries),
 bool HasMatchingRemovedDNRRequestHeader(
     const extensions::WebRequestInfo& request,
     const std::string& header) {
-  for (const auto& action : request.dnr_actions) {
+  for (const auto& action : *request.dnr_actions) {
     if (std::find_if(action.request_headers_to_remove.begin(),
                      action.request_headers_to_remove.end(),
                      [&header](const char* header_to_remove) {
@@ -345,7 +345,7 @@ bool HasMatchingRemovedDNRRequestHeader(
 bool HasMatchingRemovedDNRResponseHeader(
     const extensions::WebRequestInfo& request,
     const std::string& header) {
-  for (const auto& action : request.dnr_actions) {
+  for (const auto& action : *request.dnr_actions) {
     if (std::find_if(action.response_headers_to_remove.begin(),
                      action.response_headers_to_remove.end(),
                      [&header](const char* header_to_remove) {
@@ -968,7 +968,7 @@ void MergeOnBeforeSendHeadersResponses(
 
         // Prevent extensions from adding any header removed by the Declarative
         // Net Request API.
-        DCHECK(!request.dnr_actions.empty());
+        DCHECK(request.dnr_actions);
         if (HasMatchingRemovedDNRRequestHeader(request, key)) {
           extension_conflicts = true;
           break;
@@ -1340,8 +1340,7 @@ void MergeOnHeadersReceivedResponses(
 
     // Prevent extensions from adding any response header which was removed by
     // the Declarative Net Request API.
-    DCHECK(!request.dnr_actions.empty());
-
+    DCHECK(request.dnr_actions);
     if (!extension_conflicts) {
       for (const ResponseHeader& header : delta.added_response_headers) {
         if (HasMatchingRemovedDNRResponseHeader(request, header.first)) {

extensions/browser/api/web_request/web_request_info.h

@@ -166,11 +166,12 @@ struct WebRequestInfo {
   const int web_view_rules_registry_id;
   const int web_view_embedder_process_id;
 
-  // The Declarative Net Request action associated with this request. Mutable
+  // The Declarative Net Request actions associated with this request. Mutable
   // since this is lazily computed. Cached to avoid redundant computations.
-  // Valid when non-empty. In case no action is taken, populated with
-  // Action::Type::NONE.
-  mutable std::vector<declarative_net_request::RulesetManager::Action>
+  // Valid when not null. In case no actions are taken, populated with an empty
+  // vector.
+  mutable base::Optional<
+      std::vector<declarative_net_request::RulesetManager::Action>>
       dnr_actions;
 
   const bool is_service_worker_script;