Content API changes to improve DOM stitching in ThreatDetails code.

chrome/browser/safe_browsing/threat_details_unittest.cc

Index: chrome/browser/safe_browsing/threat_details_unittest.cc
diff --git a/chrome/browser/safe_browsing/threat_details_unittest.cc b/chrome/browser/safe_browsing/threat_details_unittest.cc
index 33d428dc0492503dd822d7b5d3284ca6f0a99aa3..6092e8056efd6ceccce8180044dd91e392608a21 100644
--- a/chrome/browser/safe_browsing/threat_details_unittest.cc
+++ b/chrome/browser/safe_browsing/threat_details_unittest.cc
@@ -52,13 +52,13 @@ namespace {
 static const char* kOriginalLandingURL =
     "http://www.originallandingpage.com/with/path";
 static const char* kDOMChildURL = "https://www.domchild.com/with/path";
-static const char* kDOMChildUrl2 = "https://www.domchild2.com/path";
+// static const char* kDOMChildUrl2 = "https://www.domchild2.com/path";

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

I don't think you meant to leave all this commented out code in the test, right?
 :)

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

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?

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

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.

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

Awesome, AppendChild did exactly what I need here. Thanks!

 static const char* kDOMParentURL = "https://www.domparent.com/with/path";
 static const char* kFirstRedirectURL = "http://redirectone.com/with/path";
 static const char* kSecondRedirectURL = "https://redirecttwo.com/with/path";
 static const char* kReferrerURL = "http://www.referrer.com/with/path";
-static const char* kDataURL = "data:text/html;charset=utf-8;base64,PCFET0";
-static const char* kBlankURL = "about:blank";
+// static const char* kDataURL = "data:text/html;charset=utf-8;base64,PCFET0";
+// static const char* kBlankURL = "about:blank";
 
 static const char* kThreatURL = "http://www.threat.com/with/path";
 static const char* kThreatURLHttps = "https://www.threat.com/with/path";
