Content API changes to improve DOM stitching in ThreatDetails code.

chrome/browser/safe_browsing/threat_details.h

 // Copyright (c) 2011 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.
 
 #ifndef CHROME_BROWSER_SAFE_BROWSING_THREAT_DETAILS_H_
 #define CHROME_BROWSER_SAFE_BROWSING_THREAT_DETAILS_H_
 
 // A class that encapsulates the detailed threat reports sent when
 // users opt-in to do so from the safe browsing warning page.
 
@@ skipping 38 matching lines @@
 using ResourceMap =
     base::hash_map<std::string,
                    std::unique_ptr<ClientSafeBrowsingReportRequest::Resource>>;
 
 // Maps a key of an HTML element to its corresponding HTMLElement proto message.
 // HTML Element keys have the form "<frame_id>-<node_id>", where |frame_id| is
 // the FrameTree NodeID of the render frame containing the element, and
 // |node_id| is a seqeuntial ID for the element generated by the renderer.
 using ElementMap = base::hash_map<std::string, std::unique_ptr<HTMLElement>>;
 
-// Maps a URL to some HTML Elements. Used to maintain parent/child relationship
-// for HTML Elements across IFrame boundaries.
-// The key is the string URL set as the src attribute of an iframe. The value is
-// the HTMLElement proto that represents the iframe element with that URL.
-// The HTMLElement protos are not owned by this map.
-using UrlToDomElementMap = base::hash_map<std::string, HTMLElement*>;
+// Maps the key of an iframe element to the FrameTreeNodeId of the render frame

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

nit: FrameTreeNode ID
nit: s/render frame/frame/

(RenderFrame is a renderer process object, but this sentence refers to the
conceptual frame that is represented by FrameTreeNode, regardless of process.)

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

Done.

+// that rendered the contents of the iframe.
+using KeyToFrameTreeIdMap = base::hash_map<std::string, int>;
 
-// Maps a URL to some Element IDs. Used to maintain parent/child relationship
-// for HTML Elements across IFrame boundaries.
-// The key is the string URL of a render frame. The value is the set of Element
-// IDs that are at the top-level of this render frame.
-using UrlToChildIdsMap = base::hash_map<std::string, std::unordered_set<int>>;
+// Maps a FrameTreeNodeId of a render frame to a set of child IDs. The child IDs
+// are the Element IDs of the top-level HTML Elements in this render frame.

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

Same: FrameTreeNode ID, s/render frame/frame/

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

Done.

+using FrameTreeIdToChildIdsMap = base::hash_map<int, std::unordered_set<int>>;
 
 class ThreatDetails : public base::RefCountedThreadSafe<
                           ThreatDetails,
                           content::BrowserThread::DeleteOnUIThread>,
                       public content::WebContentsObserver {
  public:
   typedef security_interstitials::UnsafeResource UnsafeResource;
 
   // Constructs a new ThreatDetails instance, using the factory.
   static ThreatDetails* NewThreatDetails(
@@ skipping 32 matching lines @@
   ThreatDetails(BaseUIManager* ui_manager,
                 content::WebContents* web_contents,
                 const UnsafeResource& resource,
                 net::URLRequestContextGetter* request_context_getter,
                 history::HistoryService* history_service);
 
   ~ThreatDetails() override;
 
   // Called on the IO thread with the DOM details.
   virtual void AddDOMDetails(
+      const int process_id,
       const int frame_tree_node_id,
       const GURL& frame_last_committed_url,
       const std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node>& params);
 
   // The report protocol buffer.
   std::unique_ptr<ClientSafeBrowsingReportRequest> report_;
 
   // Used to get a pointer to the HTTP cache.
   scoped_refptr<net::URLRequestContextGetter> request_context_getter_;
 
@@ skipping 27 matching lines @@
       const std::string& tagname,
       const std::vector<GURL>* children);
 
   // Message handler.
   void OnReceivedThreatDOMDetails(
       content::RenderFrameHost* sender,
       const std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node>& params);
 
   void AddRedirectUrlList(const std::vector<GURL>& urls);
 
-  // Adds an HTML Element to the DOM structure.
-  // |frame_tree_node_id| is the unique ID of the render frame the element came
-  // from. |frame_url| is the URL that the render frame was handling.
-  // |element_node_id| is a unique ID of the element within the render frame.
-  // |tag_name| is the tag of the element. |parent_element_node_id| is the
-  // unique ID of the parent element with the render frame. |attributes|
-  // containes the names and values of the element's attributes.|resource| is
-  // set if this element is a resource.
+  // Adds an HTML Element to the DOM structure. |frame_tree_node_id| is the
+  // unique ID of the render frame the element came from, and |process_id| is

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

nit: s/render frame/frame/

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

Done.

+  // that renderer's process ID. |other_frame_routing_id| is only set for iframe

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

FrameTreeNodes don't directly have a renderer process (or rather, it can change
over time).  Let's rephrase to:
"and |process_id| is that frame's current renderer process ID."

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

