Grab additional malware details from the renderer: frame hierarchy, list of scripts and embed urls from each frame.

TEST=unit_tests, relevant browser_test
Review URL: http://codereview.chromium.org/6373014

git-svn-id: svn://svn.chromium.org/chrome/trunk/src@75155 0039d316-1c4b-4281-b951-d872f2087c98

6 files changed, 358 insertions, 45 deletions

diff --git a/chrome/browser/safe_browsing/malware_details.cc b/chrome/browser/safe_browsing/malware_details.cc
index d7b113c..5cfa993 100644
--- a/chrome/browser/safe_browsing/malware_details.cc
+++ b/chrome/browser/safe_browsing/malware_details.cc
@@ -6,13 +6,59 @@
 
 #include "chrome/browser/safe_browsing/malware_details.h"
 
+#include "base/lazy_instance.h"
+#include "chrome/browser/browser_thread.h"
+#include "chrome/browser/renderer_host/render_view_host.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/tab_contents/navigation_entry.h"
 #include "chrome/browser/tab_contents/tab_contents.h"
 #include "chrome/browser/safe_browsing/report.pb.h"
+#include "chrome/common/render_messages.h"
+#include "chrome/common/render_messages_params.h"
 
 using safe_browsing::ClientMalwareReportRequest;
 
