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

Prior to this change, WebContentsTaskProvider would defer task creation if the
PID was not known at RenderFrameCreated time. However, for a newly-cloned
WebContents, no event ever occurred to re-create the Task once the process
finished launching. This was bug 738169.

Fix the problem by never deferring task_manager::Task creation; instead, create
Tasks for all live, current RenderFrameHosts. If the PID is not known immediately,
allow it to be set on the task at a later time (when RenderProcessHost::IsReady()
becomes true).

Because the TaskManagerImpl was written with the expectation that Tasks have
a constant PID, PID mutation is implemented in terms of temporarily removing the
task, mutating the PID, and re-adding it.

In WebContentsTaskProvider, we no longer need to implement RenderProcessGone.

In TaskGroup, don't waste time collecting stats for a null process ID.

Tests are added exercising this bug, and its subframe equivalent. Test coverage
is added for the PID==0 case, and we ensure that the "Memory" column is
populated with something nonzero.

BUG=738169

Review-Url: https://codereview.chromium.org/2961423002
Cr-Commit-Position: refs/heads/master@{#491180}

chrome/browser/task_manager/providers/task.cc

@@ -12,6 +12,7 @@
 #include "chrome/browser/profiles/profile_attributes_entry.h"
 #include "chrome/browser/profiles/profile_attributes_storage.h"
 #include "chrome/browser/profiles/profile_manager.h"
+#include "chrome/browser/task_manager/providers/task_provider_observer.h"
 #include "chrome/browser/task_manager/task_manager_observer.h"
 #include "content/public/common/result_codes.h"
 
@@ -22,6 +23,13 @@ namespace {
 // The last ID given to the previously created task.
 int64_t g_last_id = 0;
 
+base::ProcessId DetermineProcessId(base::ProcessHandle handle,
+                                   base::ProcessId process_id) {
+  if (process_id != base::kNullProcessId)
+    return process_id;
+  return base::GetProcId(handle);
+}
+
 }  // namespace
 
 Task::Task(const base::string16& title,
@@ -40,9 +48,7 @@ Task::Task(const base::string16& title,
       rappor_sample_name_(rappor_sample),
       icon_(icon ? *icon : gfx::ImageSkia()),
       process_handle_(handle),
-      process_id_(process_id != base::kNullProcessId
-                      ? process_id
-                      : base::GetProcId(handle)) {}
+      process_id_(DetermineProcessId(handle, process_id)) {}
 
 Task::~Task() {}
 
@@ -94,6 +100,24 @@ void Task::Refresh(const base::TimeDelta& update_interval,
   last_refresh_cumulative_bytes_sent_ = cumulative_bytes_sent_;
 }
 
+void Task::UpdateProcessInfo(base::ProcessHandle handle,
+                             base::ProcessId process_id,
+                             TaskProviderObserver* observer) {
+  process_id = DetermineProcessId(handle, process_id);
+
+  // Don't remove the task if there is no change to the process ID.
+  if (process_id == process_id_)
+    return;
+
+  // TaskManagerImpl and TaskGroup implementations assume that a process ID is
+  // consistent for the lifetime of a Task. So to change the process ID,
+  // temporarily unregister this Task.
+  observer->TaskRemoved(this);
+  process_handle_ = handle;
+  process_id_ = process_id;
+  observer->TaskAdded(this);
+}
+
 void Task::OnNetworkBytesRead(int64_t bytes_read) {
   cumulative_bytes_read_ += bytes_read;
 }

chrome/browser/task_manager/providers/task.h

@@ -21,6 +21,8 @@ class Profile;
 
 namespace task_manager {
 
+class TaskProviderObserver;
+
 // Defines a task that corresponds to a tab, an app, an extension, ... etc. It
 // represents one row in the task manager table. Multiple tasks can share the
 // same process, in which case they're grouped together in the task manager
@@ -83,6 +85,13 @@ class Task {
   virtual void Refresh(const base::TimeDelta& update_interval,
                        int64_t refresh_flags);
 
+  // Modifies the value of process_id(). To mutate the process ID, this Task is
+  // temporarily unregistered from |observer|, and then re-registered before
+  // returning.
+  void UpdateProcessInfo(base::ProcessHandle handle,
+                         base::ProcessId process_id,
+                         TaskProviderObserver* observer);
+
   // Will receive this notification through the task manager from
   // |ChromeNetworkDelegate::OnNetworkBytesReceived()|. The task will add to the
   // |cummulative_read_bytes_|.
@@ -213,10 +222,10 @@ class Task {
   gfx::ImageSkia icon_;
 
   // The handle of the process on which this task is running.
-  const base::ProcessHandle process_handle_;
+  base::ProcessHandle process_handle_;
 
   // The PID of the process on which this task is running.
-  const base::ProcessId process_id_;
+  base::ProcessId process_id_;
 
   DISALLOW_COPY_AND_ASSIGN(Task);
 };

chrome/browser/task_manager/providers/task_provider.cc

@@ -41,4 +41,13 @@ void TaskProvider::NotifyObserverTaskUnresponsive(Task* task) const {
   observer_->TaskUnresponsive(task);
 }
 
+void TaskProvider::UpdateTaskProcessInfoAndNotifyObserver(
+    Task* existing_task,
+    base::ProcessHandle new_process_handle,
+    base::ProcessId new_process_id) const {
+  DCHECK(observer_);
+  existing_task->UpdateProcessInfo(new_process_handle, new_process_id,
+                                   observer_);
+}
+
 }  // namespace task_manager

chrome/browser/task_manager/providers/task_provider.h

@@ -50,6 +50,10 @@ class TaskProvider {
   void NotifyObserverTaskAdded(Task* task) const;
   void NotifyObserverTaskRemoved(Task* task) const;
   void NotifyObserverTaskUnresponsive(Task* task) const;
+  void UpdateTaskProcessInfoAndNotifyObserver(
+      Task* existing_task,
+      base::ProcessHandle new_process_handle,
+      base::ProcessId new_process_id) const;
 
  private:
   // This will be called once an observer is set for this provider. When it is

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

@@ -55,7 +55,6 @@ class WebContentsEntry : public content::WebContentsObserver {
                               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;
@@ -76,6 +75,10 @@ class WebContentsEntry : public content::WebContentsObserver {
   // 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_;
 
@@ -135,49 +138,66 @@ void WebContentsEntry::ClearAllTasks(bool notify_observer) {
 
 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 process ID.
   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)
+    return;
+
+  Task* task = GetTaskForFrame(render_frame_host);
+
+  if (!task)
+    return;
+
+  const base::ProcessId determine_pid_from_handle = base::kNullProcessId;
+  provider_->UpdateTaskProcessInfoAndNotifyObserver(
+      task, render_frame_host->GetProcess()->GetHandle(),
+      determine_pid_from_handle);
 }
 
 void WebContentsEntry::WebContentsDestroyed() {
@@ -185,10 +205,6 @@ void WebContentsEntry::WebContentsDestroyed() {
   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.
@@ -242,23 +258,34 @@ void WebContentsEntry::TitleWasSet(content::NavigationEntry* entry,
 }
 
 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())
-    return;
+
+  // Exclude sad tabs and sad oopifs.
   if (!render_frame_host->IsRenderFrameLive())
     return;
 
+  // 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()) {
+    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 =
@@ -280,7 +307,7 @@ void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* render_frame_host) {
     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
@@ -298,6 +325,15 @@ void WebContentsEntry::CreateTaskForFrame(RenderFrameHost* 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) {
+      render_frame_host->GetProcess()->PostTaskWhenProcessIsReady(base::Bind(
+          &WebContentsEntry::RenderFrameReady, weak_factory_.GetWeakPtr(),
+          render_frame_host->GetProcess()->GetID(),
+          render_frame_host->GetRoutingID()));
+    }
   }
 }
 
@@ -320,6 +356,9 @@ void WebContentsEntry::ClearTaskForFrame(RenderFrameHost* render_frame_host) {
     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(
@@ -335,9 +374,7 @@ void WebContentsEntry::ForEachTask(
 
 ////////////////////////////////////////////////////////////////////////////////
 
-WebContentsTaskProvider::WebContentsTaskProvider()
-    : is_updating_(false) {
-}
+WebContentsTaskProvider::WebContentsTaskProvider() : is_updating_(false) {}
 
 WebContentsTaskProvider::~WebContentsTaskProvider() {
   if (is_updating_) {

chrome/browser/task_manager/sampling/task_group.cc

@@ -109,32 +109,33 @@ TaskGroup::TaskGroup(
       gpu_memory_has_duplicates_(false),
       is_backgrounded_(false),
       weak_ptr_factory_(this) {
-  scoped_refptr<TaskGroupSampler> sampler(
-      new TaskGroupSampler(base::Process::Open(proc_id),
-                           blocking_pool_runner,
-                           base::Bind(&TaskGroup::OnCpuRefreshDone,
-                                      weak_ptr_factory_.GetWeakPtr()),
-                           base::Bind(&TaskGroup::OnMemoryUsageRefreshDone,
-                                      weak_ptr_factory_.GetWeakPtr()),
-                           base::Bind(&TaskGroup::OnIdleWakeupsRefreshDone,
-                                      weak_ptr_factory_.GetWeakPtr()),
+  if (process_id_ != base::kNullProcessId) {
+    worker_thread_sampler_ = base::MakeRefCounted<TaskGroupSampler>(
+        base::Process::Open(process_id_), blocking_pool_runner,
+        base::Bind(&TaskGroup::OnCpuRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&TaskGroup::OnMemoryUsageRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&TaskGroup::OnIdleWakeupsRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()),
 #if defined(OS_LINUX)
-                           base::Bind(&TaskGroup::OnOpenFdCountRefreshDone,
-                                      weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&TaskGroup::OnOpenFdCountRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()),
 #endif  // defined(OS_LINUX)
-                           base::Bind(&TaskGroup::OnProcessPriorityDone,
-                                      weak_ptr_factory_.GetWeakPtr())));
-  worker_thread_sampler_.swap(sampler);
-
-  shared_sampler_->RegisterCallbacks(
-      process_id_, base::Bind(&TaskGroup::OnIdleWakeupsRefreshDone,
-                              weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&TaskGroup::OnPhysicalMemoryUsageRefreshDone,
-                 weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&TaskGroup::OnStartTimeRefreshDone,
-                 weak_ptr_factory_.GetWeakPtr()),
-      base::Bind(&TaskGroup::OnCpuTimeRefreshDone,
-                 weak_ptr_factory_.GetWeakPtr()));
+        base::Bind(&TaskGroup::OnProcessPriorityDone,
+                   weak_ptr_factory_.GetWeakPtr()));
+
+    shared_sampler_->RegisterCallbacks(
+        process_id_,
+        base::Bind(&TaskGroup::OnIdleWakeupsRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&TaskGroup::OnPhysicalMemoryUsageRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&TaskGroup::OnStartTimeRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()),
+        base::Bind(&TaskGroup::OnCpuTimeRefreshDone,
+                   weak_ptr_factory_.GetWeakPtr()));
+  }
 }
 
 TaskGroup::~TaskGroup() {
@@ -226,7 +227,8 @@ void TaskGroup::Refresh(const gpu::VideoMemoryUsageStats& gpu_memory_stats,
   // 9-  Idle Wakeups per second.
   // 10-  (Linux and ChromeOS only) The number of file descriptors current open.
   // 11- Process priority (foreground vs. background).
-  worker_thread_sampler_->Refresh(refresh_flags);
+  if (worker_thread_sampler_)
+    worker_thread_sampler_->Refresh(refresh_flags);
 }
 
 Task* TaskGroup::GetTaskById(TaskId task_id) const {

chrome/browser/task_manager/task_manager_browsertest.cc

@@ -867,6 +867,109 @@ IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, DevToolsOldUndockedWindow) {
   DevToolsWindowTesting::CloseDevToolsWindowSync(devtools);
 }
 
+IN_PROC_BROWSER_TEST_F(TaskManagerBrowserTest, HistoryNavigationInNewTab) {
+  ShowTaskManager();
+
+  ui_test_utils::NavigateToURL(browser(), GetTestURL());
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(1, MatchTab("title1.html")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(1, MatchAnyTab()));
+
+  ui_test_utils::NavigateToURL(browser(), GURL("about:version"));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(1, MatchTab("About Version")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(1, MatchAnyTab()));
+
+  chrome::GoBack(browser(), WindowOpenDisposition::NEW_BACKGROUND_TAB);
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(1, MatchTab("About Version")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(1, MatchTab("title1.html")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(2, MatchAnyTab()));
+
+  // In http://crbug.com/738169, the task_manager::Task for the background tab
+  // was created with process id 0, resulting in zero values for all process
+  // metrics. Ensure that this is not the case.
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerStatToExceed(
+      MatchTab("title1.html"), ColumnSpecifier::PROCESS_ID,
+      base::kNullProcessId));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerStatToExceed(
+      MatchTab("title1.html"), ColumnSpecifier::PHYSICAL_MEMORY, 1000));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerStatToExceed(
+      MatchTab("About Version"), ColumnSpecifier::PHYSICAL_MEMORY, 1000));
+}
+
+IN_PROC_BROWSER_TEST_P(TaskManagerOOPIFBrowserTest, SubframeHistoryNavigation) {
+  if (!ShouldExpectSubframes())
+    return;  // This test is lame without OOPIFs.
+
+  ShowTaskManager();
+
+  // This URL will have two out-of-process iframe processes (for b.com and
+  // c.com) under --site-per-process: it's an a.com page containing a b.com
+  // <iframe> containing a b.com <iframe> containing a c.com <iframe>.
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL(
+                     "a.com", "/cross_site_iframe_factory.html?a(b(b(c)))"));
+
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchTab("Cross-site iframe factory")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://b.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://c.com/")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(2, MatchAnySubframe()));
+
+  GURL d_url = embedded_test_server()->GetURL(
+      "d.com", "/cross_site_iframe_factory.html?d(e)");
+  ASSERT_TRUE(content::ExecuteScript(
+      browser()->tab_strip_model()->GetActiveWebContents()->GetMainFrame(),
+      "frames[0][0].location.href = '" + d_url.spec() + "';"));
+
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(0, MatchSubframe("http://c.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://d.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://e.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://b.com/")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(3, MatchAnySubframe()));
+
+  chrome::GoBack(browser(), WindowOpenDisposition::CURRENT_TAB);
+
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://c.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(0, MatchSubframe("http://d.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(0, MatchSubframe("http://e.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://b.com/")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(2, MatchAnySubframe()));
+
+  chrome::GoForward(browser(), WindowOpenDisposition::NEW_BACKGROUND_TAB);
+
+  // When the subframe appears in the cloned process, it must have a valid
+  // process ID.
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerStatToExceed(
+      MatchSubframe("http://d.com/"), ColumnSpecifier::PROCESS_ID,
+      base::kNullProcessId));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerStatToExceed(
+      MatchSubframe("http://e.com/"), ColumnSpecifier::PROCESS_ID,
+      base::kNullProcessId));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(2, MatchSubframe("http://b.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(2, MatchSubframe("http://b.com/")));
+  ASSERT_NO_FATAL_FAILURE(
+      WaitForTaskManagerRows(1, MatchSubframe("http://c.com/")));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(5, MatchAnySubframe()));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(2, MatchAnyTab()));
+
+  // Subframe processes should report some amount of physical memory usage.
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerStatToExceed(
+      MatchSubframe("http://d.com/"), ColumnSpecifier::PHYSICAL_MEMORY, 1000));
+  ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerStatToExceed(
+      MatchSubframe("http://e.com/"), ColumnSpecifier::PHYSICAL_MEMORY, 1000));
+}
+
 IN_PROC_BROWSER_TEST_P(TaskManagerOOPIFBrowserTest, KillSubframe) {
   ShowTaskManager();
 
@@ -1156,14 +1259,7 @@ IN_PROC_BROWSER_TEST_P(TaskManagerOOPIFBrowserTest,
   ASSERT_NO_FATAL_FAILURE(WaitForTaskManagerRows(0, MatchAnySubframe()));
 }
 
-// Flaky on Linux http://crbug.com/700684
-#if defined(OS_LINUX)
-#define MAYBE_OrderingOfDependentRows DISABLED_OrderingOfDependentRows
-#else
-#define MAYBE_OrderingOfDependentRows OrderingOfDependentRows
-#endif
-IN_PROC_BROWSER_TEST_P(TaskManagerOOPIFBrowserTest,
-                       MAYBE_OrderingOfDependentRows) {
+IN_PROC_BROWSER_TEST_P(TaskManagerOOPIFBrowserTest, OrderingOfDependentRows) {
   ShowTaskManager();
 
   GURL a_with_frames(embedded_test_server()->GetURL(

chrome/browser/task_manager/task_manager_browsertest_util.cc

@@ -106,6 +106,10 @@ class ResourceChangeObserver {
     switch (column_specifier_) {
       case ColumnSpecifier::COLUMN_NONE:
         return "N/A";
+      case ColumnSpecifier::PROCESS_ID:
+        return "Process ID";
+      case ColumnSpecifier::PHYSICAL_MEMORY:
+        return "Physical Memory";
       case ColumnSpecifier::V8_MEMORY:
         return "V8 Memory";
       case ColumnSpecifier::V8_MEMORY_USED:

chrome/browser/task_manager/task_manager_browsertest_util.h

@@ -21,6 +21,8 @@ namespace browsertest_util {
 // Specifies some integer-valued column of numeric data reported by the task
 // manager model. Please add more here as needed by tests.
 enum class ColumnSpecifier {
+  PROCESS_ID,
+  PHYSICAL_MEMORY,
   V8_MEMORY,
   V8_MEMORY_USED,
   SQLITE_MEMORY_USED,

chrome/browser/task_manager/task_manager_tester.cc

@@ -99,6 +99,12 @@ void TaskManagerTester::ToggleColumnVisibility(ColumnSpecifier column) {
   switch (column) {
     case ColumnSpecifier::COLUMN_NONE:
       return;
+    case ColumnSpecifier::PROCESS_ID:
+      column_id = IDS_TASK_MANAGER_PROCESS_ID_COLUMN;
+      break;
+    case ColumnSpecifier::PHYSICAL_MEMORY:
+      column_id = IDS_TASK_MANAGER_PHYSICAL_MEM_COLUMN;
+      break;
     case ColumnSpecifier::SQLITE_MEMORY_USED:
       column_id = IDS_TASK_MANAGER_SQLITE_MEMORY_USED_COLUMN;
       break;
@@ -130,6 +136,14 @@ int64_t TaskManagerTester::GetColumnValue(ColumnSpecifier column, int row) {
     case ColumnSpecifier::COLUMN_NONE:
     case ColumnSpecifier::MEMORY_STATE:
       break;
+    case ColumnSpecifier::PHYSICAL_MEMORY:
+      value = task_manager()->GetPhysicalMemoryUsage(task_id);
+      success = true;
+      break;
+    case ColumnSpecifier::PROCESS_ID:
+      value = static_cast<int64_t>(task_manager()->GetProcessId(task_id));
+      success = true;
+      break;
     case ColumnSpecifier::V8_MEMORY:
       success = task_manager()->GetV8Memory(task_id, &value, &ignored);
       break;