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 43dd6ece4ffa9838c37835fdbea1597e6e8b1404..7579b6b0fa7b38fe0bf211ec01eadabed116ce05 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,6 +6,8 @@
 
 #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/navigation_handle.h"
@@ -51,6 +53,7 @@ class WebContentsEntry : public content::WebContentsObserver {
   void RenderFrameDeleted(RenderFrameHost* render_frame_host) override;
   void RenderFrameHostChanged(RenderFrameHost* old_host,
                               RenderFrameHost* new_host) override;
+  void RenderFrameCreated(RenderFrameHost*) override;
   void RenderViewReady() override;
   void WebContentsDestroyed() override;
   void RenderProcessGone(base::TerminationStatus status) override;
@@ -59,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
@@ -87,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);
 };
 
@@ -96,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);
@@ -146,6 +153,22 @@ 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.
+  render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(base::Bind(

[ncarter (slow), 2017/05/12 21:56:02]

When we show the task manager, this means we will post 1 task per RFH,
browser-wide. That seems like a lot of tasks, especially considering most RFH's
are going to be same-process with their parent.

[Łukasz Anforowicz, 2017/05/16 16:37:25]

In the most recent patchset, I am returning early for:

- non-subframes (which will always be processed in RenderFrameHostChanged;  I've
verified that reloading a killed main frame correctly re-shows the entry in task
manager)

- subframes that are in the same instance as their parent

Do you think that should help sufficiently?

+      &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)

[ncarter (slow), 2017/05/12 21:56:02]

This would need to be:

render_frame_host && render_frame_host->IsRenderFrameLive() &&
 /* something to rule out this being a pending delete host */

[Łukasz Anforowicz, 2017/05/16 16:37:25]

RE: IsRenderFrameLive

Wouldn't this be redundant with |!site_instance->GetProcess()->HasConnection()|
in WebContentsEntry::CreateTaskForFrame?


RE: pending delete

Is this really necessary, if we delete the task manager's entry upon
RenderFrameDeleted?  I think it is okay if there are temporarily 2 entries for
the given frame during a swap-out.

+    CreateTaskForFrame(render_frame_host);
+}
+
 void WebContentsEntry::RenderViewReady() {
   ClearAllTasks(true);
   CreateAllTasks();
@@ -213,7 +236,8 @@ void WebContentsEntry::TitleWasSet(content::NavigationEntry* entry,
 }
 
 void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* render_frame_host) {

[ncarter (slow), 2017/05/12 21:56:02]

This function is implemented assuming that |render_frame_host|'s ancestors have
already been processed (see the subsequent comments for some subtle implications
of that.

So I'm wondering if we can preserve that invariant by making the enumeration
work like this:

 - Remove the ForEachFrame() in CreateAllTasks, and make CreateTaskForFrame be
responsible for adding children (most of the time, |render_frame_host| will not
have children at this point).

 - At the bottom of CreateTaskForFrame, recursively call CreateTaskForFrame on
all children.  (might require a content/public change, but
RenderFrameHost::GetChildren() or ForEachChild() would not be unwelcome, IMO).

 - At the top of CreateTaskForFrame, do the following "filter" and "defer"
operations:

if (!render_frame_host_->IsRenderFrameLive())
  return;  // Sad tab; ignore.

if (render_frame_host_->GetParent() not in tasks_by_frames_) {
  return;  // do nothing, we're waiting on an ancestor.

if (!render_frame_host_->GetProcess()->IsReady()) {
  render_frame_host->GetProcess()->PostTaskWhenProcessIsReady();
  pending_frames_.push_back(render_frame_host);  // maybe
  return;  // Defer this subtree until we get a PID.
}

 - Figure out what, if anything, |pending_frames_| above ought to be.
It could potentially be something like a map<RenderFrameHost,
WeakPtrFactory<RenderFrameHost>>, if we wanted to support cancelling all
outstanding callbacks; but a manually curated set would probably work well too.
My gut sense is that we need something to support cancellation when RFH's leave
the current slot in their FTN -- especially the ClearTaskForFrame(old_host) that
happens in RenderFrameHostChanged.

[Łukasz Anforowicz, 2017/05/16 16:37:25]

I am not sure if this is still needed?

-  DCHECK(!tasks_by_frames_.count(render_frame_host));
+  if (base::ContainsKey(tasks_by_frames_, render_frame_host))
+    ClearTaskForFrame(render_frame_host);

[ncarter (slow), 2017/05/12 21:56:02]

When does this happen?

If |render_frame_host| is the main frame, this cleanup is unsafe, because
existing subframe tasks hold onto a reference to the task of the main frame
https://cs.chromium.org/chromium/src/chrome/browser/task_manager/providers/we...

In the edit history of the task manager, we've generally tried to avoid the
"deleting and re-add, just to be sure" pattern, since it turns out to not
usually be a total no-op. Making this change causes the window to repaint, and
can cause UI state (like the row selection) to be lost, and it regenerates the
ID, which can cause rows to jump around in the order.

So, I'd prefer to keep these DCHECKs if possible.

[Łukasz Anforowicz, 2017/05/16 16:37:25]

This would happen when CreateTaskForFrame would be called from both
RenderFrameReady and RenderFrameHostChanged.  See also
https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man...
and
https://codereview.chromium.org/2857263003/diff/20001/chrome/browser/task_man....

In the most recent patchset, I've moved the ContainsKey check to
RenderFrameHostChanged - this way I can restore the DCHECK here.
Makes sense.

 
   content::SiteInstance* site_instance = render_frame_host->GetSiteInstance();
   if (!site_instance->GetProcess()->HasConnection())

[ncarter (slow), 2017/05/12 21:56:02]

Should probably be IsReady()

[Łukasz Anforowicz, 2017/05/16 16:37:25]

Changing this makes PrintPreviewDialogControllerBrowserTest.TaskManagementTest
fail, by failing to create tasks for chrome://print contents - in this case I
see the following in RenderProcessHostImpl:

IsReady -> returning false;
HasConnection() -> returning true;
channel_connected_ = 0;
GetHandle() = 0;
is_initialized_ = 1;
is_dead_ = 0