@@ -545,194 +545,209 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails) {
 //    \- <iframe src=kDOMChildURL foo=bar>
 //      \- <div id=inner bar=baz/> - div and script are at the same level.
 //      \- <script src=kDOMChildURL2>
-TEST_F(ThreatDetailsTest, ThreatDOMDetails_MultipleFrames) {
-  // Define two sets of DOM nodes - one for an outer page containing an iframe,
-  // and then another for the inner page containing the contents of that iframe.
-  std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> outer_params;
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_child_div;
-  outer_child_div.node_id = 1;
-  outer_child_div.child_node_ids.push_back(2);
-  outer_child_div.tag_name = "div";
-  outer_child_div.parent = GURL(kDOMParentURL);
-  outer_child_div.attributes.push_back(std::make_pair("id", "outer"));
-  outer_params.push_back(outer_child_div);
-
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_child_iframe;
-  outer_child_iframe.node_id = 2;
-  outer_child_iframe.parent_node_id = 1;
-  outer_child_iframe.url = GURL(kDOMChildURL);
-  outer_child_iframe.tag_name = "iframe";
-  outer_child_iframe.parent = GURL(kDOMParentURL);
-  outer_child_iframe.attributes.push_back(std::make_pair("src", kDOMChildURL));
-  outer_child_iframe.attributes.push_back(std::make_pair("foo", "bar"));
-  outer_params.push_back(outer_child_iframe);
-
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_summary_node;
-  outer_summary_node.url = GURL(kDOMParentURL);
-  outer_summary_node.children.push_back(GURL(kDOMChildURL));
-  outer_params.push_back(outer_summary_node);
-
-  // Now define some more nodes for the body of the iframe.
-  std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> inner_params;
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_child_div;
-  inner_child_div.node_id = 1;
-  inner_child_div.tag_name = "div";
-  inner_child_div.parent = GURL(kDOMChildURL);
-  inner_child_div.attributes.push_back(std::make_pair("id", "inner"));
-  inner_child_div.attributes.push_back(std::make_pair("bar", "baz"));
-  inner_params.push_back(inner_child_div);
-
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_child_script;
-  inner_child_script.node_id = 2;
-  inner_child_script.url = GURL(kDOMChildUrl2);
-  inner_child_script.tag_name = "script";
-  inner_child_script.parent = GURL(kDOMChildURL);
-  inner_child_script.attributes.push_back(std::make_pair("src", kDOMChildUrl2));
-  inner_params.push_back(inner_child_script);
-
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_summary_node;
-  inner_summary_node.url = GURL(kDOMChildURL);
-  inner_summary_node.children.push_back(GURL(kDOMChildUrl2));
-  inner_params.push_back(inner_summary_node);
-
-  ClientSafeBrowsingReportRequest expected;
-  expected.set_type(ClientSafeBrowsingReportRequest::URL_UNWANTED);
-  expected.set_url(kThreatURL);
-  expected.set_page_url(kLandingURL);
-  expected.set_referrer_url("");
-  expected.set_did_proceed(false);
-  expected.set_repeat_visit(false);
-
-  ClientSafeBrowsingReportRequest::Resource* pb_resource =
-      expected.add_resources();
-  pb_resource->set_id(0);
-  pb_resource->set_url(kLandingURL);
-
-  pb_resource = expected.add_resources();
-  pb_resource->set_id(1);
-  pb_resource->set_url(kThreatURL);
-
-  ClientSafeBrowsingReportRequest::Resource* res_dom_child =
-      expected.add_resources();
-  res_dom_child->set_id(2);
-  res_dom_child->set_url(kDOMChildURL);
-  res_dom_child->set_parent_id(3);
-  res_dom_child->add_child_ids(4);
-
-  ClientSafeBrowsingReportRequest::Resource* res_dom_parent =
-      expected.add_resources();
-  res_dom_parent->set_id(3);
-  res_dom_parent->set_url(kDOMParentURL);
-  res_dom_parent->add_child_ids(2);
-
-  ClientSafeBrowsingReportRequest::Resource* res_dom_child2 =
-      expected.add_resources();
-  res_dom_child2->set_id(4);
-  res_dom_child2->set_url(kDOMChildUrl2);
-  res_dom_child2->set_parent_id(2);
-
-  expected.set_complete(false);  // Since the cache was missing.
-
-  HTMLElement* elem_dom_outer_div = expected.add_dom();
-  elem_dom_outer_div->set_id(0);
-  elem_dom_outer_div->set_tag("DIV");
-  elem_dom_outer_div->add_attribute()->set_name("id");
-  elem_dom_outer_div->mutable_attribute(0)->set_value("outer");
-  elem_dom_outer_div->add_child_ids(1);
-
-  HTMLElement* elem_dom_outer_iframe = expected.add_dom();
-  elem_dom_outer_iframe->set_id(1);
-  elem_dom_outer_iframe->set_tag("IFRAME");
-  elem_dom_outer_iframe->set_resource_id(res_dom_child->id());
-  elem_dom_outer_iframe->add_attribute()->set_name("src");
-  elem_dom_outer_iframe->mutable_attribute(0)->set_value(kDOMChildURL);
-  elem_dom_outer_iframe->add_attribute()->set_name("foo");
-  elem_dom_outer_iframe->mutable_attribute(1)->set_value("bar");
-  elem_dom_outer_iframe->add_child_ids(2);
-  elem_dom_outer_iframe->add_child_ids(3);
-
-  HTMLElement* elem_dom_inner_div = expected.add_dom();
-  elem_dom_inner_div->set_id(2);
-  elem_dom_inner_div->set_tag("DIV");
-  elem_dom_inner_div->add_attribute()->set_name("id");
-  elem_dom_inner_div->mutable_attribute(0)->set_value("inner");
-  elem_dom_inner_div->add_attribute()->set_name("bar");
-  elem_dom_inner_div->mutable_attribute(1)->set_value("baz");
-
-  HTMLElement* elem_dom_inner_script = expected.add_dom();
-  elem_dom_inner_script->set_id(3);
-  elem_dom_inner_script->set_tag("SCRIPT");
-  elem_dom_inner_script->set_resource_id(res_dom_child2->id());
-  elem_dom_inner_script->add_attribute()->set_name("src");
-  elem_dom_inner_script->mutable_attribute(0)->set_value(kDOMChildUrl2);
-
-  content::WebContentsTester::For(web_contents())
-      ->NavigateAndCommit(GURL(kLandingURL));
-
-  UnsafeResource resource;
-  InitResource(&resource, SB_THREAT_TYPE_URL_UNWANTED,
-               true /* is_subresource */, GURL(kThreatURL));
-
-  // Send both sets of nodes, from different render frames.
-  {
-    scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
-        ui_manager_.get(), web_contents(), resource, NULL, history_service());
-
-    // We call AddDOMDetails directly so we can specify different render frame
-    // IDs.
-    report->AddDOMDetails(100, GURL(kDOMParentURL), outer_params);
-    report->AddDOMDetails(200, GURL(kDOMChildURL), inner_params);
-    std::string serialized = WaitForSerializedReport(
-        report.get(), false /* did_proceed*/, 0 /* num_visit */);
-    ClientSafeBrowsingReportRequest actual;
-    actual.ParseFromString(serialized);
-    VerifyResults(actual, expected);
-  }
-
-  // Try again but with the messages coming in a different order. The IDs change
-  // slightly, but everything else remains the same.
-  {
-    // Adjust the expected IDs: the inner params come first, so InnerScript and
-    // appear before DomParent
-    res_dom_child2->set_id(2);
-    res_dom_child2->set_parent_id(3);
-    res_dom_child->set_id(3);
-    res_dom_child->set_parent_id(4);
-    res_dom_child->clear_child_ids();
-    res_dom_child->add_child_ids(2);
-    res_dom_parent->set_id(4);
-    res_dom_parent->clear_child_ids();
-    res_dom_parent->add_child_ids(3);
-
-    // Also adjust the elements - they change order since InnerDiv and
-    // InnerScript come in first.
-    elem_dom_inner_div->set_id(0);
-    elem_dom_inner_script->set_id(1);
-    elem_dom_inner_script->set_resource_id(res_dom_child2->id());
-
-    elem_dom_outer_div->set_id(2);
-    elem_dom_outer_div->clear_child_ids();
-    elem_dom_outer_div->add_child_ids(3);
-    elem_dom_outer_iframe->set_id(3);
-    elem_dom_outer_iframe->set_resource_id(res_dom_child->id());
-    elem_dom_outer_iframe->clear_child_ids();
-    elem_dom_outer_iframe->add_child_ids(0);
-    elem_dom_outer_iframe->add_child_ids(1);
-
-    scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
-        ui_manager_.get(), web_contents(), resource, NULL, history_service());
-
-    // We call AddDOMDetails directly so we can specify different render frame
-    // IDs.
-    report->AddDOMDetails(200, GURL(kDOMChildURL), inner_params);
-    report->AddDOMDetails(100, GURL(kDOMParentURL), outer_params);
-    std::string serialized = WaitForSerializedReport(
-        report.get(), false /* did_proceed*/, 0 /* num_visit */);
-    ClientSafeBrowsingReportRequest actual;
-    actual.ParseFromString(serialized);
-    VerifyResults(actual, expected);
-  }
-}
+// TEST_F(ThreatDetailsTest, ThreatDOMDetails_MultipleFrames) {
+//  // Define two sets of DOM nodes - one for an outer page containing an
+//  iframe,
+//  // and then another for the inner page containing the contents of that
+//  iframe. std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> outer_params;
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_child_div;
+//  outer_child_div.node_id = 1;
+//  outer_child_div.child_node_ids.push_back(2);
+//  outer_child_div.tag_name = "div";
+//  outer_child_div.parent = GURL(kDOMParentURL);
+//  outer_child_div.attributes.push_back(std::make_pair("id", "outer"));
+//  outer_params.push_back(outer_child_div);
+//
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_child_iframe;
+//  outer_child_iframe.node_id = 2;
+//  outer_child_iframe.parent_node_id = 1;
+//  outer_child_iframe.url = GURL(kDOMChildURL);
+//  outer_child_iframe.tag_name = "iframe";
+//  outer_child_iframe.parent = GURL(kDOMParentURL);
+//  outer_child_iframe.attributes.push_back(std::make_pair("src",
+//  kDOMChildURL));
+//  outer_child_iframe.attributes.push_back(std::make_pair("foo", "bar"));
+//  outer_child_iframe.other_frame_routing_id = main_rfh()->GetRoutingID();

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

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?

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

