Task Manager should listen to WebContentsObserver::RenderFrameCreated.

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

 // Copyright 2015 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/task_manager/providers/web_contents/web_contents_task_provider.h"
 
 #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"
 
 using content::RenderFrameHost;
 using content::RenderWidgetHost;
 using content::SiteInstance;
 using content::WebContents;
 
 namespace task_manager {
@@ skipping 19 matching lines @@
   void ClearAllTasks(bool notify_observer);
 
   // Returns the |RendererTask| that corresponds to the given
   // |render_frame_host| or |nullptr| if the given frame is not tracked by this
   // entry.
   RendererTask* GetTaskForFrame(RenderFrameHost* render_frame_host) const;
 
   // 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;
   void OnRendererUnresponsive(RenderWidgetHost* render_widget_host) override;
   void DidFinishNavigation(
       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
   // provider's observer of the new task.
   void CreateTaskForFrame(RenderFrameHost* render_frame_host);
 
   // Clears the task that corresponds to the given |render_frame_host| and
   // notifies the provider's observer of the tasks removal.
   void ClearTaskForFrame(RenderFrameHost* render_frame_host);
 
   // Calls |on_task| for each task managed by this WebContentsEntry.
   void ForEachTask(const base::Callback<void(RendererTask*)>& on_task);
 
   // The provider that owns this entry.
   WebContentsTaskProvider* provider_;
 
   // The RenderFrameHosts associated with this entry's WebContents that we're
   // tracking mapped by their SiteInstances.
   using FramesList = std::vector<RenderFrameHost*>;
   std::map<SiteInstance*, FramesList> frames_by_site_instance_;
 
   // The RendererTasks that we create for the task manager, mapped by their
   // RenderFrameHosts. This owns the RenderTasks.
   std::map<RenderFrameHost*, RendererTask*> tasks_by_frames_;
 
   // 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);
 };
 
 ////////////////////////////////////////////////////////////////////////////////
 
 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);
 }
 
 void WebContentsEntry::CreateAllTasks() {
   DCHECK(web_contents()->GetMainFrame());
   web_contents()->ForEachFrame(base::Bind(&WebContentsEntry::CreateTaskForFrame,
                                           base::Unretained(this)));
 }
@@ skipping 28 matching lines @@
 void WebContentsEntry::RenderFrameDeleted(RenderFrameHost* render_frame_host) {
   ClearTaskForFrame(render_frame_host);
 }
 
 void WebContentsEntry::RenderFrameHostChanged(RenderFrameHost* old_host,
                                               RenderFrameHost* new_host) {
   ClearTaskForFrame(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();
 }
 
 void WebContentsEntry::WebContentsDestroyed() {
   ClearAllTasks(true);
   provider_->DeleteEntry(web_contents());
 }
 
@@ skipping 38 matching lines @@
 
 void WebContentsEntry::TitleWasSet(content::NavigationEntry* entry,
                                    bool explicit_set) {
   ForEachTask(base::Bind([](RendererTask* task) {
     task->UpdateTitle();
     task->UpdateFavicon();
   }));
 }
 
 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())
     return;
 
   bool site_instance_exists =
       frames_by_site_instance_.count(site_instance) != 0;
   bool is_main_frame = (render_frame_host == web_contents()->GetMainFrame());
   bool site_instance_is_main = (site_instance == main_frame_site_instance_);
 
@@ skipping 172 matching lines @@
 }
 
 void WebContentsTaskProvider::DeleteEntry(content::WebContents* web_contents) {
   // This erase() will delete the WebContentsEntry, which is actually our
   // caller, but it's expecting us to delete it.
   bool success = entries_map_.erase(web_contents) != 0;
   DCHECK(success);
 }
 
 }  // namespace task_manager