[TaskManager] Allow a Task to mutate its PID after creation

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"
@@ skipping 37 matching lines @@
   // |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 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);
 
+  // Walks parents until hitting a process boundary. Returns the highest frame
+  // in the same SiteInstance as |render_frame_host|.
+  RenderFrameHost* FindLocalRoot(RenderFrameHost* render_frame_host) const;
+
   // 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.
@@ skipping 39 matching lines @@
       },
       provider_, notify_observer, web_contents()));
 
   frames_by_site_instance_.clear();
   tasks_by_frames_.clear();
   main_frame_site_instance_ = nullptr;
 }
 
 RendererTask* WebContentsEntry::GetTaskForFrame(
     RenderFrameHost* render_frame_host) const {
-  auto itr = tasks_by_frames_.find(render_frame_host);
+  // Only local roots are in |tasks_by_frames_|.
+  auto itr = tasks_by_frames_.find(FindLocalRoot(render_frame_host));
   if (itr == tasks_by_frames_.end())
     return nullptr;
 
   return itr->second;
 }
 
+RenderFrameHost* WebContentsEntry::FindLocalRoot(
+    RenderFrameHost* render_frame_host) const {
+  SiteInstance* site_instance = render_frame_host->GetSiteInstance();
+  RenderFrameHost* candidate = render_frame_host;
+  while (RenderFrameHost* parent = candidate->GetParent()) {
+    if (parent->GetSiteInstance() != site_instance)
+      break;
+    candidate = parent;
+  }
+  return candidate;
+}
+
 void WebContentsEntry::RenderFrameDeleted(RenderFrameHost* render_frame_host) {
   ClearTaskForFrame(render_frame_host);
 }
 
 void WebContentsEntry::RenderFrameHostChanged(RenderFrameHost* old_host,
                                               RenderFrameHost* new_host) {
+  DCHECK(new_host->IsCurrent());
   ClearTaskForFrame(old_host);
   CreateTaskForFrame(new_host);
 }
 
 void WebContentsEntry::RenderFrameCreated(RenderFrameHost* render_frame_host) {
+  DCHECK(render_frame_host->IsRenderFrameLive());
+
   // Skip pending/speculative hosts. We'll create tasks for these if the
   // navigation commits, at which point RenderFrameHostChanged() will fire.
   if (!render_frame_host->IsCurrent())
     return;
 
-  // Task manager will have no separate entry for |render_frame_host| if it has
-  // the same site instance as its parent - quit early in this case.
-  if (render_frame_host->GetParent() &&
-      render_frame_host->GetParent()->GetSiteInstance() ==
-          render_frame_host->GetSiteInstance())
-    return;
-
-  // Postpone processing |render_frame_host| until its process has a PID.
-  render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(base::Bind(
-      &WebContentsEntry::RenderFrameReady, weak_factory_.GetWeakPtr(),
-      render_frame_host->GetProcess()->GetID(),
-      render_frame_host->GetRoutingID()));
+  CreateTaskForFrame(render_frame_host);
 }
 
 void WebContentsEntry::RenderFrameReady(int render_process_id,
                                         int render_frame_id) {
+  // We get here when a RenderProcessHost we are tracking transitions to the
+  // IsReady state. This might mean we know its the process ID.

[afakhry, 2017/07/10 17:27:53]

Nit: This might mean we know its (-the) process ID?

[ncarter (slow), 2017/07/31 22:30:29]

Done.

   content::RenderFrameHost* render_frame_host =
       content::RenderFrameHost::FromID(render_process_id, render_frame_id);
-  if (render_frame_host)
-    CreateTaskForFrame(render_frame_host);
+  if (render_frame_host) {
+    Task* task = GetTaskForFrame(render_frame_host);
+
+    if (task) {

[afakhry, 2017/07/10 17:27:53]

Nit: Much indented nested blocks?
if (!render_frame_host)
  return;

Task* task = GetTaskForFrame(render_frame_host);
if (!task) 
  return;

[ncarter (slow), 2017/07/31 22:30:29]

Done.

+      base::ProcessId determine_pid_from_handle = base::kNullProcessId;

[afakhry, 2017/07/10 17:27:53]

Nit: Can you make this a const?

[ncarter (slow), 2017/07/31 22:30:29]

Done.

+      provider_->UpdateTaskProcessInfoAndNotifyObserver(
+          task, render_frame_host->GetProcess()->GetHandle(),
+          determine_pid_from_handle);
+    }
+  }
 }
 
 void WebContentsEntry::WebContentsDestroyed() {
   ClearAllTasks(true);
   provider_->DeleteEntry(web_contents());
 }
 