Done.

+  // elements, and indicates the routing_id of the render frame containing the

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

"other" is a bit vague-- I thought it might mean parent at first.  Maybe it
should be |child_frame_routing_id|?

Rephrase to:
|child_frame_routing_id| is only set for iframe elements, and indicates the
routing ID of the RenderFrame (or placeholder within |process_id|, if it is an
out-of-process iframe) containing the body of the iframe.

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

Done.

+  // body of the iframe. |element_node_id| is a unique ID of the element within
+  // the render frame. |tag_name| is the tag of the element.
+  // |parent_element_node_id| is the unique ID of the parent element with the
+  // render frame. |attributes| containes the names and values of the element's
+  // attributes.|resource| is set if this element is a resource.
   void AddDomElement(const int frame_tree_node_id,
-                     const std::string& frame_url,
+                     const int process_id,
+                     const int other_frame_routing_id,
                      const int element_node_id,
                      const std::string& tag_name,
                      const int parent_element_node_id,
                      const std::vector<AttributeNameValue>& attributes,
                      const ClientSafeBrowsingReportRequest::Resource* resource);
 
+  // Looks up the FrameTreeNodeId of a frame with |other_frame_routing_id|, and

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

nit: FrameTreeNode ID
nit: |child_frame_routing_id|

(Same below as well)

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

Method deleted.

+  // updates |iframe_key_to_frame_tree_id_map_| by mapping |element_key| to the
+  // FrameTreeNodeId that was looked up.
+  // Must be called on UI thread to perform the lookup.
+  void LookupOtherFrameId(const std::string& element_key,

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

Should Other be Child here as well?  I don't know what "other" refers to.

It's probably worth saying FrameTreeNodeId in the name as well, since frame ID
is ambiguous.

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

Method deleted.

+                          const int sender_process_id,
+                          const int other_frame_routing_id);
+
   scoped_refptr<BaseUIManager> ui_manager_;
 
   const UnsafeResource resource_;
 
   // For every Url we collect we create a Resource message. We keep
   // them in a map so we can avoid duplicates.
   ResourceMap resources_;
 
   // Store all HTML elements collected, keep them in a map for easy lookup.
   ElementMap elements_;
 
-  // For each iframe element encountered we map the src of the iframe to the
-  // iframe element. This is used when we receive elements from a different
-  // frame whose document URL matches the src of an iframe in this map. We can
-  // then add all elements from the subframe as children of the iframe element
-  // stored here.
-  UrlToDomElementMap iframe_src_to_element_map_;
+  // For each iframe element encountered we map the key of the iframe to the
+  // FrameTreeNodeId of the render frame containing the contents of that iframe.

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

Same nits, here and below.

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

Done.

+  // Lookup of FrameTreeNodeId for an iframe element must be done on the UI
+  // thread, while other processing happens on the IO thread. So we populate
+  // this map as we encounter iframes, but use it in a second pass (after
+  // FinishCollection) to attach children to iframe elements.
+  KeyToFrameTreeIdMap iframe_key_to_frame_tree_id_map_;
 
   // When getting a set of elements from a render frame, we store the frame's
-  // URL and a collection of all the top-level elements in that frame. When we
-  // later encounter the parent iframe with the same src URL, we can add all of
-  // these elements as children of that iframe.
-  UrlToChildIdsMap document_url_to_children_map_;
+  // FrameTreeNodeId and a collection of all top-level elements in that frame.
+  // It is populated as we receive sets of nodes from different render frames.
+  // It is used together with |iframe_key_to_frame_tree_id_map_| in a second
+  // pass to insert child elements under their parent iframe elements.
+  FrameTreeIdToChildIdsMap frame_tree_id_to_children_map_;
 
   // Result from the cache extractor.
   bool cache_result_;
 
   // Whether user did proceed with the safe browsing blocking page or
   // not.
   bool did_proceed_;
 
   // How many times this user has visited this page before.
   int num_visits_;
@@ skipping 34 matching lines @@
       BaseUIManager* ui_manager,
       content::WebContents* web_contents,
       const security_interstitials::UnsafeResource& unsafe_resource,
       net::URLRequestContextGetter* request_context_getter,
       history::HistoryService* history_service) = 0;
 };
 
 }  // namespace safe_browsing
 
 #endif  // CHROME_BROWSER_SAFE_BROWSING_THREAT_DETAILS_H_