Task Manager should listen to WebContentsObserver::RenderFrameCreated.

chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc

Index: chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
diff --git a/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc b/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
index 98284767b575d8e5a8d0a1d58f220a3fe55178e6..9b57f0ba7b9babf9016504c7aba2701b03113e1e 100644
--- a/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
+++ b/chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.cc
@@ -6,10 +6,13 @@
 
 #include "base/bind.h"
 #include "base/macros.h"
+#include "base/memory/weak_ptr.h"
+#include "base/stl_util.h"
 #include "chrome/browser/task_manager/providers/web_contents/subframe_task.h"
 #include "chrome/browser/task_manager/providers/web_contents/web_contents_tags_manager.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.h"
+#include "content/public/browser/render_process_host_utils.h"
 #include "content/public/browser/render_widget_host.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
@@ -50,6 +53,7 @@ class WebContentsEntry : public content::WebContentsObserver {
   void RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
   void RenderFrameHostChanged(RenderFrameHost* old_host,
                               RenderFrameHost* new_host) override;

[Łukasz Anforowicz, 2017/05/05 22:01:05]

Initially I've thought that I can remove RenderFrameHostChanged (thinking that
RenderFrameCreated would always cover the |new_host| from
RenderFrameHostChanged), but that turned out to not be true as shown by failures
in
TaskManagerBrowserTest.NoticeHostedAppTabChanges and/or
TaskManagerOOPIFBrowserTest.NavigateToSiteWithSubframeToOriginalSite.

+  void RenderFrameCreated(RenderFrameHost*) override;
   void RenderViewReady() override;
   void WebContentsDestroyed() override;
   void RenderProcessGone(base::TerminationStatus status) override;
@@ -58,6 +62,8 @@ class WebContentsEntry : public content::WebContentsObserver {
       content::NavigationHandle* navigation_handle) override;
   void TitleWasSet(content::NavigationEntry* entry, bool explicit_set) override;
 
+  void RenderFrameReady(int process_routing_id, int frame_routing_id);
+
  private:
   // Defines a callback for WebContents::ForEachFrame() to create a
   // corresponding task for the given |render_frame_host| and notifying the
@@ -86,6 +92,8 @@ class WebContentsEntry : public content::WebContentsObserver {
   // States whether we did record a main frame for this entry.
   SiteInstance* main_frame_site_instance_;
 
+  base::WeakPtrFactory<WebContentsEntry> weak_factory_;
+
   DISALLOW_COPY_AND_ASSIGN(WebContentsEntry);
 };
 
@@ -95,8 +103,8 @@ WebContentsEntry::WebContentsEntry(content::WebContents* web_contents,
                                    WebContentsTaskProvider* provider)
     : WebContentsObserver(web_contents),
       provider_(provider),
-      main_frame_site_instance_(nullptr) {
-}
+      main_frame_site_instance_(nullptr),
+      weak_factory_(this) {}
 
 WebContentsEntry::~WebContentsEntry() {
   ClearAllTasks(false);
@@ -145,6 +153,24 @@ void WebContentsEntry::RenderFrameHostChanged(RenderFrameHost* old_host,
   CreateTaskForFrame(new_host);
 }
 
+void WebContentsEntry::RenderFrameCreated(RenderFrameHost* render_frame_host) {
+  // Postpone processing |render_frame_host| until its process has a PID.
+  PostTaskWhenRenderProcessHostIsReady(
+      render_frame_host->GetProcess(),
+      base::Bind(&WebContentsEntry::RenderFrameReady,
+                 weak_factory_.GetWeakPtr(),
+                 render_frame_host->GetProcess()->GetID(),
+                 render_frame_host->GetRoutingID()));
+}
+
+void WebContentsEntry::RenderFrameReady(int render_process_id,
+                                        int render_frame_id) {
+  content::RenderFrameHost* render_frame_host =
+      content::RenderFrameHost::FromID(render_process_id, render_frame_id);
+  if (render_frame_host)
+    CreateTaskForFrame(render_frame_host);
+}
+
 void WebContentsEntry::RenderViewReady() {
   ClearAllTasks(true);
   CreateAllTasks();
@@ -203,7 +229,8 @@ void WebContentsEntry::TitleWasSet(content::NavigationEntry* entry,
 }
 
 void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* render_frame_host) {
-  DCHECK(!tasks_by_frames_.count(render_frame_host));

[Łukasz Anforowicz, 2017/05/05 22:01:05]

This DCHECK will be violated, because (as I described in another comment) we
have to have both RenderFrameCreated and RenderFrameHostChanged (and so - a task
for the same RFH will be created twice - once from each of WCO callbacks).

Interestingly, if there is an existing entry, it sometimes is incorrect, so we
need to clear the old entry and recreate it.  Otherwise we get failures in
TaskManagerBrowserTest.NoticeHostedAppTabChanges and/or
TaskManagerOOPIFBrowserTest.NavigateToSiteWithSubframeToOriginalSite.  To be
honest, I don't really understand why the tasks might be stale and need to be
recreated and somewhat blindly tried a few things until all the tests started
passing :-/

+  if (base::ContainsKey(tasks_by_frames_, render_frame_host))
+    ClearTaskForFrame(render_frame_host);
 
   content::SiteInstance* site_instance = render_frame_host->GetSiteInstance();
   if (!site_instance->GetProcess()->HasConnection())