Content API changes to improve DOM stitching in ThreatDetails code.

components/safe_browsing/browser/threat_details.cc

 // Copyright (c) 2012 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.
 //
 // Implementation of the ThreatDetails class.
 
 #include "components/safe_browsing/browser/threat_details.h"
 
 #include <stddef.h>
 #include <stdint.h>
 
 #include "base/bind.h"
 #include "base/lazy_instance.h"
 #include "base/metrics/histogram_macros.h"
 #include "base/strings/string_util.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/safe_browsing/base_ui_manager.h"
 #include "components/safe_browsing/browser/threat_details_cache.h"
 #include "components/safe_browsing/browser/threat_details_history.h"
 #include "components/safe_browsing/common/safebrowsing_messages.h"
 #include "content/public/browser/browser_thread.h"
 #include "content/public/browser/navigation_controller.h"
 #include "content/public/browser/navigation_entry.h"
 #include "content/public/browser/render_frame_host.h"
+#include "content/public/browser/render_process_host.h"
 #include "content/public/browser/web_contents.h"
 #include "net/url_request/url_request_context_getter.h"
 
 using content::BrowserThread;
 using content::NavigationEntry;
 using content::RenderFrameHost;
 using content::WebContents;
 
 // Keep in sync with KMaxNodes in components/safe_browsing/renderer/
 // threat_dom_details.cc