-void WebContentsEntry::RenderProcessGone(base::TerminationStatus status) {
-  ClearAllTasks(true);
-}
-
 void WebContentsEntry::OnRendererUnresponsive(
     RenderWidgetHost* render_widget_host) {
   // Find the first RenderFrameHost matching the RenderWidgetHost.
   RendererTask* task = nullptr;
   for (const auto& pair : tasks_by_frames_) {
     if (pair.first->GetView() == render_widget_host->GetView()) {
       DCHECK_EQ(pair.first->GetProcess(), render_widget_host->GetProcess());
       task = pair.second;
       break;
     }
@@ skipping 33 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) {
+  // Currently we do not track pending hosts, or pending delete hosts.
+  DCHECK(render_frame_host->IsCurrent());
   DCHECK(render_frame_host);
   DCHECK(!tasks_by_frames_.count(render_frame_host));
 
   content::SiteInstance* site_instance = render_frame_host->GetSiteInstance();
-  if (!site_instance->GetProcess()->HasConnection())
+
+  // Exclude sad tabs and sad oopifs.
+  if (!render_frame_host->IsRenderFrameLive())
     return;
-  if (!render_frame_host->IsRenderFrameLive())
+
+  // Exclude frames in the same SiteInstance as their parent; |tasks_by_frames_|
+  // only contains local roots.
+  if (render_frame_host->GetParent() &&
+      site_instance == render_frame_host->GetParent()->GetSiteInstance())

[afakhry, 2017/07/10 17:27:53]

Nit: braces here.

[ncarter (slow), 2017/07/31 22:30:29]

Done.

     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.
+
+  // We need to create a task if one doesn't already exist for this
+  // SiteInstance, or if the main frame navigates to a process that currently is
+  // represented by a SubframeTask.
   if (!site_instance_exists || (is_main_frame && !site_instance_is_main)) {
     if (is_main_frame) {
       const WebContentsTag* tag =
           WebContentsTag::FromWebContents(web_contents());
       new_task = tag->CreateTask();
       main_frame_site_instance_ = site_instance;
     } else {
       new_task =
           new SubframeTask(render_frame_host, web_contents(),
                            GetTaskForFrame(web_contents()->GetMainFrame()));
     }
   }
 
   if (site_instance_exists) {
     // One of the existing frame hosts for this site instance.
     FramesList& existing_frames_for_site_instance =
         frames_by_site_instance_[site_instance];
     RenderFrameHost* existing_rfh = existing_frames_for_site_instance[0];
     RendererTask* old_task = tasks_by_frames_[existing_rfh];
 
     if (!new_task) {
-      // We didn't create any new task, so we keep appending the old one.
+      // We didn't create any new task, so we keep using the old one.
       tasks_by_frames_[render_frame_host] = old_task;
     } else {
       // Overwrite all the existing old tasks with the new one, and delete the
       // old one.
       for (RenderFrameHost* frame : existing_frames_for_site_instance)
         tasks_by_frames_[frame] = new_task;
 
       provider_->NotifyObserverTaskRemoved(old_task);
       delete old_task;
     }
   }
 
   frames_by_site_instance_[site_instance].push_back(render_frame_host);
 
   if (new_task) {
     tasks_by_frames_[render_frame_host] = new_task;
     provider_->NotifyObserverTaskAdded(new_task);
+
+    // If we don't know the OS process handle yet (e.g., because this
+    // task is still launching), update the task when it becomes
+    // available.
+    if (new_task->process_id() == base::kNullProcessId) {

[afakhry, 2017/07/10 17:27:53]

Nit: Indented nested blocks here too?

if (new_task->process_id() != base::kNullProcessId)
  return;

// <Your above comment here>
// PostTaskWhenProcessIsReady(...);

[ncarter (slow), 2017/07/31 22:30:29]

I opted not to do this one; in my mind early-return is for cases where we know
we're done with the whole operation (we hit an error or a null value, say).
Whereas this just happens to be the last if() in the function.

[afakhry, 2017/08/01 17:25:35]

Acknowledged.

+      render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(base::Bind(
+          &WebContentsEntry::RenderFrameReady, weak_factory_.GetWeakPtr(),
+          render_frame_host->GetProcess()->GetID(),
+          render_frame_host->GetRoutingID()));
+    }
   }
 }
 
 void WebContentsEntry::ClearTaskForFrame(RenderFrameHost* render_frame_host) {
   auto itr = tasks_by_frames_.find(render_frame_host);
   if (itr == tasks_by_frames_.end())
     return;
 
   RendererTask* task = itr->second;
   tasks_by_frames_.erase(itr);
   content::SiteInstance* site_instance = render_frame_host->GetSiteInstance();
   FramesList& frames = frames_by_site_instance_[site_instance];
   frames.erase(std::find(frames.begin(), frames.end(), render_frame_host));
 
   if (frames.empty()) {
     frames_by_site_instance_.erase(site_instance);
     provider_->NotifyObserverTaskRemoved(task);
     delete task;
 
     if (site_instance == main_frame_site_instance_)
       main_frame_site_instance_ = nullptr;
   }
+
+  // Whenever we have a task, we should have a main frame site instance.
+  DCHECK(tasks_by_frames_.empty() == (main_frame_site_instance_ == nullptr));
 }
 
 void WebContentsEntry::ForEachTask(
     const base::Callback<void(RendererTask*)>& on_task) {
   for (const auto& pair : frames_by_site_instance_) {
     const FramesList& frames_list = pair.second;
     DCHECK(!frames_list.empty());
     RendererTask* task = tasks_by_frames_[frames_list[0]];
 
     on_task.Run(task);
   }
 }
 
 ////////////////////////////////////////////////////////////////////////////////
 
-WebContentsTaskProvider::WebContentsTaskProvider()
-    : is_updating_(false) {
-}
+WebContentsTaskProvider::WebContentsTaskProvider() : is_updating_(false) {}
 
 WebContentsTaskProvider::~WebContentsTaskProvider() {
   if (is_updating_) {
     StopUpdating();
   }
 }
 
 void WebContentsTaskProvider::OnWebContentsTagCreated(
     const WebContentsTag* tag) {
   DCHECK(tag);
@@ skipping 80 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