Query NaCl processes' GDB debug port on the correct thread.

chrome/browser/task_manager/sampling/task_group.cc

Index: chrome/browser/task_manager/sampling/task_group.cc
diff --git a/chrome/browser/task_manager/sampling/task_group.cc b/chrome/browser/task_manager/sampling/task_group.cc
index a7a0d11116ec0b15b4f361873cfdc146174b294e..4cb387ab5f2f9974a08450ad8dfcecada9fe14ef 100644
--- a/chrome/browser/task_manager/sampling/task_group.cc
+++ b/chrome/browser/task_manager/sampling/task_group.cc
@@ -31,7 +31,7 @@ const int kBackgroundRefreshTypesMask =
 #if defined(OS_LINUX)
     REFRESH_TYPE_FD_COUNT |
 #endif  // defined(OS_LINUX)
-    REFRESH_TYPE_PRIORITY;
+    REFRESH_TYPE_NACL | REFRESH_TYPE_PRIORITY;

[afakhry, 2017/02/10 20:08:52]

You still need to wrap REFRESH_TYPE_NACL inside:

#if !defined(DISABLE_NACL)

#endif

One of your tests should have caught this bug.

[Wez, 2017/02/13 01:22:19]

Done.

 
 #if defined(OS_WIN)
 // Gets the GDI and USER Handles on Windows at one shot.
@@ -89,7 +89,7 @@ TaskGroup::TaskGroup(
       user_peak_handles_(-1),
 #endif  // defined(OS_WIN)
 #if !defined(DISABLE_NACL)
-      nacl_debug_stub_port_(-1),
+      nacl_debug_stub_port_(nacl::kGdbDebugStubPortUnknown),
 #endif  // !defined(DISABLE_NACL)
       idle_wakeups_per_second_(-1),
 #if defined(OS_LINUX)
@@ -176,12 +176,16 @@ void TaskGroup::Refresh(const gpu::VideoMemoryUsageStats& gpu_memory_stats,
   }
 #endif  // defined(OS_WIN)
 
-  // 4- Refresh the NACL debug stub port (if enabled).
+// 4- Refresh the NACL debug stub port (if enabled). This calls out to
+//    NaClBrowser on the browser's IO thread, completing asynchronously.
 #if !defined(DISABLE_NACL)
   if (TaskManagerObserver::IsResourceRefreshEnabled(REFRESH_TYPE_NACL,
-                                                    refresh_flags) &&
-      !tasks_.empty()) {
-    RefreshNaClDebugStubPort(tasks_[0]->GetChildProcessUniqueID());
+                                                    refresh_flags)) {
+    if (!tasks_.empty()) {
+      RefreshNaClDebugStubPort(tasks_[0]->GetChildProcessUniqueID());
+    } else {
+      expected_on_bg_done_flags_ &= ~REFRESH_TYPE_NACL;

[afakhry, 2017/02/10 20:08:52]

Thanks for adding this!

Nit: Can you please remove the braces?

[Wez, 2017/02/13 01:22:19]

I thought there was a general preference for including braces, especially for
nested if()'s, after the OpenSSL goto issue, but OK. :)

Under what circumstances can a TaskGroup have zero tasks, out of interest?

[afakhry, 2017/02/15 02:58:19]

That shouldn't happen (See my comment on the unittest in the new patch). We can
probably replace this if with a DCHECK. I think I added that as a sanity check
before doing tasks_[0]. I *think* I wrote TaskGroup before I wrote
TaskManagerImpl.

[Wez, 2017/02/16 22:44:19]

Looking at TaskManagerImpl, it's clear that TaskGroup is removed as soon as it
is empty(), so I'm adding DCHECK(!empty()) to the top of the Refresh() call, as
a sanity-check.

+    }
   }
 #endif  // !defined(DISABLE_NACL)
 
@@ -255,14 +259,28 @@ void TaskGroup::RefreshWindowsHandles() {
 #endif  // defined(OS_WIN)
 }
 
-void TaskGroup::RefreshNaClDebugStubPort(int child_process_unique_id) {
 #if !defined(DISABLE_NACL)
-  nacl::NaClBrowser* nacl_browser = nacl::NaClBrowser::GetInstance();
-  nacl_debug_stub_port_ =
-      nacl_browser->GetProcessGdbDebugStubPort(child_process_unique_id);
-#endif  // !defined(DISABLE_NACL)
+static int GetNaClDebugStubPortOnIoThread(int process_id) {

[afakhry, 2017/02/10 20:08:52]

No DCHECK for IO?

Ideally the DCHECK should be placed inside
NaClBrowser::GetProcessGdbDebugStubPort() so that no other clients of that code
may repeat the same bug.

What about moving this function to the anonymous namespace above?

[Wez, 2017/02/13 01:22:19]

Yes, that's the change that prompted this patch, in fact. :) (see
https://codereview.chromium.org/2630443003/).
Done.

[afakhry, 2017/02/15 02:58:19]

Acknowledged.

+  return nacl::NaClBrowser::GetInstance()->GetProcessGdbDebugStubPort(
+      process_id);
 }
 
+void TaskGroup::RefreshNaClDebugStubPort(int child_process_unique_id) {
+  content::BrowserThread::PostTaskAndReplyWithResult(
+      content::BrowserThread::IO, FROM_HERE,
+      base::Bind(&GetNaClDebugStubPortOnIoThread, child_process_unique_id),
+      base::Bind(&TaskGroup::OnRefreshNaClDebugStubPortDone,
+                 weak_ptr_factory_.GetWeakPtr()));
+}
+
+void TaskGroup::OnRefreshNaClDebugStubPortDone(int nacl_debug_stub_port) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  nacl_debug_stub_port_ = nacl_debug_stub_port;
+  OnBackgroundRefreshTypeFinished(REFRESH_TYPE_NACL);
+}
+#endif  // !defined(DISABLE_NACL)
+
 void TaskGroup::OnCpuRefreshDone(double cpu_usage) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);