Synchronously construct filters for subframes inheriting activation

Instead of walking up the frame tree for subframes that inherit
activation from their parents, we now synchronously construct a filter.
This uses the functionality introduced for inheriting activation from
same-origin openers, allowing synchronous construction. This should
simplify the logic used, ensuring any activated frame has an associated
filter.

Bug: 1134288
Change-Id: Iaa624145c2f77fcce3f0e40f7248fa713ff889c1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2561265
Commit-Queue: Alex Turner <alexmt@chromium.org>
Reviewed-by: Charlie Harrison <csharrison@chromium.org>
Cr-Commit-Position: refs/heads/master@{#833352}

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.cc

@@ -323,6 +323,7 @@ ContentSubresourceFilterThrottleManager::FilterForFinishedNavigation(
   DCHECK(frame_host);
 
   std::unique_ptr<AsyncDocumentSubresourceFilter> filter;
+  base::Optional<mojom::ActivationState> activation_to_inherit;
   did_inherit_opener_activation = false;
 
   if (navigation_handle->HasCommitted() && throttle) {
@@ -331,47 +332,43 @@ ContentSubresourceFilterThrottleManager::FilterForFinishedNavigation(
   }
 
   // If the frame should inherit its activation then, if it has an activated
-  // opener, construct a filter with the inherited activation state. The
+  // opener/parent, construct a filter with the inherited activation state. The
   // filter's activation state will be available immediately so a throttle is
   // not required. Instead, we construct the filter synchronously.
   if (ShouldInheritOpenerActivation(navigation_handle, frame_host)) {
     content::RenderFrameHost* opener_rfh =
         navigation_handle->GetWebContents()->GetOpener();
-    base::Optional<mojom::ActivationState> opener_activation;
     if (auto* opener_throttle_manager =
             ContentSubresourceFilterThrottleManager::FromWebContents(
                 content::WebContents::FromRenderFrameHost(opener_rfh))) {
-      opener_activation =
+      activation_to_inherit =
           opener_throttle_manager->GetFrameActivationState(opener_rfh);
       did_inherit_opener_activation = true;
     }
+  } else if (ShouldInheritParentActivation(navigation_handle)) {
+    // Throttles are only constructed for navigations handled by the network
+    // stack and we only release filters for committed navigations.
+    DCHECK(!filter);
+    activation_to_inherit =
+        GetFrameActivationState(navigation_handle->GetParentFrame());
+  }
 
-    if (opener_activation && opener_activation->activation_level !=
-                                 mojom::ActivationLevel::kDisabled) {
-      DCHECK(dealer_handle_);
+  if (activation_to_inherit.has_value() &&
+      activation_to_inherit->activation_level !=
+          mojom::ActivationLevel::kDisabled) {
+    DCHECK(dealer_handle_);
 
-      // This constructs the filter in a way that allows it to be immediately
-      // used. See the AsyncDocumentSubresourceFilter constructor for details.
-      filter = std::make_unique<AsyncDocumentSubresourceFilter>(
-          EnsureRulesetHandle(), frame_host->GetLastCommittedOrigin(),
-          *opener_activation);
-    }
+    // This constructs the filter in a way that allows it to be immediately
+    // used. See the AsyncDocumentSubresourceFilter constructor for details.
+    filter = std::make_unique<AsyncDocumentSubresourceFilter>(
+        EnsureRulesetHandle(), frame_host->GetLastCommittedOrigin(),
+        activation_to_inherit.value());
   }
 
-  // Make sure |frame_host_filter_map_| is updated or cleaned up depending on
-  // this navigation's activation state.
+  // Make sure `frame_host_filter_map_` is cleaned up if necessary. Otherwise,
+  // it is updated below.
   if (!filter) {
-    if (ShouldInheritParentActivation(navigation_handle) &&
-        base::Contains(frame_host_filter_map_,
-                       navigation_handle->GetParentFrame())) {
-      // TODO(crbug.com/1134288): Synchronously construct filters for subframes
-      // to inherit activation from their parents, instead of walking up the
-      // frame tree. Once done, consider updating the map in the caller.
-      // |nullptr| indicates a subframe inheriting its activation.
-      frame_host_filter_map_[frame_host] = nullptr;
-    } else {
-      frame_host_filter_map_.erase(frame_host);
-    }
+    frame_host_filter_map_.erase(frame_host);
     return nullptr;
   }
 
@@ -592,24 +589,12 @@ ContentSubresourceFilterThrottleManager::GetFrameFilter(
     content::RenderFrameHost* frame_host) {
   DCHECK(frame_host);
 
-  // Filter will be null for those special url navigations that were added in
-  // MaybeActivateSubframeSpecialUrls and for subframes with an aborted load.
-  // Return the filter of the first parent with a non-null filter.
-  while (frame_host) {
-    auto it = frame_host_filter_map_.find(frame_host);
-    if (it == frame_host_filter_map_.end())
-      return nullptr;
-
-    if (it->second)
-      return it->second.get();
-    frame_host = it->first->GetParent();
-  }
+  auto it = frame_host_filter_map_.find(frame_host);
+  if (it == frame_host_filter_map_.end())
+    return nullptr;
 
-  // Since a null filter is only possible for special navigations of iframes and
-  // aborted loads in a subframe, the above loop should have found a filter for
-  // at least the top level frame, thus making this unreachable.
-  NOTREACHED();
-  return nullptr;
+  DCHECK(it->second);
+  return it->second.get();
 }
 
 void ContentSubresourceFilterThrottleManager::MaybeShowNotification() {

components/subresource_filter/content/browser/content_subresource_filter_throttle_manager.h

@@ -235,12 +235,9 @@ class ContentSubresourceFilterThrottleManager
       const mojom::ActivationLevel& activation_level,
       bool did_inherit_opener_activation);
 
-  // For each RenderFrameHost where the last committed load has subresource
-  // filtering activated, owns the corresponding AsyncDocumentSubresourceFilter.
-  // A null filter indicates that the filter should be inherited from its
-  // parent if the parent has one. This is possible if the last load was a
-  // special navigation (see MaybeActivateSubframeSpecialUrls) or if no
-  // navigations have committed.
+  // For each RenderFrameHost where the last committed load (or the initial load
+  // if no committed load has occurred) has subresource filtering activated,
+  // owns the corresponding AsyncDocumentSubresourceFilter.
   std::map<content::RenderFrameHost*,
            std::unique_ptr<AsyncDocumentSubresourceFilter>>
       frame_host_filter_map_;