Done.

+//  outer_params.push_back(outer_child_iframe);
+//
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_summary_node;
+//  outer_summary_node.url = GURL(kDOMParentURL);
+//  outer_summary_node.children.push_back(GURL(kDOMChildURL));
+//  outer_params.push_back(outer_summary_node);
+//
+//  // Now define some more nodes for the body of the iframe.
+//  std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> inner_params;
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_child_div;
+//  inner_child_div.node_id = 3;
+//  inner_child_div.tag_name = "div";
+//  inner_child_div.parent = GURL(kDOMChildURL);
+//  inner_child_div.attributes.push_back(std::make_pair("id", "inner"));
+//  inner_child_div.attributes.push_back(std::make_pair("bar", "baz"));
+//  inner_params.push_back(inner_child_div);
+//
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_child_script;
+//  inner_child_script.node_id = 4;
+//  inner_child_script.url = GURL(kDOMChildUrl2);
+//  inner_child_script.tag_name = "script";
+//  inner_child_script.parent = GURL(kDOMChildURL);
+//  inner_child_script.attributes.push_back(std::make_pair("src",
+//  kDOMChildUrl2)); inner_params.push_back(inner_child_script);
+//
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_summary_node;
+//  inner_summary_node.url = GURL(kDOMChildURL);
+//  inner_summary_node.children.push_back(GURL(kDOMChildUrl2));
+//  inner_params.push_back(inner_summary_node);
+//
+//  ClientSafeBrowsingReportRequest expected;
+//  expected.set_type(ClientSafeBrowsingReportRequest::URL_UNWANTED);
+//  expected.set_url(kThreatURL);
+//  expected.set_page_url(kLandingURL);
+//  expected.set_referrer_url("");
+//  expected.set_did_proceed(false);
+//  expected.set_repeat_visit(false);
+//
+//  ClientSafeBrowsingReportRequest::Resource* pb_resource =
+//      expected.add_resources();
+//  pb_resource->set_id(0);
+//  pb_resource->set_url(kLandingURL);
+//
+//  pb_resource = expected.add_resources();
+//  pb_resource->set_id(1);
+//  pb_resource->set_url(kThreatURL);
+//
+//  ClientSafeBrowsingReportRequest::Resource* res_dom_child =
+//      expected.add_resources();
+//  res_dom_child->set_id(2);
+//  res_dom_child->set_url(kDOMChildURL);
+//  res_dom_child->set_parent_id(3);
+//  res_dom_child->add_child_ids(4);
+//
+//  ClientSafeBrowsingReportRequest::Resource* res_dom_parent =
+//      expected.add_resources();
+//  res_dom_parent->set_id(3);
+//  res_dom_parent->set_url(kDOMParentURL);
+//  res_dom_parent->add_child_ids(2);
+//
+//  ClientSafeBrowsingReportRequest::Resource* res_dom_child2 =
+//      expected.add_resources();
+//  res_dom_child2->set_id(4);
+//  res_dom_child2->set_url(kDOMChildUrl2);
+//  res_dom_child2->set_parent_id(2);
+//
+//  expected.set_complete(false);  // Since the cache was missing.
+//
+//  HTMLElement* elem_dom_outer_div = expected.add_dom();
+//  elem_dom_outer_div->set_id(0);
+//  elem_dom_outer_div->set_tag("DIV");
+//  elem_dom_outer_div->add_attribute()->set_name("id");
+//  elem_dom_outer_div->mutable_attribute(0)->set_value("outer");
+//  elem_dom_outer_div->add_child_ids(1);
+//
+//  HTMLElement* elem_dom_outer_iframe = expected.add_dom();
+//  elem_dom_outer_iframe->set_id(1);
+//  elem_dom_outer_iframe->set_tag("IFRAME");
+//  elem_dom_outer_iframe->set_resource_id(res_dom_child->id());
+//  elem_dom_outer_iframe->add_attribute()->set_name("src");
+//  elem_dom_outer_iframe->mutable_attribute(0)->set_value(kDOMChildURL);
+//  elem_dom_outer_iframe->add_attribute()->set_name("foo");
+//  elem_dom_outer_iframe->mutable_attribute(1)->set_value("bar");
+//  elem_dom_outer_iframe->add_child_ids(2);
+//  elem_dom_outer_iframe->add_child_ids(3);
+//
+//  HTMLElement* elem_dom_inner_div = expected.add_dom();
+//  elem_dom_inner_div->set_id(2);
+//  elem_dom_inner_div->set_tag("DIV");
+//  elem_dom_inner_div->add_attribute()->set_name("id");
+//  elem_dom_inner_div->mutable_attribute(0)->set_value("inner");
+//  elem_dom_inner_div->add_attribute()->set_name("bar");
+//  elem_dom_inner_div->mutable_attribute(1)->set_value("baz");
+//
+//  HTMLElement* elem_dom_inner_script = expected.add_dom();
+//  elem_dom_inner_script->set_id(3);
+//  elem_dom_inner_script->set_tag("SCRIPT");
+//  elem_dom_inner_script->set_resource_id(res_dom_child2->id());
+//  elem_dom_inner_script->add_attribute()->set_name("src");
+//  elem_dom_inner_script->mutable_attribute(0)->set_value(kDOMChildUrl2);
+//
+//  content::WebContentsTester::For(web_contents())
+//      ->NavigateAndCommit(GURL(kLandingURL));
+//
+//  UnsafeResource resource;
+//  InitResource(&resource, SB_THREAT_TYPE_URL_UNWANTED,
+//               true /* is_subresource */, GURL(kThreatURL));
+//
+//  // Send both sets of nodes, from different render frames.
+//  {
+//    scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
+//        ui_manager_.get(), web_contents(), resource, NULL, history_service());
+//
+//    // We call AddDOMDetails directly so we can specify different render frame
+//    // IDs.
+//    // TODO: this is still failing - maybe try by setting
+//    // "other_frame_routing_id" on the node in the IPC?
+//    report->AddDOMDetails(main_rfh()->GetProcess()->GetID(),
+//                          main_rfh()->GetRoutingID(), GURL(kDOMParentURL),
+//                          outer_params);
+//    report->AddDOMDetails(main_rfh()->GetProcess()->GetID(),
+//                          main_rfh()->GetRoutingID(), GURL(kDOMChildURL),
+//                          inner_params);
+//    std::string serialized = WaitForSerializedReport(
+//        report.get(), false /* did_proceed*/, 0 /* num_visit */);
+//    ClientSafeBrowsingReportRequest actual;
+//    actual.ParseFromString(serialized);
+//    VerifyResults(actual, expected);
+//  }
+//
+//  // Try again but with the messages coming in a different order. The IDs
+//  change
+//  // slightly, but everything else remains the same.
+//  {
+//    // Adjust the expected IDs: the inner params come first, so InnerScript
+//    and
+//    // appear before DomParent
+//    res_dom_child2->set_id(2);
+//    res_dom_child2->set_parent_id(3);
+//    res_dom_child->set_id(3);
+//    res_dom_child->set_parent_id(4);
+//    res_dom_child->clear_child_ids();
+//    res_dom_child->add_child_ids(2);
+//    res_dom_parent->set_id(4);
+//    res_dom_parent->clear_child_ids();
+//    res_dom_parent->add_child_ids(3);
+//
+//    // Also adjust the elements - they change order since InnerDiv and
+//    // InnerScript come in first.
+//    elem_dom_inner_div->set_id(0);
+//    elem_dom_inner_script->set_id(1);
+//    elem_dom_inner_script->set_resource_id(res_dom_child2->id());
+//
+//    elem_dom_outer_div->set_id(2);
+//    elem_dom_outer_div->clear_child_ids();
+//    elem_dom_outer_div->add_child_ids(3);
+//    elem_dom_outer_iframe->set_id(3);
+//    elem_dom_outer_iframe->set_resource_id(res_dom_child->id());
+//    elem_dom_outer_iframe->clear_child_ids();
+//    elem_dom_outer_iframe->add_child_ids(0);
+//    elem_dom_outer_iframe->add_child_ids(1);
+//
+//    scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
+//        ui_manager_.get(), web_contents(), resource, NULL, history_service());
+//
+//    // We call AddDOMDetails directly so we can specify different render frame
+//    // IDs.
+//    report->AddDOMDetails(main_rfh()->GetProcess()->GetID(),
+//                          main_rfh()->GetRoutingID(), GURL(kDOMChildURL),
+//                          inner_params);
+//    report->AddDOMDetails(main_rfh()->GetProcess()->GetID(),
+//                          main_rfh()->GetRoutingID(), GURL(kDOMParentURL),
+//                          outer_params);
+//    std::string serialized = WaitForSerializedReport(
+//        report.get(), false /* did_proceed*/, 0 /* num_visit */);
+//    ClientSafeBrowsingReportRequest actual;
+//    actual.ParseFromString(serialized);
+//    VerifyResults(actual, expected);
+//  }
+//}
 
 // Tests an ambiguous DOM, meaning that an inner render frame has  URL that can
 // not be mapped to an iframe element in the parent frame with that same URL.
