Content API changes to improve DOM stitching in ThreatDetails code.

chrome/browser/safe_browsing/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 "chrome/browser/safe_browsing/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 "chrome/browser/safe_browsing/threat_details_cache.h"
 #include "chrome/browser/safe_browsing/threat_details_history.h"
 #include "components/history/core/browser/history_service.h"
 #include "components/safe_browsing/base_ui_manager.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 other_frame_routing_id,
     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;
+    // For iframes, lookup the frame tree node id of the render frame that

[Charlie Reis, 2017/05/05 21:03:06]

nit: FrameTreeNode ID of the frame that

[lpz, 2017/05/10 14:21:08]

Done.

+    // handled that iframe's content. This must be done on the UI thread, and
+    // will update a map of |element_key| to |frame_tree_node_id|. A second pass
+    // is done to update the |elements_| list using this mapping.
+    if (is_frame) {
+      BrowserThread::PostTask(
+          BrowserThread::UI, FROM_HERE,
+          base::Bind(&ThreatDetails::LookupOtherFrameId, this, element_key,
+                     process_id, other_frame_routing_id));

[Charlie Reis, 2017/05/05 21:03:06]

There's a lot of posting back and forth here.  Is it ok that the FrameTreeNode
may be gone by the time you get back to the UI thread?  What if the
FrameTreeNode ID is still there, but the process and routing ID are no longer
associated with it (since a cross process navigation has completed and they've
been cleaned up)?

[Edit: See my suggestion below about avoiding the need this thread hop.]

[lpz, 2017/05/10 14:21:08]

Applied suggestion

     }
   }
 
   // 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 46 matching lines @@
     // Get URLs of frames, scripts etc from the DOM.
     // OnReceivedThreatDOMDetails will be called when the renderer replies.
     // TODO(mattm): In theory, if the user proceeds through the warning DOM
     // 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(

[Charlie Reis, 2017/05/05 21:03:07]

Is this called on the UI thread?  That seems unfortunate-- IPC messages are sent
to the IO thread first, then posted to the UI thread if needed.  This looks like
it turns around and sends it back to the IO thread (only to go back to the UI
thread in LookupOtherFrameId).

Maybe there's a way to simplify this?

[lpz, 2017/05/10 14:21:08]

Done by looking up the child ftnids up front here, before posting to IO thread.

     content::RenderFrameHost* sender,
     const std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node>& params) {
   // 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));

[Charlie Reis, 2017/05/05 21:03:06]

From AddDOMDetails, it looks like params has all the other/child routing IDs in
node.other_frame_routing_id values?  Could we do the
RenderFrameHost::GetFrameTreeNodeIdForRoutingId call here for all the nodes
before the post to the IO thread, sending them as an additional parameter?

Then we might be able to remove LookupOtherFrameId, and
iframe_key_to_frame_tree_id_map_ could be solely accessed from the IO thread,
never from the UI thread.

[lpz, 2017/05/10 14:21:08]

Nice thanks for this. It seems to take care of the additional thread hops.

 }
 
 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) {
   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;
-  }
-
   // 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.other_frame_routing_id,
+                    node.node_id, node.tag_name, node.parent_node_id,
+                    node.attributes, resource);
     }
   }
 }
 
+void ThreatDetails::LookupOtherFrameId(const std::string& element_key,
+                                       const int process_id,
+                                       const int other_frame_routing_id) {
+  DCHECK_CURRENTLY_ON(BrowserThread::UI);
+  int other_frame_tree_node_id =
+      content::RenderFrameHost::GetFrameTreeNodeIdForRoutingId(
+          process_id, other_frame_routing_id);
+  if (other_frame_tree_node_id == content::RenderFrameHost::kNoFrameTreeNodeId)
+    ambiguous_dom_ = true;
+  iframe_key_to_frame_tree_id_map_[element_key] = other_frame_tree_node_id;
+}
+
 // 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
+  // references to their children. Children will have been received from a

[Charlie Reis, 2017/05/05 21:03:07]

s/will/may/?

(Or does this not apply to same-process iframes?)

[lpz, 2017/05/10 14:21:08]

Done - this code doesn't do anything special for same-process iframes.

+  // 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 (element->tag() == "IFRAME" || element->tag() == "FRAME") {
+      int frame_tree_id_of_iframe_renderer =
+          iframe_key_to_frame_tree_id_map_[element_key];

[Charlie Reis, 2017/05/05 21:03:07]

This doesn't look safe.  We're reading it from the IO thread and writing it on
the UI thread, with no clear synchronization boundary between them. 
(LookupOtherFrameId doesn't post anything back when it's done-- how do we know
it's not running when we get here?)

Note: we generally try to avoid sharing data structures between threads in
Chrome for this reason.  Hopefully we can avoid it with my suggestion above.

[lpz, 2017/05/10 14:21:08]

Your suggestion should cover this. In general, though, this method is called
only after a user action (eg: navigating away from the security interstitial),
which has a high probability of happening long-after the other code has run.
However, this assumption may not always hold in the future (eg: if we share this
code for something that may not require user action).

+      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