Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2837603002:
Content API changes to improve DOM stitching in ThreatDetails code. (Closed)
|
Created:
3 years, 8 months ago by lpz Modified:
3 years, 7 months ago CC:
chromium-reviews, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org, creis+watch_chromium.org, vakh+watch_chromium.org, droger+watchlist_chromium.org, grt+watch_chromium.org, sdefresne+watchlist_chromium.org, jam, timvolodine, darin-cc_chromium.org, blundell+watchlist_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionExposes some methods in the content layer to be able to 1) lookup routing_ids of FrameOwnerElements and 2) get FrameTreeNodeIDs for those frames across process boundaries.
This is an effort to improve DOM stitching capability in SafeBrowsing code, which mimics some of what Accessibility does for DOM stitching.
BUG=706823
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Review-Url: https://codereview.chromium.org/2837603002
Cr-Commit-Position: refs/heads/master@{#474642}
Committed: https://chromium.googlesource.com/chromium/src/+/e83861a5ea91d04266a9ff8cd4f2e46a5d3e93be
Patch Set 1 #Patch Set 2 : Remove unnecessary deps #
Total comments: 16
Patch Set 3 : Address comments and sync #Patch Set 4 : Set output pointers correctly #
Total comments: 62
Patch Set 5 : Sync #Patch Set 6 : Address feedback #
Total comments: 21
Patch Set 7 : Adddress comments, fix unittests #Patch Set 8 : Sync and small unittest tweak #
Total comments: 10
Patch Set 9 : Address comments #
Total comments: 5
Patch Set 10 : Fix comment, move web_frame_util function #
Total comments: 2
Patch Set 11 : Sync #
Total comments: 2
Patch Set 12 : Use explicitly-sized int types in IPC definition #Messages
Total messages: 89 (65 generated)
Description was changed from ========== Test hack to improve DOM stitching in ThreatDetails code. Exposes some methods in the content layer to be able to 1) lookup routing_ids of FrameOwnerElements and 2) get FrameTreeNodeIDs for tthose frames across process boundaries. BUG=706823 ========== to ========== Test hack to improve DOM stitching in ThreatDetails code. Exposes some methods in the content layer to be able to 1) lookup routing_ids of FrameOwnerElements and 2) get FrameTreeNodeIDs for tthose frames across process boundaries. BUG=706823 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
dmazzoni@chromium.org changed reviewers: + dmazzoni@chromium.org
https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... content/public/browser/render_frame_host.h:66: static int LookupOtherFrameTreeNodeId(int process_id, int routing_id); What does "Other" mean in this context? I think a more natural API would be something like: static RenderFrameHost* FromProcessIdAndRoutingId(int process_id, int routing_id)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2837603002/diff/20001/components/safe_browsin... File components/safe_browsing/renderer/threat_dom_details.cc (right): https://codereview.chromium.org/2837603002/diff/20001/components/safe_browsin... components/safe_browsing/renderer/threat_dom_details.cc:151: if (child_node.tag_name == "IFRAME" || child_node.tag_name == "FRAME") { Checking the tag name seems brittle to me, even though it should work. I'd just call WebFrame::FromFrameOwnerElement, since it will just return nullptr if you pass it something that's not a frame. https://codereview.chromium.org/2837603002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3387: // Note: this code is copied/modified from RoutingIDToAXTreeID (below). Let's try to share this and come up with a cleaner name. https://codereview.chromium.org/2837603002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3397: return -1; Is there a constant somewhere like kNoFrameTreeNodeId? https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: int RenderFrame::GetRoutingIdForWebFrame(blink::WebFrame* web_frame) { Do you need this if it's just a one-liner?
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Test hack to improve DOM stitching in ThreatDetails code. Exposes some methods in the content layer to be able to 1) lookup routing_ids of FrameOwnerElements and 2) get FrameTreeNodeIDs for tthose frames across process boundaries. BUG=706823 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Exposes some methods in the content layer to be able to 1) lookup routing_ids of FrameOwnerElements and 2) get FrameTreeNodeIDs for those frames across process boundaries. This is an effort to improve DOM stitching capability in SafeBrowsing code, which mimics some of what Accessibility does for DOM stitching. BUG=706823 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
lpz@chromium.org changed reviewers: + creis@chromium.org
lpz@chromium.org changed reviewers: + nparker@chromium.org
Thanks Dominic for the feedback so far. +Charlie for a closer look as well +Nathan as FYI for now, and for SB code once the content discussion shakes out. https://codereview.chromium.org/2837603002/diff/20001/components/safe_browsin... File components/safe_browsing/renderer/threat_dom_details.cc (right): https://codereview.chromium.org/2837603002/diff/20001/components/safe_browsin... components/safe_browsing/renderer/threat_dom_details.cc:151: if (child_node.tag_name == "IFRAME" || child_node.tag_name == "FRAME") { On 2017/05/01 20:10:07, dmazzoni wrote: > Checking the tag name seems brittle to me, even though it > should work. I'd just call WebFrame::FromFrameOwnerElement, > since it will just return nullptr if you pass it something > that's not a frame. Thanks, done. https://codereview.chromium.org/2837603002/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3387: // Note: this code is copied/modified from RoutingIDToAXTreeID (below). On 2017/05/01 20:10:07, dmazzoni wrote: > Let's try to share this and come up with a cleaner name. Done. https://codereview.chromium.org/2837603002/diff/20001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3397: return -1; On 2017/05/01 20:10:07, dmazzoni wrote: > Is there a constant somewhere like kNoFrameTreeNodeId? Couldn't find one so added it. https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... content/public/browser/render_frame_host.h:66: static int LookupOtherFrameTreeNodeId(int process_id, int routing_id); On 2017/04/21 15:48:48, dmazzoni wrote: > What does "Other" mean in this context? > > I think a more natural API would be something like: > > static RenderFrameHost* FromProcessIdAndRoutingId(int process_id, int > routing_id) Renamed this function to be more generic. For "other", I just used reused the wording from the safe browsing code (other_frame_routing_id), where other refers to the fact that the contents of an iframe are in some "other" renderer from the one that sees the iframe element. As for returning a RenderFrameHost*, how would we support the case where the {pid,rid} pair points to a RenderFrameProxyHost? https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: int RenderFrame::GetRoutingIdForWebFrame(blink::WebFrame* web_frame) { On 2017/05/01 20:10:07, dmazzoni wrote: > Do you need this if it's just a one-liner? GetRoutingIdForFrameOrProxy is from web_frame_utils, which isn't public, so I'm exposing it through this method. Or did you mean that I should just inline it in the header? In which case that seems inconsistent with the rest of the code here - there are a bunch of 1-liners in this file and no code in render_frame.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks-- it's been a busy 2 days, but I'll try to look tomorrow!
https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... content/public/browser/render_frame_host.h:66: static int LookupOtherFrameTreeNodeId(int process_id, int routing_id); On 2017/05/02 19:36:24, lpz wrote: > On 2017/04/21 15:48:48, dmazzoni wrote: > > What does "Other" mean in this context? > > > > I think a more natural API would be something like: > > > > static RenderFrameHost* FromProcessIdAndRoutingId(int process_id, int > > routing_id) > > Renamed this function to be more generic. > > For "other", I just used reused the wording from the safe browsing code > (other_frame_routing_id), where other refers to the fact that the contents of an > iframe are in some "other" renderer from the one that sees the iframe element. > > As for returning a RenderFrameHost*, how would we support the case where the > {pid,rid} pair points to a RenderFrameProxyHost? Every RenderFrameProxyHost has an associated RenderFrameHost for the process that actually hosts that frame, so you could use that as the canonical one. https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: int RenderFrame::GetRoutingIdForWebFrame(blink::WebFrame* web_frame) { On 2017/05/02 19:36:25, lpz wrote: > On 2017/05/01 20:10:07, dmazzoni wrote: > > Do you need this if it's just a one-liner? > > GetRoutingIdForFrameOrProxy is from web_frame_utils, which isn't public, so I'm > exposing it through this method. > Or did you mean that I should just inline it in the header? In which case that > seems inconsistent with the rest of the code here - there are a bunch of > 1-liners in this file and no code in render_frame.h No preference, just curious if there's some way to have just one function to do the same thing. Could it be added to render_frame.h and then deleted from web_frame_utils, so that everyone could just call RenderFrame::GetRoutingIdForWebFrame? If not, this is fine.
Most of the notes here are just rephrasing for the comments, but I have a concern about data races and thread hops. I've left a suggestion to see if we can simplify it. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details.cc (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:319: // For iframes, lookup the frame tree node id of the render frame that nit: FrameTreeNode ID of the frame that https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:327: process_id, other_frame_routing_id)); 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.] https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:431: void ThreatDetails::OnReceivedThreatDOMDetails( 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? https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:440: sender->GetLastCommittedURL(), params)); 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. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:505: // references to their children. Children will have been received from a s/will/may/? (Or does this not apply to same-process iframes?) https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:512: iframe_key_to_frame_tree_id_map_[element_key]; 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. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:59: // Maps the key of an iframe element to the FrameTreeNodeId of the render frame 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.) https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:64: // are the Element IDs of the top-level HTML Elements in this render frame. Same: FrameTreeNode ID, s/render frame/frame/ https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:167: // unique ID of the render frame the element came from, and |process_id| is nit: s/render frame/frame/ https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:168: // that renderer's process ID. |other_frame_routing_id| is only set for iframe 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." https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:169: // elements, and indicates the routing_id of the render frame containing the "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. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:184: // Looks up the FrameTreeNodeId of a frame with |other_frame_routing_id|, and nit: FrameTreeNode ID nit: |child_frame_routing_id| (Same below as well) https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:188: void LookupOtherFrameId(const std::string& element_key, 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. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:204: // FrameTreeNodeId of the render frame containing the contents of that iframe. Same nits, here and below. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details_unittest.cc (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:55: // static const char* kDOMChildUrl2 = "https://www.domchild2.com/path"; I don't think you meant to leave all this commented out code in the test, right? :) https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... File components/safe_browsing/common/safebrowsing_messages.h (right): https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/common/safebrowsing_messages.h:44: // If this node represents a Frame or Iframe, then this field is set to the nit: frame or iframe https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/common/safebrowsing_messages.h:45: // routing_id of the renderer that is responsible for rendering the contents nit: routing ID of the local or remote frame in this renderer process that is (Routing IDs identify frames/proxies, not renderers.) https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/common/safebrowsing_messages.h:47: IPC_STRUCT_MEMBER(int, other_frame_routing_id) child_frame_routing_id https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... File components/safe_browsing/renderer/threat_dom_details.cc (right): https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/renderer/threat_dom_details.cc:149: // The body of an iframe will be in a different renderer. Look up the s/will/may/ https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/renderer/threat_dom_details.cc:150: // routing_id of that renderer and store it with the iframe node. If this nit: routing ID of the local or remote frame and https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:308: *rfph = RenderFrameProxyHost::FromID(process_id, routing_id); nit: Let's swap the order and do rfh first, just because it's the more common case. https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3548: } else if (rfh) { nit: Let's swap the order and do the rfh branch first. (Same as above.) https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3564: rfh = frame_tree_node->render_manager()->current_frame_host(); Seems like we can clean up these 4 lines quite a bit, to: rfh = rfph->frame_tree_node()->current_frame_host(); dmazzoni@: I'm guessing this just predated those APIs? https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.h:84: int GetFrameTreeNodeId() const; This looks unused-- can we remove it? If we need to keep it, it should be inlined in this file as frame_tree_node_id() (since it's trivial and non-virtual). https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:52: // Constant used to denote that a lookup of a FrameTreeNodeId has failed. nit: FrameTreeNode ID https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:69: // Returns the FrameTreeNodeId of either the RenderFrameHost or the nit: FrameTreeNode ID https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:71: // pair, depending on whether they identify a local or remote frame. Let's rephrase, because RenderFrameProxyHost isn't part of the public API and the term "remote frame" isn't meaningful in the browser process. Something like: Returns the FrameTreeNode ID corresponding to the specified |process_id| and |routing_id|. This routing ID pair may represent a placeholder for frame that is currently rendered in a different process than |process_id|. https://codereview.chromium.org/2837603002/diff/60001/content/public/renderer... File content/public/renderer/render_frame.h (right): https://codereview.chromium.org/2837603002/diff/60001/content/public/renderer... content/public/renderer/render_frame.h:105: static int GetRoutingIdForWebFrame(blink::WebFrame* web_frame); Please add a comment. Note that it's easier to talk about remote frames in the renderer process than the browser process. Something like: Returns the routing ID for |web_frame|, whether it is a WebLocalFrame in this process or a WebRemoteFrame placeholder for a frame in a different process.
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks all for the insights https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/20001/content/public/browser/... content/public/browser/render_frame_host.h:66: static int LookupOtherFrameTreeNodeId(int process_id, int routing_id); On 2017/05/05 17:32:28, dmazzoni wrote: > On 2017/05/02 19:36:24, lpz wrote: > > On 2017/04/21 15:48:48, dmazzoni wrote: > > > What does "Other" mean in this context? > > > > > > I think a more natural API would be something like: > > > > > > static RenderFrameHost* FromProcessIdAndRoutingId(int process_id, int > > > routing_id) > > > > Renamed this function to be more generic. > > > > For "other", I just used reused the wording from the safe browsing code > > (other_frame_routing_id), where other refers to the fact that the contents of > an > > iframe are in some "other" renderer from the one that sees the iframe element. > > > > As for returning a RenderFrameHost*, how would we support the case where the > > {pid,rid} pair points to a RenderFrameProxyHost? > > Every RenderFrameProxyHost has an associated RenderFrameHost > for the process that actually hosts that frame, so you could > use that as the canonical one. Done. https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: int RenderFrame::GetRoutingIdForWebFrame(blink::WebFrame* web_frame) { On 2017/05/05 17:32:28, dmazzoni wrote: > On 2017/05/02 19:36:25, lpz wrote: > > On 2017/05/01 20:10:07, dmazzoni wrote: > > > Do you need this if it's just a one-liner? > > > > GetRoutingIdForFrameOrProxy is from web_frame_utils, which isn't public, so > I'm > > exposing it through this method. > > Or did you mean that I should just inline it in the header? In which case that > > seems inconsistent with the rest of the code here - there are a bunch of > > 1-liners in this file and no code in render_frame.h > > No preference, just curious if there's some way to have just one > function to do the same thing. Could it be added to render_frame.h > and then deleted from web_frame_utils, so that everyone could just > call RenderFrame::GetRoutingIdForWebFrame? > > If not, this is fine. Will defer to creis@ - any preference on this? https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details.cc (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:319: // For iframes, lookup the frame tree node id of the render frame that On 2017/05/05 21:03:06, Charlie Reis wrote: > nit: FrameTreeNode ID of the frame that Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:327: process_id, other_frame_routing_id)); On 2017/05/05 21:03:06, Charlie Reis wrote: > 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.] Applied suggestion https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:431: void ThreatDetails::OnReceivedThreatDOMDetails( On 2017/05/05 21:03:07, Charlie Reis wrote: > 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? Done by looking up the child ftnids up front here, before posting to IO thread. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:440: sender->GetLastCommittedURL(), params)); On 2017/05/05 21:03:06, Charlie Reis wrote: > 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. Nice thanks for this. It seems to take care of the additional thread hops. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:505: // references to their children. Children will have been received from a On 2017/05/05 21:03:07, Charlie Reis wrote: > s/will/may/? > > (Or does this not apply to same-process iframes?) Done - this code doesn't do anything special for same-process iframes. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.cc:512: iframe_key_to_frame_tree_id_map_[element_key]; On 2017/05/05 21:03:07, Charlie Reis wrote: > 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. 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). https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:59: // Maps the key of an iframe element to the FrameTreeNodeId of the render frame On 2017/05/05 21:03:07, Charlie Reis wrote: > 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.) Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:64: // are the Element IDs of the top-level HTML Elements in this render frame. On 2017/05/05 21:03:07, Charlie Reis wrote: > Same: FrameTreeNode ID, s/render frame/frame/ Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:167: // unique ID of the render frame the element came from, and |process_id| is On 2017/05/05 21:03:07, Charlie Reis wrote: > nit: s/render frame/frame/ Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:168: // that renderer's process ID. |other_frame_routing_id| is only set for iframe On 2017/05/05 21:03:07, Charlie Reis wrote: > 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." Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:169: // elements, and indicates the routing_id of the render frame containing the On 2017/05/05 21:03:07, Charlie Reis wrote: > "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. Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:184: // Looks up the FrameTreeNodeId of a frame with |other_frame_routing_id|, and On 2017/05/05 21:03:07, Charlie Reis wrote: > nit: FrameTreeNode ID > nit: |child_frame_routing_id| > > (Same below as well) Method deleted. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:188: void LookupOtherFrameId(const std::string& element_key, On 2017/05/05 21:03:07, Charlie Reis wrote: > 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. Method deleted. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details.h:204: // FrameTreeNodeId of the render frame containing the contents of that iframe. On 2017/05/05 21:03:07, Charlie Reis wrote: > Same nits, here and below. Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details_unittest.cc (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:55: // static const char* kDOMChildUrl2 = "https://www.domchild2.com/path"; On 2017/05/05 21:03:07, Charlie Reis wrote: > I don't think you meant to leave all this commented out code in the test, right? > :) Not forever, but maybe you can help with this part :) These tests currently fail because the call to RenderFrameHost::GetFrameTreeNodeIdForRoutingId doesn't work (I'm assuming because g_routing_id_frame_proxy_map isn't populated correctly) - it just returns -1. Is there support for mocking this lookup in content somewhere? My other options seem to be to just delete these test (we do have browser tests that exercise similar stuff), or try to figure out a way to plug in a fake object to do the lookup. Thoughts? https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... File components/safe_browsing/common/safebrowsing_messages.h (right): https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/common/safebrowsing_messages.h:44: // If this node represents a Frame or Iframe, then this field is set to the On 2017/05/05 21:03:07, Charlie Reis wrote: > nit: frame or iframe Done. https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/common/safebrowsing_messages.h:45: // routing_id of the renderer that is responsible for rendering the contents On 2017/05/05 21:03:07, Charlie Reis wrote: > nit: routing ID of the local or remote frame in this renderer process that is > > (Routing IDs identify frames/proxies, not renderers.) Done. https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/common/safebrowsing_messages.h:47: IPC_STRUCT_MEMBER(int, other_frame_routing_id) On 2017/05/05 21:03:07, Charlie Reis wrote: > child_frame_routing_id Done. https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... File components/safe_browsing/renderer/threat_dom_details.cc (right): https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/renderer/threat_dom_details.cc:149: // The body of an iframe will be in a different renderer. Look up the On 2017/05/05 21:03:08, Charlie Reis wrote: > s/will/may/ Done. https://codereview.chromium.org/2837603002/diff/60001/components/safe_browsin... components/safe_browsing/renderer/threat_dom_details.cc:150: // routing_id of that renderer and store it with the iframe node. If this On 2017/05/05 21:03:07, Charlie Reis wrote: > nit: routing ID of the local or remote frame and Done. https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_host_impl.cc (right): https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:308: *rfph = RenderFrameProxyHost::FromID(process_id, routing_id); On 2017/05/05 21:03:08, Charlie Reis wrote: > nit: Let's swap the order and do rfh first, just because it's the more common > case. Done. https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3548: } else if (rfh) { On 2017/05/05 21:03:08, Charlie Reis wrote: > nit: Let's swap the order and do the rfh branch first. (Same as above.) Done. https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_host_impl.cc:3564: rfh = frame_tree_node->render_manager()->current_frame_host(); On 2017/05/05 21:03:08, Charlie Reis wrote: > Seems like we can clean up these 4 lines quite a bit, to: > rfh = rfph->frame_tree_node()->current_frame_host(); > > dmazzoni@: I'm guessing this just predated those APIs? Done. https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_frame_proxy_host.h (right): https://codereview.chromium.org/2837603002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_frame_proxy_host.h:84: int GetFrameTreeNodeId() const; On 2017/05/05 21:03:08, Charlie Reis wrote: > This looks unused-- can we remove it? > > If we need to keep it, it should be inlined in this file as frame_tree_node_id() > (since it's trivial and non-virtual). removed. https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:52: // Constant used to denote that a lookup of a FrameTreeNodeId has failed. On 2017/05/05 21:03:08, Charlie Reis wrote: > nit: FrameTreeNode ID Done. https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:69: // Returns the FrameTreeNodeId of either the RenderFrameHost or the On 2017/05/05 21:03:08, Charlie Reis wrote: > nit: FrameTreeNode ID Done. https://codereview.chromium.org/2837603002/diff/60001/content/public/browser/... content/public/browser/render_frame_host.h:71: // pair, depending on whether they identify a local or remote frame. On 2017/05/05 21:03:08, Charlie Reis wrote: > Let's rephrase, because RenderFrameProxyHost isn't part of the public API and > the term "remote frame" isn't meaningful in the browser process. Something > like: > > Returns the FrameTreeNode ID corresponding to the specified |process_id| and > |routing_id|. This routing ID pair may represent a placeholder for frame that is > currently rendered in a different process than |process_id|. Done, and also changed to return a RenderFrameHost* instead of int as per dmazonni's suggestion. https://codereview.chromium.org/2837603002/diff/60001/content/public/renderer... File content/public/renderer/render_frame.h (right): https://codereview.chromium.org/2837603002/diff/60001/content/public/renderer... content/public/renderer/render_frame.h:105: static int GetRoutingIdForWebFrame(blink::WebFrame* web_frame); On 2017/05/05 21:03:08, Charlie Reis wrote: > Please add a comment. Note that it's easier to talk about remote frames in the > renderer process than the browser process. Something like: > > Returns the routing ID for |web_frame|, whether it is a WebLocalFrame in this > process or a WebRemoteFrame placeholder for a frame in a different process. Done.
Thanks! Looking much better now. One concern about the public API and a few suggestions on the tests. https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: int RenderFrame::GetRoutingIdForWebFrame(blink::WebFrame* web_frame) { On 2017/05/10 14:21:08, lpz wrote: > On 2017/05/05 17:32:28, dmazzoni wrote: > > On 2017/05/02 19:36:25, lpz wrote: > > > On 2017/05/01 20:10:07, dmazzoni wrote: > > > > Do you need this if it's just a one-liner? > > > > > > GetRoutingIdForFrameOrProxy is from web_frame_utils, which isn't public, so > > I'm > > > exposing it through this method. > > > Or did you mean that I should just inline it in the header? In which case > that > > > seems inconsistent with the rest of the code here - there are a bunch of > > > 1-liners in this file and no code in render_frame.h > > > > No preference, just curious if there's some way to have just one > > function to do the same thing. Could it be added to render_frame.h > > and then deleted from web_frame_utils, so that everyone could just > > call RenderFrame::GetRoutingIdForWebFrame? > > > > If not, this is fine. > > Will defer to creis@ - any preference on this? With the new API, it's a little less immediately obvious within implementation code that the routing ID could be for a frame or proxy, but I think that's probably ok. I'm fine with Dominic's suggestion to just replace GetRoutingIdForFrameOrProxy with RenderFrame::GetRoutingIdForWebFrame. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details_unittest.cc (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:55: // static const char* kDOMChildUrl2 = "https://www.domchild2.com/path"; On 2017/05/10 14:21:09, lpz wrote: > On 2017/05/05 21:03:07, Charlie Reis wrote: > > I don't think you meant to leave all this commented out code in the test, > right? > > :) > > Not forever, but maybe you can help with this part :) > > These tests currently fail because the call to > RenderFrameHost::GetFrameTreeNodeIdForRoutingId doesn't work (I'm assuming > because g_routing_id_frame_proxy_map isn't populated correctly) - it just > returns -1. Is there support for mocking this lookup in content somewhere? My > other options seem to be to just delete these test (we do have browser tests > that exercise similar stuff), or try to figure out a way to plug in a fake > object to do the lookup. > > Thoughts? Ah. Yes, it's pretty hard to test subframes and OOPIFs in unit tests, without having renderer processes involved. It does look like you might be using the wrong (or missing) routing IDs below for child frames, but then again, there probably aren't any child RenderFrameHosts or RenderFrameProxyHosts created in this test. That explains why we can't look them up. I suppose you could use TestRenderFrameHost::AppendChild to create some subframes that have routing IDs, at which point you could simulate the renderer passing up that routing ID. That might let you keep this test code. It may even be possible to simulate an OOPIF that way, but I wouldn't recommend it-- it'd be fragile, and it's something that is better to test in a browser test. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:570: // outer_child_iframe.other_frame_routing_id = main_rfh()->GetRoutingID(); This looks like one problem-- we're using the main frame's routing ID when it should be the child frame's routing ID, right? https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:769: // outer_child_node.tag_name = "frame"; Should this one have a child_frame_routing_id as well? https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... File components/safe_browsing/browser/threat_details.cc (right): https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.cc:292: const int child_frame_routing_id, Both process_id and child_frame_routing_id look unused now. Can we remove them as parameters? https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.cc:422: // Lookup the FrameTreeNodeId of any child frames in the list of DOM nodes. nit: FrameTreeNode ID https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.cc:505: // Do a second pass over the elements and update iframe elements to have Side note: I don't fully understand this second pass, so hopefully there's good test coverage for it. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:64: // the Element IDs of the top-level HTML Elements in this render frame. nit: s/render frame/frame/ https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:202: KeyToFrameTreeIdMap iframe_key_to_frame_tree_id_map_; Please add a note that this can only be accessed on the IO thread. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:206: // It is populated as we receive sets of nodes from different render frames. nit: s/render frames/frames/ (Or RenderFrames if you want to emphasize that they come from that class in the renderer process.) https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:226: // The factory used to instanciate SafeBrowsingBlockingPage objects. nit: This was correct before as "instantiate" I'm guessing this was due to a merge conflict? https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:227: // Usefull for tests, so they can provide their own implementation of nit: This was correct before as "Useful" https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... File components/safe_browsing/renderer/threat_dom_details.cc (right): https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/renderer/threat_dom_details.cc:152: blink::WebFrame* subwebframe = nit: subframe https://codereview.chromium.org/2837603002/diff/100001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/100001/content/public/browser... content/public/browser/render_frame_host.h:68: // currently rendered in a different process than |process_id|. Hmm, I really prefer the previous version of the API that returned a FrameTreeNode ID, which looks like it would be sufficient for your use case in ThreatDetails::OnReceivedThreatDOMDetails. The problem with the current API is that it's too easy for callers to assume that the RenderFrameHost returned here is actually in |process_id|, since that's the value they passed in to look it up. That could very easily lead to security bugs where we assume a frame has access to something that it does not. Can we switch back to GetFrameTreeNodeIdForRoutingId?
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lpz@chromium.org changed reviewers: + estark@chromium.org
Thanks again, Charlie! +estark for safebrowsing_messages.h @nathan: this is ready for your attention now too I think. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... File chrome/browser/safe_browsing/threat_details_unittest.cc (right): https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:55: // static const char* kDOMChildUrl2 = "https://www.domchild2.com/path"; On 2017/05/10 22:17:49, Charlie Reis wrote: > On 2017/05/10 14:21:09, lpz wrote: > > On 2017/05/05 21:03:07, Charlie Reis wrote: > > > I don't think you meant to leave all this commented out code in the test, > > right? > > > :) > > > > Not forever, but maybe you can help with this part :) > > > > These tests currently fail because the call to > > RenderFrameHost::GetFrameTreeNodeIdForRoutingId doesn't work (I'm assuming > > because g_routing_id_frame_proxy_map isn't populated correctly) - it just > > returns -1. Is there support for mocking this lookup in content somewhere? My > > other options seem to be to just delete these test (we do have browser tests > > that exercise similar stuff), or try to figure out a way to plug in a fake > > object to do the lookup. > > > > Thoughts? > > Ah. Yes, it's pretty hard to test subframes and OOPIFs in unit tests, without > having renderer processes involved. > > It does look like you might be using the wrong (or missing) routing IDs below > for child frames, but then again, there probably aren't any child > RenderFrameHosts or RenderFrameProxyHosts created in this test. That explains > why we can't look them up. > > I suppose you could use TestRenderFrameHost::AppendChild to create some > subframes that have routing IDs, at which point you could simulate the renderer > passing up that routing ID. That might let you keep this test code. > > It may even be possible to simulate an OOPIF that way, but I wouldn't recommend > it-- it'd be fragile, and it's something that is better to test in a browser > test. Awesome, AppendChild did exactly what I need here. Thanks! https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:570: // outer_child_iframe.other_frame_routing_id = main_rfh()->GetRoutingID(); On 2017/05/10 22:17:49, Charlie Reis wrote: > This looks like one problem-- we're using the main frame's routing ID when it > should be the child frame's routing ID, right? Done. https://codereview.chromium.org/2837603002/diff/60001/chrome/browser/safe_bro... chrome/browser/safe_browsing/threat_details_unittest.cc:769: // outer_child_node.tag_name = "frame"; On 2017/05/10 22:17:49, Charlie Reis wrote: > Should this one have a child_frame_routing_id as well? Done. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... File components/safe_browsing/browser/threat_details.cc (right): https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.cc:292: const int child_frame_routing_id, On 2017/05/10 22:17:49, Charlie Reis wrote: > Both process_id and child_frame_routing_id look unused now. Can we remove them > as parameters? Yep, and some cascading param cleanup from this in other methods too https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.cc:422: // Lookup the FrameTreeNodeId of any child frames in the list of DOM nodes. On 2017/05/10 22:17:49, Charlie Reis wrote: > nit: FrameTreeNode ID Done. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.cc:505: // Do a second pass over the elements and update iframe elements to have On 2017/05/10 22:17:49, Charlie Reis wrote: > Side note: I don't fully understand this second pass, so hopefully there's good > test coverage for it. 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. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:64: // the Element IDs of the top-level HTML Elements in this render frame. On 2017/05/10 22:17:49, Charlie Reis wrote: > nit: s/render frame/frame/ Done. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:202: KeyToFrameTreeIdMap iframe_key_to_frame_tree_id_map_; On 2017/05/10 22:17:49, Charlie Reis wrote: > Please add a note that this can only be accessed on the IO thread. Done. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:206: // It is populated as we receive sets of nodes from different render frames. On 2017/05/10 22:17:49, Charlie Reis wrote: > nit: s/render frames/frames/ > (Or RenderFrames if you want to emphasize that they come from that class in the > renderer process.) Done, and also replaced a few more usages of "render frame" in this file. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:226: // The factory used to instanciate SafeBrowsingBlockingPage objects. On 2017/05/10 22:17:49, Charlie Reis wrote: > nit: This was correct before as "instantiate" > > I'm guessing this was due to a merge conflict? Yeah weird. Done. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:227: // Usefull for tests, so they can provide their own implementation of On 2017/05/10 22:17:49, Charlie Reis wrote: > nit: This was correct before as "Useful" Done. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... File components/safe_browsing/renderer/threat_dom_details.cc (right): https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/renderer/threat_dom_details.cc:152: blink::WebFrame* subwebframe = On 2017/05/10 22:17:49, Charlie Reis wrote: > nit: subframe Done. https://codereview.chromium.org/2837603002/diff/100001/content/public/browser... File content/public/browser/render_frame_host.h (right): https://codereview.chromium.org/2837603002/diff/100001/content/public/browser... content/public/browser/render_frame_host.h:68: // currently rendered in a different process than |process_id|. On 2017/05/10 22:17:49, Charlie Reis wrote: > Hmm, I really prefer the previous version of the API that returned a > FrameTreeNode ID, which looks like it would be sufficient for your use case in > ThreatDetails::OnReceivedThreatDOMDetails. > > The problem with the current API is that it's too easy for callers to assume > that the RenderFrameHost returned here is actually in |process_id|, since that's > the value they passed in to look it up. That could very easily lead to security > bugs where we assume a frame has access to something that it does not. > > Can we switch back to GetFrameTreeNodeIdForRoutingId? Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! Basically looks good once we fix the DEPS problem. https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... File components/safe_browsing/browser/threat_details.cc (right): https://codereview.chromium.org/2837603002/diff/100001/components/safe_browsi... components/safe_browsing/browser/threat_details.cc:505: // Do a second pass over the elements and update iframe elements to have On 2017/05/12 13:53:16, lpz wrote: > On 2017/05/10 22:17:49, Charlie Reis wrote: > > Side note: I don't fully understand this second pass, so hopefully there's > good > > test coverage for it. > > 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. Acknowledged. https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/DEPS (right): https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/DEPS:4: "+content/test/test_render_frame_host.h", chrome/ isn't allowed to depend on non-public content directories. See my comment below on how to fix. https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/threat_details_unittest.cc (right): https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_unittest.cc:554: content::TestRenderFrameHost* test_main_rfh = TestRenderFrameHost isn't a content/public class, so we can't use it directly here. It's a little convoluted, but you can say RenderFrameHostTester::For(main_rfh())->AppendChild("subframe") instead. Then you can declare child_rfh as a RenderFrameHost* as well. https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_unittest.cc:759: content::TestRenderFrameHost* test_main_rfh = Same here. https://codereview.chromium.org/2837603002/diff/140001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/140001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:55: // the FrameTree NodeID of the frame containing the element, and Heh, I keep catching more of these. :) nit: the FrameTreeNode ID https://codereview.chromium.org/2837603002/diff/140001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:167: // frame's current renderer process ID. |child_frame_routing_id| is only set Don't forget to remove the stale params from the comment as well.
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/DEPS (right): https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/DEPS:4: "+content/test/test_render_frame_host.h", On 2017/05/12 21:40:50, Charlie Reis wrote: > chrome/ isn't allowed to depend on non-public content directories. See my > comment below on how to fix. Done. https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... File chrome/browser/safe_browsing/threat_details_unittest.cc (right): https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_unittest.cc:554: content::TestRenderFrameHost* test_main_rfh = On 2017/05/12 21:40:50, Charlie Reis wrote: > TestRenderFrameHost isn't a content/public class, so we can't use it directly > here. > > It's a little convoluted, but you can say > RenderFrameHostTester::For(main_rfh())->AppendChild("subframe") instead. > > Then you can declare child_rfh as a RenderFrameHost* as well. Ahh great, thanks https://codereview.chromium.org/2837603002/diff/140001/chrome/browser/safe_br... chrome/browser/safe_browsing/threat_details_unittest.cc:759: content::TestRenderFrameHost* test_main_rfh = On 2017/05/12 21:40:50, Charlie Reis wrote: > Same here. Done. https://codereview.chromium.org/2837603002/diff/140001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/140001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:55: // the FrameTree NodeID of the frame containing the element, and On 2017/05/12 21:40:50, Charlie Reis wrote: > Heh, I keep catching more of these. :) > > nit: the FrameTreeNode ID Yeah, this one was sneaky, sorry for the churn from these :) https://codereview.chromium.org/2837603002/diff/140001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:167: // frame's current renderer process ID. |child_frame_routing_id| is only set On 2017/05/12 21:40:50, Charlie Reis wrote: > Don't forget to remove the stale params from the comment as well. Doh, done.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! LGTM with the fixes below. https://codereview.chromium.org/2837603002/diff/160001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/160001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:167: // frame's current renderer process ID. |child_frame_routing_id| is only set This comment still hasn't been updated...? :) https://codereview.chromium.org/2837603002/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:1067: return GetRoutingIdForFrameOrProxy(web_frame); Did you decide not to go with Dominic's suggestion to replace GetRoutingIdForFrameOrProxy with RenderFrame::GetRoutingIdForWebFrame, or did that just fall between the cracks? Seems like a reasonable change to me.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/20001/content/renderer/render... content/renderer/render_frame_impl.cc:1065: int RenderFrame::GetRoutingIdForWebFrame(blink::WebFrame* web_frame) { On 2017/05/10 22:17:49, Charlie Reis wrote: > On 2017/05/10 14:21:08, lpz wrote: > > On 2017/05/05 17:32:28, dmazzoni wrote: > > > On 2017/05/02 19:36:25, lpz wrote: > > > > On 2017/05/01 20:10:07, dmazzoni wrote: > > > > > Do you need this if it's just a one-liner? > > > > > > > > GetRoutingIdForFrameOrProxy is from web_frame_utils, which isn't public, > so > > > I'm > > > > exposing it through this method. > > > > Or did you mean that I should just inline it in the header? In which case > > that > > > > seems inconsistent with the rest of the code here - there are a bunch of > > > > 1-liners in this file and no code in render_frame.h > > > > > > No preference, just curious if there's some way to have just one > > > function to do the same thing. Could it be added to render_frame.h > > > and then deleted from web_frame_utils, so that everyone could just > > > call RenderFrame::GetRoutingIdForWebFrame? > > > > > > If not, this is fine. > > > > Will defer to creis@ - any preference on this? > > With the new API, it's a little less immediately obvious within implementation > code that the routing ID could be for a frame or proxy, but I think that's > probably ok. I'm fine with Dominic's suggestion to just replace > GetRoutingIdForFrameOrProxy with RenderFrame::GetRoutingIdForWebFrame. Done. https://codereview.chromium.org/2837603002/diff/160001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/160001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:167: // frame's current renderer process ID. |child_frame_routing_id| is only set On 2017/05/15 20:23:14, Charlie Reis wrote: > This comment still hasn't been updated...? :) Ooops, done this time. https://codereview.chromium.org/2837603002/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:1067: return GetRoutingIdForFrameOrProxy(web_frame); On 2017/05/15 20:23:14, Charlie Reis wrote: > Did you decide not to go with Dominic's suggestion to replace > GetRoutingIdForFrameOrProxy with RenderFrame::GetRoutingIdForWebFrame, or did > that just fall between the cracks? Seems like a reasonable change to me. Ahh, I missed that comment. Thanks, done. Btw, the only other function in web_frame_utils (GetWebFrameFromRoutingIdForFrameOrProxy) seems to be unused, so that whole file can probably be deleted. But I'd prefer to leave that for another CL.
lgtm for accessibility changes
Thanks! LGTM. https://codereview.chromium.org/2837603002/diff/160001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2837603002/diff/160001/content/renderer/rende... content/renderer/render_frame_impl.cc:1067: return GetRoutingIdForFrameOrProxy(web_frame); On 2017/05/16 15:19:57, lpz wrote: > On 2017/05/15 20:23:14, Charlie Reis wrote: > > Did you decide not to go with Dominic's suggestion to replace > > GetRoutingIdForFrameOrProxy with RenderFrame::GetRoutingIdForWebFrame, or did > > that just fall between the cracks? Seems like a reasonable change to me. > > Ahh, I missed that comment. Thanks, done. > > Btw, the only other function in web_frame_utils > (GetWebFrameFromRoutingIdForFrameOrProxy) seems to be unused, so that whole file > can probably be deleted. But I'd prefer to leave that for another CL. Ah, nice! Yes, we can remove that after this CL lands.
LGTM for safe_browsing https://codereview.chromium.org/2837603002/diff/180001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/180001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:216: // associated with a parent Element in the parent frame. Is there any explanation you can add here as to why this would happen?
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2837603002/diff/180001/components/safe_browsi... File components/safe_browsing/browser/threat_details.h (right): https://codereview.chromium.org/2837603002/diff/180001/components/safe_browsi... components/safe_browsing/browser/threat_details.h:216: // associated with a parent Element in the parent frame. On 2017/05/17 00:06:34, Nathan Parker wrote: > Is there any explanation you can add here as to why this would happen? Hrmm, nothing of substance I don't think. I'm not quite sure what conditions could cause this to happen, but it is technically possible. I'm hoping this is false all the time and can just be removed, but if it's not then we can try to track down why.
Sorry I haven't been following the discussion on this CL closely -- do you still need me to review safebrowsing_messages.h, and if so is it ready for me to review?
On 2017/05/23 03:55:09, estark wrote: > Sorry I haven't been following the discussion on this CL closely -- do you still > need me to review safebrowsing_messages.h, and if so is it ready for me to > review? Hi Emily, yes, please take a look at safebrowsing_message.h. Thanks!
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2837603002/diff/200001/components/safe_browsi... File components/safe_browsing/common/safebrowsing_messages.h (right): https://codereview.chromium.org/2837603002/diff/200001/components/safe_browsi... components/safe_browsing/common/safebrowsing_messages.h:47: IPC_STRUCT_MEMBER(int, child_frame_routing_id) I'm not sure it matters too much in this case, but https://dev.chromium.org/Home/chromium-security/education/security-tips-for-i.... says this should be an explicitly sized integer type.
The CQ bit was checked by lpz@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2837603002/diff/200001/components/safe_browsi... File components/safe_browsing/common/safebrowsing_messages.h (right): https://codereview.chromium.org/2837603002/diff/200001/components/safe_browsi... components/safe_browsing/common/safebrowsing_messages.h:47: IPC_STRUCT_MEMBER(int, child_frame_routing_id) On 2017/05/24 00:42:35, estark wrote: > I'm not sure it matters too much in this case, but > https://dev.chromium.org/Home/chromium-security/education/security-tips-for-i.... > says this should be an explicitly sized integer type. Done, and thanks for the handy link!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
safebrowsing_messages.h lgtm
The CQ bit was checked by lpz@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nparker@chromium.org, creis@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2837603002/#ps220001 (title: "Use explicitly-sized int types in IPC definition")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1495721376931400,
"parent_rev": "d4b51415288015da7a7a41e92461810ec5b3674b", "commit_rev":
"e83861a5ea91d04266a9ff8cd4f2e46a5d3e93be"}
Message was sent while issue was closed.
Description was changed from ========== Exposes some methods in the content layer to be able to 1) lookup routing_ids of FrameOwnerElements and 2) get FrameTreeNodeIDs for those frames across process boundaries. This is an effort to improve DOM stitching capability in SafeBrowsing code, which mimics some of what Accessibility does for DOM stitching. BUG=706823 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Exposes some methods in the content layer to be able to 1) lookup routing_ids of FrameOwnerElements and 2) get FrameTreeNodeIDs for those frames across process boundaries. This is an effort to improve DOM stitching capability in SafeBrowsing code, which mimics some of what Accessibility does for DOM stitching. BUG=706823 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Review-Url: https://codereview.chromium.org/2837603002 Cr-Commit-Position: refs/heads/master@{#474642} Committed: https://chromium.googlesource.com/chromium/src/+/e83861a5ea91d04266a9ff8cd4f2... ==========
Message was sent while issue was closed.
Committed patchset #12 (id:220001) as https://chromium.googlesource.com/chromium/src/+/e83861a5ea91d04266a9ff8cd4f2... |