@@ -741,113 +756,119 @@ TEST_F(ThreatDetailsTest, ThreatDOMDetails_MultipleFrames) {
 // kDOMParentURL
 //   \- <iframe src=kDataURL>
 //        \- <script src=kDOMChildURL2>
-TEST_F(ThreatDetailsTest, ThreatDOMDetails_AmbiguousDOM) {
-  const char kAmbiguousDomMetric[] = "SafeBrowsing.ThreatReport.DomIsAmbiguous";
-
-  // Define two sets of DOM nodes - one for an outer page containing an iframe,
-  // and then another for the inner page containing the contents of that iframe.
-  std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> outer_params;
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_child_node;
-  outer_child_node.url = GURL(kDataURL);
-  outer_child_node.tag_name = "frame";
-  outer_child_node.parent = GURL(kDOMParentURL);
-  outer_child_node.attributes.push_back(std::make_pair("src", kDataURL));
-  outer_params.push_back(outer_child_node);
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_summary_node;
-  outer_summary_node.url = GURL(kDOMParentURL);
-  outer_summary_node.children.push_back(GURL(kDataURL));
-  outer_params.push_back(outer_summary_node);
-
-  // Now define some more nodes for the body of the iframe. The URL of this
-  // inner frame is "about:blank".
-  std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> inner_params;
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_child_node;
-  inner_child_node.url = GURL(kDOMChildUrl2);
-  inner_child_node.tag_name = "script";
-  inner_child_node.parent = GURL(kBlankURL);
-  inner_child_node.attributes.push_back(std::make_pair("src", kDOMChildUrl2));
-  inner_params.push_back(inner_child_node);
-  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_summary_node;
-  inner_summary_node.url = GURL(kBlankURL);
-  inner_summary_node.children.push_back(GURL(kDOMChildUrl2));
-  inner_params.push_back(inner_summary_node);
-
-  ClientSafeBrowsingReportRequest expected;
-  expected.set_type(ClientSafeBrowsingReportRequest::URL_UNWANTED);
-  expected.set_url(kThreatURL);
-  expected.set_page_url(kLandingURL);
-  expected.set_referrer_url("");
-  expected.set_did_proceed(false);
-  expected.set_repeat_visit(false);
-
-  ClientSafeBrowsingReportRequest::Resource* pb_resource =
-      expected.add_resources();
-  pb_resource->set_id(0);
-  pb_resource->set_url(kLandingURL);
-
-  pb_resource = expected.add_resources();
-  pb_resource->set_id(1);
-  pb_resource->set_url(kThreatURL);
-
-  pb_resource = expected.add_resources();
-  pb_resource->set_id(2);
-  pb_resource->set_url(kDOMParentURL);
-  pb_resource->add_child_ids(3);
-
-  // TODO(lpz): The data URL is added, despite being unreportable, because it
-  // is a child of the top-level page. Consider if this should happen.
-  pb_resource = expected.add_resources();
-  pb_resource->set_id(3);
-  pb_resource->set_url(kDataURL);
-
-  // This child can't be mapped to its containing iframe so its parent is unset.
-  pb_resource = expected.add_resources();
-  pb_resource->set_id(4);
-  pb_resource->set_url(kDOMChildUrl2);
-
-  expected.set_complete(false);  // Since the cache was missing.
-
-  // This Element represents the Frame with the data URL. It has no resource or
-  // children since it couldn't be mapped to anything. It does still contain the
-  // src attribute with the data URL set.
-  HTMLElement* pb_element = expected.add_dom();
-  pb_element->set_id(0);
-  pb_element->set_tag("FRAME");
-  pb_element->add_attribute()->set_name("src");
-  pb_element->mutable_attribute(0)->set_value(kDataURL);
-
-  pb_element = expected.add_dom();
-  pb_element->set_id(1);
-  pb_element->set_tag("SCRIPT");
-  pb_element->set_resource_id(4);
-  pb_element->add_attribute()->set_name("src");
-  pb_element->mutable_attribute(0)->set_value(kDOMChildUrl2);
-
-  content::WebContentsTester::For(web_contents())
-      ->NavigateAndCommit(GURL(kLandingURL));
-
-  UnsafeResource resource;
-  InitResource(&resource, SB_THREAT_TYPE_URL_UNWANTED,
-               true /* is_subresource */, GURL(kThreatURL));
-  scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
-      ui_manager_.get(), web_contents(), resource, NULL, history_service());
-  base::HistogramTester histograms;
-
-  // Send both sets of nodes, from different render frames. We call
-  // AddDOMDetails directly so we can specify different render frame IDs.
-  report->AddDOMDetails(100, GURL(kDOMParentURL), outer_params);
-  // The inner frame was using a data URL so its last committed URL is empty.
-  report->AddDOMDetails(200, GURL(), inner_params);
-
-  std::string serialized = WaitForSerializedReport(
-      report.get(), false /* did_proceed*/, 0 /* num_visit */);
-  ClientSafeBrowsingReportRequest actual;
-  actual.ParseFromString(serialized);
-  VerifyResults(actual, expected);
-
-  // This DOM should be ambiguous, expect the UMA metric to be incremented.
-  histograms.ExpectTotalCount(kAmbiguousDomMetric, 1);
-}
+// TEST_F(ThreatDetailsTest, ThreatDOMDetails_AmbiguousDOM) {
+//  //  const char kAmbiguousDomMetric[] =
+//  //  "SafeBrowsing.ThreatReport.DomIsAmbiguous";
+//
+//  // Define two sets of DOM nodes - one for an outer page containing an
+//  iframe,
+//  // and then another for the inner page containing the contents of that
+//  iframe. std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> outer_params;
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_child_node;
+//  outer_child_node.url = GURL(kDataURL);
+//  outer_child_node.tag_name = "frame";

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