+// Keep in sync with KMaxNodes in renderer/safe_browsing/malware_dom_details
+static const uint32 kMaxDomNodes = 500;
+
+// static
+MalwareDetailsFactory* MalwareDetails::factory_ = NULL;
+
+// The default MalwareDetailsFactory.  Global, made a singleton so we
+// don't leak it.
+class MalwareDetailsFactoryImpl
+    : public MalwareDetailsFactory {
+ public:
+  MalwareDetails* CreateMalwareDetails(
+      TabContents* tab_contents,
+      const SafeBrowsingService::UnsafeResource& unsafe_resource) {
+    return new MalwareDetails(tab_contents, unsafe_resource);
+  }
+
+ private:
+  friend struct base::DefaultLazyInstanceTraits<
+      MalwareDetailsFactoryImpl>;
+
+  MalwareDetailsFactoryImpl() { }
+
+  DISALLOW_COPY_AND_ASSIGN(MalwareDetailsFactoryImpl);
+};
+
+static base::LazyInstance<MalwareDetailsFactoryImpl>
+    g_malware_details_factory_impl(base::LINKER_INITIALIZED);
+
+// Create a MalwareDetails for the given tab.
+/* static */
+MalwareDetails* MalwareDetails::NewMalwareDetails(
+    TabContents* tab_contents,
+    const SafeBrowsingService::UnsafeResource& resource) {
+  // Set up the factory if this has not been done already (tests do that
+  // before this method is called).
+  if (!factory_)
+    factory_ = g_malware_details_factory_impl.Pointer();
+  return factory_->CreateMalwareDetails(tab_contents, resource);
+}
+
 // Create a MalwareDetails for the given tab. Runs in the UI thread.
 MalwareDetails::MalwareDetails(
     TabContents* tab_contents,
@@ -24,6 +70,16 @@ MalwareDetails::MalwareDetails(
 
 MalwareDetails::~MalwareDetails() {}
 
+bool MalwareDetails::OnMessageReceived(const IPC::Message& message) {
+  bool handled = true;
+  IPC_BEGIN_MESSAGE_MAP(MalwareDetails, message)
+    IPC_MESSAGE_HANDLER(ViewHostMsg_MalwareDOMDetails,
+                        OnReceivedMalwareDOMDetails)
+    IPC_MESSAGE_UNHANDLED(handled = false)
+  IPC_END_MESSAGE_MAP()
+  return handled;
+}
+
 bool MalwareDetails::IsPublicUrl(const GURL& url) const {
   return url.SchemeIs("http");  // TODO(panayiotis): also skip internal urls.
 }
@@ -32,8 +88,8 @@ bool MalwareDetails::IsPublicUrl(const GURL& url) const {
 // updates |resource|. Otherwise, it creates a new message, adds it to
 // resources_ and updates |resource| to point to it.
 ClientMalwareReportRequest::Resource* MalwareDetails::FindOrCreateResource(
-    const std::string& url) {
-  ResourceMap::iterator it = resources_.find(url);
+    const GURL& url) {
+  ResourceMap::iterator it = resources_.find(url.spec());
   if (it != resources_.end()) {
     return it->second.get();
   }
@@ -42,27 +98,40 @@ ClientMalwareReportRequest::Resource* MalwareDetails::FindOrCreateResource(
   int id = resources_.size();
   linked_ptr<ClientMalwareReportRequest::Resource> new_resource(
       new ClientMalwareReportRequest::Resource());
-  new_resource->set_url(url);
+  new_resource->set_url(url.spec());
   new_resource->set_id(id);
-  resources_[url] = new_resource;
+  resources_[url.spec()] = new_resource;
   return new_resource.get();
 }
 
-void MalwareDetails::AddUrl(const std::string& url,
-                             const std::string& parent) {
-  if (!IsPublicUrl(GURL(url)))
+void MalwareDetails::AddUrl(const GURL& url,
+                            const GURL& parent,
+                            const std::string& tagname,
+                            const std::vector<GURL>* children) {
+  if (!IsPublicUrl(url))
     return;
 
   // Find (or create) the resource for the url.
   ClientMalwareReportRequest::Resource* url_resource =
       FindOrCreateResource(url);
-  if (!parent.empty() && IsPublicUrl(GURL(parent))) {
+  if (!tagname.empty()) {
+    url_resource->set_tag_name(tagname);
+  }
+  if (!parent.is_empty() && IsPublicUrl(parent)) {
     // Add the resource for the parent.
     ClientMalwareReportRequest::Resource* parent_resource =
         FindOrCreateResource(parent);
     // Update the parent-child relation
     url_resource->set_parent_id(parent_resource->id());
   }
+  if (children) {
+    for (std::vector<GURL>::const_iterator it = children->begin();
+         it != children->end(); it++) {
+      ClientMalwareReportRequest::Resource* child_resource =
+          FindOrCreateResource(*it);
+      url_resource->add_child_ids(child_resource->id());
+    }
+  }
 }
 
 void MalwareDetails::StartCollection() {
@@ -88,38 +157,74 @@ void MalwareDetails::StartCollection() {
   }
 
   // Add the nodes, starting from the page url.
-  AddUrl(page_url.spec(), "");
+  AddUrl(page_url, GURL(), "", NULL);
 
   // Add the resource_url and its original url, if non-empty and different.
-  if (!resource_.original_url.spec().empty() &&
+  if (!resource_.original_url.is_empty() &&
       resource_.url != resource_.original_url) {
     // Add original_url, as the parent of resource_url.
-    AddUrl(resource_.original_url.spec(), "");
-    AddUrl(resource_.url.spec(), resource_.original_url.spec());
+    AddUrl(resource_.original_url, GURL(), "", NULL);
+    AddUrl(resource_.url, resource_.original_url, "", NULL);
   } else {
-    AddUrl(resource_.url.spec(), "");
+    AddUrl(resource_.url, GURL(), "", NULL);
   }
 
   // Add the redirect urls, if non-empty. The redirect urls do not include the
   // original url, but include the unsafe url which is the last one of the
   // redirect urls chain
-  std::string parent_url = "";
+  GURL parent_url;
   // Set the original url as the parent of the first redirect url if it's not
   // empty.
   if (!resource_.original_url.is_empty()) {
-    parent_url = resource_.original_url.spec();
+    parent_url = resource_.original_url;
   }
   // Set the previous redirect url as the parent of the next one
   for (unsigned int i = 0; i < resource_.redirect_urls.size(); ++i) {
-    AddUrl(resource_.redirect_urls[i].spec(), parent_url);
-    parent_url = resource_.redirect_urls[i].spec();
+    AddUrl(resource_.redirect_urls[i], parent_url, "", NULL);
+    parent_url = resource_.redirect_urls[i];
   }
 
   // Add the referrer url.
-  if (nav_entry && !referrer_url.spec().empty()) {
-    AddUrl(referrer_url.spec(), "");
+  if (nav_entry && !referrer_url.is_empty()) {
+    AddUrl(referrer_url, GURL(), "", NULL);
+  }
+
+  // Get URLs of frames, scripts etc from the DOM.
+  // OnReceivedMalwareDOMDetails will be called when the renderer replies.
+  tab_contents_->render_view_host()->GetMalwareDOMDetails();
+}
+
+// When the renderer is done, this is called.
+void MalwareDetails::OnReceivedMalwareDOMDetails(
+    const ViewHostMsg_MalwareDOMDetails_Params& params) {
+  // Schedule this in IO thread, so it doesn't conflict with future users
+  // of our data structures (eg GetSerializedReport).
+  BrowserThread::PostTask(
+      BrowserThread::IO, FROM_HERE,
+      NewRunnableMethod(
+          this, &MalwareDetails::AddDOMDetails, params));
+}
+
+void MalwareDetails::AddDOMDetails(
+    const ViewHostMsg_MalwareDOMDetails_Params& params) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+  // Add the urls from the DOM to |resources_|.  The renderer could be
+  // sending bogus messages, so limit the number of nodes we accept.
+  DVLOG(1) << "Nodes from the DOM: " << params.nodes.size();
+  for (uint32 i = 0; i < params.nodes.size() && i < kMaxDomNodes; ++i) {
+    ViewHostMsg_MalwareDOMDetails_Node node = params.nodes[i];
+    DVLOG(1) << node.url << ", " << node.tag_name << ", " << node.parent;
+    AddUrl(node.url, node.parent, node.tag_name, &(node.children));
   }
+}
 
+// Called from the SB Service on the IO thread, after the user has
+// closed the tab, or clicked proceed or goback.  Since the user needs
+// to take an action, we expect this to be called after
+// OnReceivedMalwareDOMDetails in most cases. If not, we don't include
+// the DOM data in our report.
+const std::string* MalwareDetails::GetSerializedReport() {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   // The |report_| protocol buffer is now generated: We add all the
   // urls in our |resources_| maps.
   for (ResourceMap::const_iterator it = resources_.begin();
@@ -128,10 +233,7 @@ void MalwareDetails::StartCollection() {
         report_->add_resources();
     pb_resource->CopyFrom(*(it->second));
   }
-}
 
-// Called from the SB Service on the IO thread.
-const std::string* MalwareDetails::GetSerializedReport() {
   scoped_ptr<std::string> request_data(new std::string());
   if (!report_->SerializeToString(request_data.get())) {
     DLOG(ERROR) << "Unable to serialize the malware report.";
diff --git a/chrome/browser/safe_browsing/malware_details.h b/chrome/browser/safe_browsing/malware_details.h
index bbe63e1..94944f0 100644
--- a/chrome/browser/safe_browsing/malware_details.h
+++ b/chrome/browser/safe_browsing/malware_details.h
@@ -21,18 +21,46 @@
 #include "base/scoped_ptr.h"
 #include "chrome/browser/safe_browsing/report.pb.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
+#include "chrome/browser/tab_contents/tab_contents_observer.h"
 
 class TabContents;
+struct ViewHostMsg_MalwareDOMDetails_Params;
 
-class MalwareDetails : public base::RefCountedThreadSafe<MalwareDetails> {
+class MalwareDetailsFactory;
+
+class MalwareDetails : public base::RefCountedThreadSafe<MalwareDetails>,
+                       public TabContentsObserver {
  public:
-  MalwareDetails(TabContents* tab_contents,
-                 const SafeBrowsingService::UnsafeResource resource);
+  // Constructs a new MalwareDetails instance, using the factory.
+  static MalwareDetails* NewMalwareDetails(
+      TabContents* tab_contents,
+      const SafeBrowsingService::UnsafeResource& resource);
+
+  // Makes the passed |factory| the factory used to instanciate
+  // SafeBrowsingBlockingPage objects. Useful for tests.
+  static void RegisterFactory(MalwareDetailsFactory* factory) {
+    factory_ = factory;
+  }
 
   // The SafeBrowsingService calls this from the IO thread, to get the
   // serialized report as a string and send it over.
   const std::string* GetSerializedReport();
 
+  // TabContentsObserver implementation.
+  virtual bool OnMessageReceived(const IPC::Message& message);
+
+ protected:
+  friend class MalwareDetailsFactoryImpl;
+
+  MalwareDetails(TabContents* tab_contents,
+                 const SafeBrowsingService::UnsafeResource resource);
+
+  // Called on the IO thread with the DOM details.
+  virtual void AddDOMDetails(
+      const ViewHostMsg_MalwareDOMDetails_Params& params);
+
+  virtual ~MalwareDetails();
+
  private:
   friend class base::RefCountedThreadSafe<MalwareDetails>;
 
@@ -52,13 +80,18 @@ class MalwareDetails : public base::RefCountedThreadSafe<MalwareDetails> {
   // one if not found, and adds it to |resources_|. Returns the
   // found/created resource.
   safe_browsing::ClientMalwareReportRequest::Resource* FindOrCreateResource(
-      const std::string& url);
+      const GURL& url);
 
-  // Adds a Resource to resources_ with the given parent
-  // relationship. |parent| can be empty.
-  void AddUrl(const std::string& url, const std::string& parent);
+  // Adds a Resource to resources_ with the given parent-child
+  // relationship. |parent| and |tagname| can be empty, |children| can be NULL.
+  void AddUrl(const GURL& url,
+              const GURL& parent,
+              const std::string& tagname,
+              const std::vector<GURL>* children);
 
-  ~MalwareDetails();
+  // Message handler.
+  void OnReceivedMalwareDOMDetails(
+      const ViewHostMsg_MalwareDOMDetails_Params& params);
 
   TabContents* tab_contents_;
   const SafeBrowsingService::UnsafeResource resource_;
@@ -70,7 +103,24 @@ class MalwareDetails : public base::RefCountedThreadSafe<MalwareDetails> {
   // The report protocol buffer.
   scoped_ptr<safe_browsing::ClientMalwareReportRequest> report_;
 
+  // The factory used to instanciate SafeBrowsingBlockingPage objects.
+  // Usefull for tests, so they can provide their own implementation of
+  // SafeBrowsingBlockingPage.
+  static MalwareDetailsFactory* factory_;
+
+  FRIEND_TEST_ALL_PREFIXES(MalwareDetailsTest, MalwareDOMDetails);
+
   DISALLOW_COPY_AND_ASSIGN(MalwareDetails);
 };
 
+// Factory for creating MalwareDetails.  Useful for tests.
+class MalwareDetailsFactory {
+ public:
+  virtual ~MalwareDetailsFactory() { }
+
+  virtual MalwareDetails* CreateMalwareDetails(
+      TabContents* tab_contents,
+      const SafeBrowsingService::UnsafeResource& unsafe_resource) = 0;
+};
+
 #endif  // CHROME_BROWSER_SAFE_BROWSING_MALWARE_DETAILS_H_
diff --git a/chrome/browser/safe_browsing/malware_details_unittest.cc b/chrome/browser/safe_browsing/malware_details_unittest.cc
index d5ba537..2976383 100644
--- a/chrome/browser/safe_browsing/malware_details_unittest.cc
+++ b/chrome/browser/safe_browsing/malware_details_unittest.cc
@@ -17,6 +17,8 @@ static const char* kOriginalLandingURL = "http://www.originallandingpage.com/";
 static const char* kLandingURL = "http://www.landingpage.com/";
 static const char* kMalwareURL = "http://www.malware.com/";
 static const char* kHttpsURL = "https://www.url.com/";
+static const char* kDOMChildURL = "http://www.domparent.com/";
+static const char* kDOMParentURL = "http://www.domchild.com/";
 static const char* kFirstRedirectURL = "http://redirectone.com/";
 static const char* kSecondRedirectURL = "http://redirecttwo.com/";
 
@@ -81,6 +83,10 @@ class MalwareDetailsTest : public RenderViewHostTestHarness {
       EXPECT_EQ(expected[i]->id(), resources[i]->id());
       EXPECT_EQ(expected[i]->url(), resources[i]->url());
       EXPECT_EQ(expected[i]->parent_id(), resources[i]->parent_id());
+      ASSERT_EQ(expected[i]->child_ids_size(), resources[i]->child_ids_size());
+      for (int j = 0; j < expected[i]->child_ids_size(); j++) {
+        EXPECT_EQ(expected[i]->child_ids(j), resources[i]->child_ids(j));
+      }
     }
   }
 
@@ -96,7 +102,7 @@ TEST_F(MalwareDetailsTest, MalwareSubResource) {
   SafeBrowsingService::UnsafeResource resource;
   InitResource(&resource, ResourceType::SUB_RESOURCE, GURL(kMalwareURL));
 
-  scoped_refptr<MalwareDetails> report = new MalwareDetails(
+  scoped_refptr<MalwareDetails> report = MalwareDetails::NewMalwareDetails(
       contents(), resource);
 
   scoped_ptr<const std::string> serialized(report->GetSerializedReport());
@@ -127,7 +133,7 @@ TEST_F(MalwareDetailsTest, MalwareSubResourceWithOriginalUrl) {
   InitResource(&resource, ResourceType::SUB_RESOURCE, GURL(kMalwareURL));
   resource.original_url = GURL(kOriginalLandingURL);
 
-  scoped_refptr<MalwareDetails> report = new MalwareDetails(
+  scoped_refptr<MalwareDetails> report = MalwareDetails::NewMalwareDetails(
       contents(), resource);
 
   scoped_ptr<const std::string> serialized(report->GetSerializedReport());
@@ -157,12 +163,67 @@ TEST_F(MalwareDetailsTest, MalwareSubResourceWithOriginalUrl) {
   VerifyResults(actual, expected);
 }
 
+// Tests creating a malware report with data from the renderer.
+TEST_F(MalwareDetailsTest, MalwareDOMDetails) {
+  controller().LoadURL(GURL(kLandingURL), GURL(), PageTransition::TYPED);
+
+  SafeBrowsingService::UnsafeResource resource;
+  InitResource(&resource, ResourceType::SUB_RESOURCE, GURL(kMalwareURL));
+
+  scoped_refptr<MalwareDetails> report = MalwareDetails::NewMalwareDetails(
+      contents(), resource);
+
+  // Send a message from the DOM, with 2 nodes, a parent and a child.
+  ViewHostMsg_MalwareDOMDetails_Params params;
+  ViewHostMsg_MalwareDOMDetails_Node child_node;
+  child_node.url = GURL(kDOMChildURL);
+  child_node.tag_name = "iframe";
+  child_node.parent = GURL(kDOMParentURL);
+  params.nodes.push_back(child_node);
+  ViewHostMsg_MalwareDOMDetails_Node parent_node;
+  parent_node.url = GURL(kDOMParentURL);
+  parent_node.children.push_back(GURL(kDOMChildURL));
+  params.nodes.push_back(parent_node);
+  report->OnReceivedMalwareDOMDetails(params);
+
+  MessageLoop::current()->RunAllPending();
+
+  scoped_ptr<const std::string> serialized(report->GetSerializedReport());
+  ClientMalwareReportRequest actual;
+  actual.ParseFromString(*serialized);
+
+  ClientMalwareReportRequest expected;
+  expected.set_malware_url(kMalwareURL);
+  expected.set_page_url(kLandingURL);
+  expected.set_referrer_url("");
+
+  ClientMalwareReportRequest::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(kMalwareURL);
+
+  pb_resource = expected.add_resources();
+  pb_resource->set_id(2);
+  pb_resource->set_url(kDOMChildURL);
+  pb_resource->set_parent_id(3);
+
+  pb_resource = expected.add_resources();
+  pb_resource->set_id(3);
+  pb_resource->set_url(kDOMParentURL);
+  pb_resource->add_child_ids(2);
+
+  VerifyResults(actual, expected);
+}
+
 // Verify that https:// urls are dropped.
 TEST_F(MalwareDetailsTest, NotPublicUrl) {
   controller().LoadURL(GURL(kHttpsURL), GURL(), PageTransition::TYPED);
   SafeBrowsingService::UnsafeResource resource;
   InitResource(&resource, ResourceType::SUB_RESOURCE, GURL(kMalwareURL));
-  scoped_refptr<MalwareDetails> report = new MalwareDetails(
+  scoped_refptr<MalwareDetails> report = MalwareDetails::NewMalwareDetails(
       contents(), resource);
 
   scoped_ptr<const std::string> serialized(report->GetSerializedReport());
@@ -193,7 +254,7 @@ TEST_F(MalwareDetailsTest, MalwareWithRedirectUrl) {
   resource.redirect_urls.push_back(GURL(kSecondRedirectURL));
   resource.redirect_urls.push_back(GURL(kMalwareURL));
 
-  scoped_refptr<MalwareDetails> report = new MalwareDetails(
+  scoped_refptr<MalwareDetails> report = MalwareDetails::NewMalwareDetails(
       contents(), resource);
 
   scoped_ptr<const std::string> serialized(report->GetSerializedReport());
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
index 6f2f945..2f9210c 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page.cc
@@ -147,7 +147,9 @@ SafeBrowsingBlockingPage::SafeBrowsingBlockingPage(
       unsafe_resources[0].threat_type == SafeBrowsingService::URL_MALWARE &&
       malware_details_ == NULL &&
       CanShowMalwareDetailsOption()) {
-    malware_details_ = new MalwareDetails(tab(), unsafe_resources[0]);
+    malware_details_ = MalwareDetails::NewMalwareDetails(
+        tab(), unsafe_resources[0]);
+    tab_contents->AddObserver(malware_details_);
   }
 }
 
@@ -599,6 +601,10 @@ void SafeBrowsingBlockingPage::FinishMalwareDetails() {
   if (malware_details_ == NULL)
     return;  // Not all interstitials have malware details (eg phishing).
 
+  // Stop observing IPC messages from the tab. A subsequent warning page
+  // on the same tab will listen instead.
+  tab()->RemoveObserver(malware_details_);
+
   const PrefService::Preference* pref =
       tab()->profile()->GetPrefs()->FindPreference(
           prefs::kSafeBrowsingReportingEnabled);
diff --git a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
index 0138224..ecab767 100644
--- a/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc
@@ -78,7 +78,6 @@ class FakeSafeBrowsingService :  public SafeBrowsingService {
 
   std::list<scoped_refptr<MalwareDetails> > details_;
 
-
  private:
   base::hash_map<std::string, UrlCheckResult> badurls;
 };
@@ -94,6 +93,77 @@ class TestSafeBrowsingServiceFactory : public SafeBrowsingServiceFactory {
   }
 };
 
+// A MalwareDetails class lets us intercept calls from the renderer.
+class FakeMalwareDetails : public MalwareDetails {
+ public:
+  FakeMalwareDetails(TabContents* tab_contents,
+                     const SafeBrowsingService::UnsafeResource& unsafe_resource)
+      : MalwareDetails(tab_contents, unsafe_resource) { }
+
+  virtual ~FakeMalwareDetails() {}
+
+  virtual void AddDOMDetails(
+      const ViewHostMsg_MalwareDOMDetails_Params& params) {
+    EXPECT_TRUE(BrowserThread::CurrentlyOn(BrowserThread::IO));
+    MalwareDetails::AddDOMDetails(params);
+
+    // Notify the UI thread that we got the dom details.
+    BrowserThread::PostTask(BrowserThread::UI, FROM_HERE,
+                            NewRunnableMethod(this,
+                            &FakeMalwareDetails::OnDOMDetailsDone));
+  }
+
+  void OnDOMDetailsDone() {
+    got_dom_ = true;
+    if (waiting_) {
+      MessageLoopForUI::current()->Quit();
+    }
+  }
+
+  bool got_dom() const {
+    return got_dom_;
+  }
+
+  bool waiting() const {
+    return waiting_;
+  }
+
+  void set_got_dom(bool got_dom) {
+    got_dom_ = got_dom;
+  }
+
+  void set_waiting(bool waiting) {
+    waiting_ = waiting;
+  }
+
+ private:
+  // Some logic to figure out if we should wait for the dom details or not.
+  // These variables should only be accessed in the UI thread.
+  bool got_dom_;
+  bool waiting_;
+
+};
+
+class TestMalwareDetailsFactory : public MalwareDetailsFactory {
+ public:
+  TestMalwareDetailsFactory() { }
+  virtual ~TestMalwareDetailsFactory() { }
+
+  virtual MalwareDetails* CreateMalwareDetails(
+      TabContents* tab_contents,
+      const SafeBrowsingService::UnsafeResource& unsafe_resource) {
+    details_ = new FakeMalwareDetails(tab_contents, unsafe_resource);
+    return details_;
+  }
+
+  FakeMalwareDetails* get_details() {
+    return details_;
+  }
+
+ private:
+  FakeMalwareDetails* details_;
+};
+
 // Tests the safe browsing blocking page in a browser.
 class SafeBrowsingBlockingPageTest : public InProcessBrowserTest,
                                      public SafeBrowsingService::Client {
@@ -102,13 +172,15 @@ class SafeBrowsingBlockingPageTest : public InProcessBrowserTest,
   }
 
   virtual void SetUp() {
-    SafeBrowsingService::RegisterFactory(&factory);
+    SafeBrowsingService::RegisterFactory(&factory_);
+    MalwareDetails::RegisterFactory(&details_factory_);
     InProcessBrowserTest::SetUp();
   }
 
   virtual void TearDown() {
     InProcessBrowserTest::TearDown();
     SafeBrowsingService::RegisterFactory(NULL);
+    MalwareDetails::RegisterFactory(NULL);
   }
 
   virtual void SetUpInProcessBrowserTestFixture() {
@@ -136,30 +208,39 @@ class SafeBrowsingBlockingPageTest : public InProcessBrowserTest,
 
   void SendCommand(const std::string& command) {
     TabContents* contents = browser()->GetSelectedTabContents();
+    // We use InterstitialPage::GetInterstitialPage(tab) instead of
+    // tab->interstitial_page() because the tab doesn't have a pointer
+    // to its interstital page until it gets a command from the renderer
+    // that it has indeed displayed it -- and this sometimes happens after
+    // NavigateToURL returns.
     SafeBrowsingBlockingPage* interstitial_page =
         static_cast<SafeBrowsingBlockingPage*>(
-            contents->interstitial_page());
+            InterstitialPage::GetInterstitialPage(contents));
     ASSERT_TRUE(interstitial_page);
     interstitial_page->CommandReceived(command);
   }
 
   void DontProceedThroughInterstitial() {
     TabContents* contents = browser()->GetSelectedTabContents();
-    InterstitialPage* interstitial_page = contents->interstitial_page();
+    InterstitialPage* interstitial_page = InterstitialPage::GetInterstitialPage(
+        contents);
     ASSERT_TRUE(interstitial_page);
     interstitial_page->DontProceed();
   }
 
   void ProceedThroughInterstitial() {
     TabContents* contents = browser()->GetSelectedTabContents();
-    InterstitialPage* interstitial_page = contents->interstitial_page();
+    InterstitialPage* interstitial_page = InterstitialPage::GetInterstitialPage(
+        contents);
     ASSERT_TRUE(interstitial_page);
     interstitial_page->Proceed();
   }
 
   void AssertNoInterstitial() {
+    // ui_test_utils::RunAllPendingInMessageLoop();
     TabContents* contents = browser()->GetSelectedTabContents();
-    InterstitialPage* interstitial_page = contents->interstitial_page();
+    InterstitialPage* interstitial_page = InterstitialPage::GetInterstitialPage(
+        contents);
     ASSERT_FALSE(interstitial_page);
   }
 
@@ -180,8 +261,11 @@ class SafeBrowsingBlockingPageTest : public InProcessBrowserTest,
     ASSERT_EQ(1u, service->GetDetails()->size());
   }
 
+ protected:
+  TestMalwareDetailsFactory details_factory_;
+
  private:
-  TestSafeBrowsingServiceFactory factory;
+  TestSafeBrowsingServiceFactory factory_;
 
   DISALLOW_COPY_AND_ASSIGN(SafeBrowsingBlockingPageTest);
 };
@@ -307,6 +391,17 @@ IN_PROC_BROWSER_TEST_F(SafeBrowsingBlockingPageTest,
 
   ui_test_utils::NavigateToURL(browser(), url);
 
+  // If the DOM details from renderer did not already return, wait for them.
+  if (!details_factory_.get_details()->got_dom()) {
+    // This condition might not trigger normally, but if you add a
+    // sleep(1) in malware_dom_details it triggers :).
+    details_factory_.get_details()->set_waiting(true);
+    LOG(INFO) << "Waiting for dom details.";
+    ui_test_utils::RunMessageLoop();
+  } else {
+    LOG(INFO) << "Already got the dom details.";
+  }
+
   SendCommand("\"doReport\"");  // Simulate the user checking the checkbox.
   EXPECT_TRUE(browser()->GetProfile()->GetPrefs()->GetBoolean(
       prefs::kSafeBrowsingReportingEnabled));
diff --git a/chrome/browser/safe_browsing/safe_browsing_service.cc b/chrome/browser/safe_browsing/safe_browsing_service.cc
index e1e1efd..cc18867 100644
--- a/chrome/browser/safe_browsing/safe_browsing_service.cc
+++ b/chrome/browser/safe_browsing/safe_browsing_service.cc
@@ -1094,9 +1094,8 @@ void SafeBrowsingService::ReportSafeBrowsingHit(
                                            threat_type);
 }
 
-// A MalwareDetails report is sent after the blocking page is going
-// away, at which point we see if the user had opted-in using the
-// checkbox on the blocking page.
+// If the user had opted-in to send MalwareDetails, this gets called
+// at the time that the blocking page is going away.
 void SafeBrowsingService::ReportMalwareDetails(
     scoped_refptr<MalwareDetails> details) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));