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/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/render_process_host.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;
@@ skipping 25 matching lines @@
 
   // 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;
+  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.
+  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();
 }
 
 void WebContentsEntry::WebContentsDestroyed() {
   ClearAllTasks(true);
   provider_->DeleteEntry(web_contents());
 }
 
@@ skipping 46 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) {

[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

     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_);
 
   RendererTask* new_task = nullptr;
   // We don't create a task if there's one for this site_instance AND
   // if this is not the main frame or we did record a main frame for the entry.
@@ skipping 169 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