Should this one have a child_frame_routing_id as well?

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

Done.

+//  outer_child_node.parent = GURL(kDOMParentURL);
+//  outer_child_node.attributes.push_back(std::make_pair("src", kDataURL));
+//  outer_params.push_back(outer_child_node);
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node outer_summary_node;
+//  outer_summary_node.url = GURL(kDOMParentURL);
+//  outer_summary_node.children.push_back(GURL(kDataURL));
+//  outer_params.push_back(outer_summary_node);
+//
+//  // Now define some more nodes for the body of the iframe. The URL of this
+//  // inner frame is "about:blank".
+//  std::vector<SafeBrowsingHostMsg_ThreatDOMDetails_Node> inner_params;
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_child_node;
+//  inner_child_node.url = GURL(kDOMChildUrl2);
+//  inner_child_node.tag_name = "script";
+//  inner_child_node.parent = GURL(kBlankURL);
+//  inner_child_node.attributes.push_back(std::make_pair("src", kDOMChildUrl2));
+//  inner_params.push_back(inner_child_node);
+//  SafeBrowsingHostMsg_ThreatDOMDetails_Node inner_summary_node;
+//  inner_summary_node.url = GURL(kBlankURL);
+//  inner_summary_node.children.push_back(GURL(kDOMChildUrl2));
+//  inner_params.push_back(inner_summary_node);
+//
+//  ClientSafeBrowsingReportRequest expected;
+//  expected.set_type(ClientSafeBrowsingReportRequest::URL_UNWANTED);
+//  expected.set_url(kThreatURL);
+//  expected.set_page_url(kLandingURL);
+//  expected.set_referrer_url("");
+//  expected.set_did_proceed(false);
+//  expected.set_repeat_visit(false);
+//
+//  ClientSafeBrowsingReportRequest::Resource* pb_resource =
+//      expected.add_resources();
+//  pb_resource->set_id(0);
+//  pb_resource->set_url(kLandingURL);
+//
+//  pb_resource = expected.add_resources();
+//  pb_resource->set_id(1);
+//  pb_resource->set_url(kThreatURL);
+//
+//  pb_resource = expected.add_resources();
+//  pb_resource->set_id(2);
+//  pb_resource->set_url(kDOMParentURL);
+//  pb_resource->add_child_ids(3);
+//
+//  // TODO(lpz): The data URL is added, despite being unreportable, because it
+//  // is a child of the top-level page. Consider if this should happen.
+//  pb_resource = expected.add_resources();
+//  pb_resource->set_id(3);
+//  pb_resource->set_url(kDataURL);
+//
+//  // This child can't be mapped to its containing iframe so its parent is
+//  unset. pb_resource = expected.add_resources(); pb_resource->set_id(4);
+//  pb_resource->set_url(kDOMChildUrl2);
+//
+//  expected.set_complete(false);  // Since the cache was missing.
+//
+//  // This Element represents the Frame with the data URL. It has no resource
+//  or
+//  // children since it couldn't be mapped to anything. It does still contain
+//  the
+//  // src attribute with the data URL set.
+//  HTMLElement* pb_element = expected.add_dom();
+//  pb_element->set_id(0);
+//  pb_element->set_tag("FRAME");
+//  pb_element->add_attribute()->set_name("src");
+//  pb_element->mutable_attribute(0)->set_value(kDataURL);
+//
+//  pb_element = expected.add_dom();
+//  pb_element->set_id(1);
+//  pb_element->set_tag("SCRIPT");
+//  pb_element->set_resource_id(4);
+//  pb_element->add_attribute()->set_name("src");
+//  pb_element->mutable_attribute(0)->set_value(kDOMChildUrl2);
+//
+//  content::WebContentsTester::For(web_contents())
+//      ->NavigateAndCommit(GURL(kLandingURL));
+//
+//  UnsafeResource resource;
+//  InitResource(&resource, SB_THREAT_TYPE_URL_UNWANTED,
+//               true /* is_subresource */, GURL(kThreatURL));
+//  scoped_refptr<ThreatDetailsWrap> report = new ThreatDetailsWrap(
+//      ui_manager_.get(), web_contents(), resource, NULL, history_service());
+//  base::HistogramTester histograms;
+//
+//  // Send both sets of nodes, from different render frames. We call
+//  // AddDOMDetails directly so we can specify different render frame IDs.
+//  report->AddDOMDetails(main_rfh()->GetProcess()->GetID(),
+//                        main_rfh()->GetRoutingID(), GURL(kDOMParentURL),
+//                        outer_params);
+//  // The inner frame was using a data URL so its last committed URL is empty.
+//  report->AddDOMDetails(main_rfh()->GetProcess()->GetID(),
+//                        main_rfh()->GetRoutingID(), GURL(), inner_params);
+//
+//  std::string serialized = WaitForSerializedReport(
+//      report.get(), false /* did_proceed*/, 0 /* num_visit */);
+//  ClientSafeBrowsingReportRequest actual;
+//  actual.ParseFromString(serialized);
+//  VerifyResults(actual, expected);
+//
+//  // This DOM should be ambiguous, expect the UMA metric to be incremented.
+//  //  histograms.ExpectTotalCount(kAmbiguousDomMetric, 1);
+//}
 
 // Tests creating a threat report of a malware page where there are redirect
 // urls to an unsafe resource url.