@@ skipping 244 matching lines @@
       }
       if (!duplicate_child)
         url_resource->add_child_ids(child_resource->id());
     }
   }
 
   return url_resource;
 }
 
 void ThreatDetails::AddDomElement(
+    const int process_id,
     const int frame_tree_node_id,
-    const std::string& frame_url,
+    const int child_frame_routing_id,

[Charlie Reis, 2017/05/10 22:17:49]

Both process_id and child_frame_routing_id look unused now.  Can we remove them
as parameters?

[lpz, 2017/05/12 13:53:16]

Yep, and some cascading param cleanup from this in other methods too

     const int element_node_id,
     const std::string& tagname,
     const int parent_element_node_id,
     const std::vector<AttributeNameValue>& attributes,
     const ClientSafeBrowsingReportRequest::Resource* resource) {
   // Create the element. It should not exist already since this function should
   // only be called once for each element.
   const std::string element_key =
       GetElementKey(frame_tree_node_id, element_node_id);
   HTMLElement* cur_element = FindOrCreateElement(element_key);
 
   // Set some basic metadata about the element.
   const std::string tag_name_upper = base::ToUpperASCII(tagname);
   if (!tag_name_upper.empty()) {
     cur_element->set_tag(tag_name_upper);
   }
   for (const AttributeNameValue& attribute : attributes) {
     HTMLElement::Attribute* attribute_pb = cur_element->add_attribute();
     attribute_pb->set_name(attribute.first);
     attribute_pb->set_value(attribute.second);
   }
-  bool is_frame = tag_name_upper == "IFRAME" || tag_name_upper == "FRAME";
 
   if (resource) {
     cur_element->set_resource_id(resource->id());
-
-    // For iframes, remember that this HTML Element represents an iframe with a
-    // specific URL. Elements from a frame with this URL are children of this
-    // element.
-    if (is_frame &&
-        !base::ContainsKey(iframe_src_to_element_map_, resource->url())) {
-      iframe_src_to_element_map_[resource->url()] = cur_element;
-    }
   }
 
   // Next we try to lookup the parent of the current element and add ourselves
   // as a child of it.
   HTMLElement* parent_element = nullptr;
   if (parent_element_node_id == 0) {
     // No parent indicates that this element is at the top of the current frame.
-    // This frame could be a child of an iframe in another frame, or it could be
-    // at the root of the whole page. If we have a frame URL then we can try to
-    // map this element to its parent.
-    if (!frame_url.empty()) {
-      // First, remember that this element is at the top-level of a frame with
-      // our frame URL.
-      document_url_to_children_map_[frame_url].insert(cur_element->id());
-
-      // Now check if the frame URL matches the src URL of an iframe elsewhere.
-      // This means that we processed the parent iframe element earlier, so we
-      // can add ourselves as a child of that iframe.
-      // If no such iframe exists, it could be processed later, or this element
-      // is in the top-level frame and truly has no parent.
-      if (base::ContainsKey(iframe_src_to_element_map_, frame_url)) {
-        parent_element = iframe_src_to_element_map_[frame_url];
-      }
-    }
+    // Remember that this is a top-level element of the frame with the
+    // current |frame_tree_node_id|. If this element is inside an iframe, a
+    // second pass will insert this element as a child of its parent iframe.
+    frame_tree_id_to_children_map_[frame_tree_node_id].insert(
+        cur_element->id());
   } else {
     // We have a parent ID, so this element is just a child of something inside
     // of our current frame. We can easily lookup our parent.
     const std::string& parent_key =
         GetElementKey(frame_tree_node_id, parent_element_node_id);
     if (base::ContainsKey(elements_, parent_key)) {
       parent_element = elements_[parent_key].get();
     }
   }
 
   // If a parent element was found, add ourselves as a child, ensuring not to
   // duplicate child IDs.
   if (parent_element) {
     bool duplicate_child = false;
     for (const int child_id : parent_element->child_ids()) {
       if (child_id == cur_element->id()) {
         duplicate_child = true;
         break;
       }
     }
     if (!duplicate_child) {
       parent_element->add_child_ids(cur_element->id());
     }
   }
-
-  // Finally, we need to check if the current element is the parent of some
-  // other elements that came in from another frame earlier. This only happens
-  // if we are an iframe, and our src URL exists in
-  // document_url_to_children_map_. If there is a match, then all of the
-  // children in that map belong to us.
-  if (is_frame && resource &&
-      base::ContainsKey(document_url_to_children_map_, resource->url())) {
-    const std::unordered_set<int>& child_ids =
-        document_url_to_children_map_[resource->url()];
-    for (const int child_id : child_ids) {
-      cur_element->add_child_ids(child_id);
-    }
-  }
 }
 
 void ThreatDetails::StartCollection() {
   DVLOG(1) << "Starting to compute threat details.";
   report_.reset(new ClientSafeBrowsingReportRequest());
 
   if (IsReportableUrl(resource_.url)) {
     report_->set_url(resource_.url.spec());
     report_->set_type(GetReportTypeFromSBThreatType(resource_.threat_type));
   }
@@ skipping 49 matching lines @@
     // detail collection could be started once the page loads.
     web_contents()->SendToAllFrames(
         new SafeBrowsingMsg_GetThreatDOMDetails(MSG_ROUTING_NONE));
   }
 }
 
 // When the renderer is done, this is called.
 void ThreatDetails::OnReceivedThreatDOMDetails(
     content::RenderFrameHost* sender,
     const std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node>& params) {
+  // Lookup the FrameTreeNodeId of any child frames in the list of DOM nodes.

[Charlie Reis, 2017/05/10 22:17:49]

nit: FrameTreeNode ID

[lpz, 2017/05/12 13:53:16]

Done.

+  const int sender_process_id = sender->GetProcess()->GetID();
+  const int sender_frame_tree_node_id = sender->GetFrameTreeNodeId();
+  KeyToFrameTreeIdMap child_frame_tree_map;
+  for (const SafeBrowsingHostMsg_ThreatDOMDetails_Node& node : params) {
+    if (node.child_frame_routing_id == 0)
+      continue;
+
+    const std::string cur_element_key =
+        GetElementKey(sender_frame_tree_node_id, node.node_id);
+    RenderFrameHost* rfh =
+        content::RenderFrameHost::GetRenderFrameHostForRoutingId(
+            sender_process_id, node.child_frame_routing_id);
+    if (!rfh) {
+      ambiguous_dom_ = true;
+    } else {
+      child_frame_tree_map[cur_element_key] = rfh->GetFrameTreeNodeId();
+    }
+  }
+
   // Schedule this in IO thread, so it doesn't conflict with future users
   // of our data structures (eg GetSerializedReport).
   BrowserThread::PostTask(
       BrowserThread::IO, FROM_HERE,
-      base::BindOnce(&ThreatDetails::AddDOMDetails, this,
-                     sender->GetFrameTreeNodeId(),
-                     sender->GetLastCommittedURL(), params));
+      base::Bind(&ThreatDetails::AddDOMDetails, this,
+                 sender->GetProcess()->GetID(), sender->GetFrameTreeNodeId(),
+                 sender->GetLastCommittedURL(), params, child_frame_tree_map));
 }
 
 void ThreatDetails::AddDOMDetails(
+    const int process_id,
     const int frame_tree_node_id,
     const GURL& frame_last_committed_url,
-    const std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node>& params) {
+    const std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node>& params,
+    const KeyToFrameTreeIdMap& child_frame_tree_map) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
   DVLOG(1) << "Nodes from the DOM: " << params.size();
 
   // If we have already started getting redirects from history service,
   // don't modify state, otherwise will invalidate the iterators.
   if (redirects_collector_->HasStarted())
     return;
 
   // If we have already started collecting data from the HTTP cache, don't
   // modify our state.
   if (cache_collector_->HasStarted())
     return;
 
   // Exit early if there are no nodes to process.
   if (params.empty())
     return;
 
-  // Try to deduce the URL that the render frame was handling. First check if
-  // the summary node from the renderer has a document URL. If not, try looking
-  // at the last committed URL of the frame.
-  GURL frame_url;
-  if (IsReportableUrl(params.back().url)) {
-    frame_url = params.back().url;
-  } else if (IsReportableUrl(frame_last_committed_url)) {
-    frame_url = frame_last_committed_url;
-  }
-
-  // If we can't figure out which URL the frame was rendering then we don't know
-  // where these elements belong in the hierarchy. The DOM will be ambiguous.
-  if (frame_url.is_empty()) {
-    ambiguous_dom_ = true;
-  }
+  // Copy FrameTreeNode IDs for the child frame into the combined mapping.
+  iframe_key_to_frame_tree_id_map_.insert(child_frame_tree_map.begin(),
+                                          child_frame_tree_map.end());
 
   // Add the urls from the DOM to |resources_|. The renderer could be sending
   // bogus messages, so limit the number of nodes we accept.
   // Also update |elements_| with the DOM structure.
   for (size_t i = 0; i < params.size() && i < kMaxDomNodes; ++i) {
     SafeBrowsingHostMsg_ThreatDOMDetails_Node node = params[i];
     DVLOG(1) << node.url << ", " << node.tag_name << ", " << node.parent;
     ClientSafeBrowsingReportRequest::Resource* resource = nullptr;
     if (!node.url.is_empty()) {
       resource = AddUrl(node.url, node.parent, node.tag_name, &(node.children));
     }
     // Check for a tag_name to avoid adding the summary node to the DOM.
     if (!node.tag_name.empty()) {
-      AddDomElement(frame_tree_node_id, frame_url.spec(), node.node_id,
-                    node.tag_name, node.parent_node_id, node.attributes,
-                    resource);
+      AddDomElement(process_id, frame_tree_node_id, node.child_frame_routing_id,
+                    node.node_id, node.tag_name, node.parent_node_id,
+                    node.attributes, resource);
     }
   }
 }
 
 // Called from the SB Service on the IO thread, after the user has
 // closed the tab, or clicked proceed or goback.  Since the user needs
 // to take an action, we expect this to be called after
 // OnReceivedThreatDOMDetails in most cases. If not, we don't include
 // the DOM data in our report.
 void ThreatDetails::FinishCollection(bool did_proceed, int num_visit) {
   DCHECK_CURRENTLY_ON(BrowserThread::IO);
 
+  // Do a second pass over the elements and update iframe elements to have

[Charlie Reis, 2017/05/10 22:17:49]

Side note: I don't fully understand this second pass, so hopefully there's good
test coverage for it.

[lpz, 2017/05/12 13:53:16]

Yes, this is tested by ThreatDetailsTest.ThreatDOMDetails_MultipleFrames. It's
meant to cover the case where data from renderers is received in different
orders. It waits until we have data from all renderers before stitching them
together.

[Charlie Reis, 2017/05/12 21:40:50]

Acknowledged.

+  // references to their children. Children may have been received from a
+  // different renderer than the iframe element.
+  for (auto& element_pair : elements_) {
+    const std::string& element_key = element_pair.first;
+    HTMLElement* element = element_pair.second.get();
+    if (base::ContainsKey(iframe_key_to_frame_tree_id_map_, element_key)) {
+      int frame_tree_id_of_iframe_renderer =
+          iframe_key_to_frame_tree_id_map_[element_key];
+      const std::unordered_set<int>& child_ids =
+          frame_tree_id_to_children_map_[frame_tree_id_of_iframe_renderer];
+      for (const int child_id : child_ids) {
+        element->add_child_ids(child_id);
+      }
+    }
+  }
   did_proceed_ = did_proceed;
   num_visits_ = num_visit;
   std::vector<GURL> urls;
   for (ResourceMap::const_iterator it = resources_.begin();
        it != resources_.end(); ++it) {
     urls.push_back(GURL(it->first));
   }
   redirects_collector_->StartHistoryCollection(
       urls, base::Bind(&ThreatDetails::OnRedirectionCollectionReady, this));
 }
@@ skipping 55 matching lines @@
   // Send the report, using the SafeBrowsingService.
   std::string serialized;
   if (!report_->SerializeToString(&serialized)) {
     DLOG(ERROR) << "Unable to serialize the threat report.";
     return;
   }
   ui_manager_->SendSerializedThreatDetails(serialized);
 }
 
 }  // namespace safe_browsing