Show all system process in the task_manager.

chrome/browser/chromeos/arc/arc_process_service.cc

Index: chrome/browser/chromeos/arc/arc_process_service.cc
diff --git a/chrome/browser/chromeos/arc/arc_process_service.cc b/chrome/browser/chromeos/arc/arc_process_service.cc
index 550ea0ae0c2f5a62cf0815969f2168c52a3a353c..d43b1bfd2ee382aa160a06af13345dece11f9ca6 100644
--- a/chrome/browser/chromeos/arc/arc_process_service.cc
+++ b/chrome/browser/chromeos/arc/arc_process_service.cc
@@ -13,6 +13,7 @@
 #include <set>
 #include <string>
 
+#include "base/callback_forward.h"

[Yusuke Sato, 2016/06/30 19:46:52]

including _forward.h in .cc seems a bit weird, and this file actually use
.Run(). Did you mean "base/callback.h" probably?

 #include "base/process/process.h"
 #include "base/process/process_iterator.h"
 #include "base/task_runner_util.h"
@@ -23,7 +24,7 @@ namespace arc {
 
 namespace {
 
-const char kSequenceToken[] = "arc_process_service";
+constexpr char kSequenceToken[] = "arc_process_service";
 
 // Weak pointer.  This class is owned by ArcServiceManager.
 ArcProcessService* g_arc_process_service = nullptr;
@@ -77,7 +78,60 @@ void ArcProcessService::Reset() {
   nspid_to_pid_.clear();
 }
 
-bool ArcProcessService::RequestProcessList(
+void ArcProcessService::PostTaskToOwnThreadAndReply(
+    const base::Closure& task,
+    const base::Closure& reply) {
+  auto runner = worker_pool_->GetSequencedTaskRunner(
+      worker_pool_->GetNamedSequenceToken(kSequenceToken));
+  runner->PostTaskAndReply(FROM_HERE, task, reply);
+}
+
+void ArcProcessService::RequestSystemProcessList(
+    RequestProcessListCallback callback) {
+  DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
+
+  auto ret_processes = new vector<ArcProcess>();

[Luis Héctor Chávez, 2016/06/30 16:46:03]

Raw pointers scare me a bit.

This can potentially leak if this object goes away before CallbackRelay can be
called. Can you instead make GetArcSystemProcessList return a vector<ArcProcess>
instead? PostTaskAndReplyWithResult should be able to take care of that
scenario.

[Yusuke Sato, 2016/06/30 19:46:52]

+1, although you probably have to make ArcProcessService a base::RefCounted<>
class.

[cylee1, 2016/06/30 21:02:39]

When I wrote the code I tried to use PostTaskAndReplyWithResult as you said, but
WeakPtr can only bind to functions with no return type. See
https://cs.chromium.org/chromium/src/base/bind_internal.h?rcl=0&l=317
So I made it so.

That said, I found later that I shouldn't use WeakPtr here at all, because from
comment of WeakPtr:
// Weak pointers may be passed safely between threads, but must always be
// dereferenced and invalidated on the same SequencedTaskRunner otherwise
// checking the pointer would be racey.
I shouldn't use WeakPtr in another thread, because it may be invalidated in the
calling thread if ArcProcessService object dies (so the WeakPtrFactor
invalidates all WeakPtr it has issued).  
I know the problem but haven't find a time to fix yet. Sorry for it.

I think using Unretained here might be fine, or we need to use RefCounted as
Yusuke said.
I've thought of using RefCounted, but since ArcProcessService is managed by
ArcServiceManager
(https://cs.chromium.org/chromium/src/components/arc/arc_service_manager.h?rcl...)
and it uses unique_ptr there, we may need more change to make it happen...

Also Line 169 needs to be changed as well.

BTW, I found it not rare in our code base to new a raw pointer and use a
Unretained then Owned pair in PostTaskAndReply.
E.g.
https://cs.chromium.org/chromium/src/components/drive/chromeos/change_list_lo...
https://cs.chromium.org/chromium/src/chrome/browser/chromeos/login/users/avat...
https://cs.chromium.org/chromium/src/components/drive/chromeos/file_system/co...
... and many
IMHO I think it may not be that risky.

Also the implementation of PostTaskAndReplyWithResult
https://cs.chromium.org/chromium/src/base/task_runner_util.h?rcl=1467300303&l=54
just new the ReturnType and use it like what I did here.
I don't seem to see any special handling to prevent from memory leak ??

[Yusuke Sato, 2016/06/30 22:23:53]

hmm... Luis, any idea?
I was more concerned about code quality. Using raw pointers makes the code more
difficult to modify (and review) IMO.
Typically the object passed from the first callback to the second one is
std::unique_ptr, which automatically prevents memory leaks.

[Luis Héctor Chávez, 2016/06/30 23:21:51]

CallbackRelay doesn't need to be a member function at all. It can be a
standalone function, obviating the need for the weak pointer for the second
call.

That being said, CallbackRelay might even be itself unnecessary, since IIUC you
can just pass the callback directly to PostTaskAndReplyWithResult.

[Yusuke Sato, 2016/06/30 23:40:50]

I think I misspoke. My understanding is that PostTaskAndReplyWithResult ensures
no memory leaks by forcing us to use RefCounted types.

[cylee1, 2016/07/01 23:15:14]

Yes.. I remember I introduced CallbackRelay because I thought I can't use
PostTaskAndReplyWithResult at that time.  If using PostTaskAndReplyWithResult
it's not needed.

[cylee1, 2016/07/01 23:15:14]

Sorry I don't see where it forcing us to use RefCounted type? But I remember
Bind forcing us to use either refcounted type of WeakPtrs, That's why it needs
UnRetained for raw pointers.
Anyway, I agree switching to PostTaskAndReplyWithResult() is the right way to
go.

+  PostTaskToOwnThreadAndReply(
+      base::Bind(&ArcProcessService::GetArcSystemProcessList,
+                 weak_ptr_factory_.GetWeakPtr(),
+                 base::Unretained(ret_processes)),

[Yusuke Sato, 2016/06/30 19:46:52]

remove

+      base::Bind(&ArcProcessService::CallbackRelay,
+                 weak_ptr_factory_.GetWeakPtr(), callback,
+                 base::Owned(ret_processes)));

[Yusuke Sato, 2016/06/30 19:46:52]

remove

+}
+
+void ArcProcessService::GetArcSystemProcessList(
+    vector<ArcProcess>* ret_processes) {

[Yusuke Sato, 2016/06/30 19:46:51]

remove once you switch to PostTaskAndReplyWithResult.

+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  const base::ProcessIterator::ProcessEntries& entry_list =
+      base::ProcessIterator(nullptr).Snapshot();
+  ProcessId arc_init_pid = GetArcInitProcessId(entry_list);
+
+  if (arc_init_pid == kNullProcessId) {
+    return;
+  }
+
+  // Enumerate the child processes of ARC init for gathering ARC System
+  // Processes.
+  for (const base::ProcessEntry& entry : entry_list) {
+    // TODO(hctsai): For now, we only gather direct child process of init, need
+    //               to get the processes below.

[Yusuke Sato, 2016/06/30 19:46:52]

Can you mention the intalled/dex2oat issue as an example?

+    if (entry.parent_pid() == arc_init_pid) {
+      const ProcessId child_pid = entry.pid();
+      const ProcessId child_nspid =
+          base::Process(child_pid).GetPidInNamespace();
+      const std::string process_name =
+          !entry.cmd_line_args().empty() ? entry.cmd_line_args()[0] : "";

[Yusuke Sato, 2016/06/30 19:46:52]

nit: can you factor this out as a function in anonymous namespace? line 298 is
doing the same.

+      if (child_nspid != kNullProcessId) {
+        ret_processes->emplace_back(child_nspid, child_pid, process_name,
+                                    mojom::ProcessState::PERSISTENT);
+      }
+    }
+  }
+}
+
+bool ArcProcessService::RequestAppProcessList(
     RequestProcessListCallback callback) {
   DCHECK_CURRENTLY_ON(content::BrowserThread::UI);
 
@@ -111,17 +165,12 @@ void ArcProcessService::OnReceiveProcessList(
   // To be safe I still use weak pointers, but weak_ptrs can only bind to
   // methods without return values. That's why I can't use
   // PostTaskAndReplyWithResult but handle the return object by myself.
-  auto runner = worker_pool_->GetSequencedTaskRunner(
-      worker_pool_->GetNamedSequenceToken(kSequenceToken));
-  runner->PostTaskAndReply(
-      FROM_HERE,
+  PostTaskToOwnThreadAndReply(
       base::Bind(&ArcProcessService::UpdateAndReturnProcessList,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 base::Owned(raw_processes),
+                 weak_ptr_factory_.GetWeakPtr(), base::Owned(raw_processes),

[Yusuke Sato, 2016/06/30 19:46:51]

base::Passed(&mojo_processes) ?

[Luis Héctor Chávez, 2016/06/30 19:50:37]

If you have to add a prefix to variables, I prefer to steer off "mojo_" and
prefer using something like "instance_" or "arc_".

                  base::Unretained(ret_processes)),

[Yusuke Sato, 2016/06/30 19:46:52]

remove

       base::Bind(&ArcProcessService::CallbackRelay,
-                 weak_ptr_factory_.GetWeakPtr(),
-                 callback,
+                 weak_ptr_factory_.GetWeakPtr(), callback,
                  base::Owned(ret_processes)));

[Yusuke Sato, 2016/06/30 19:46:52]

remove

 }
 
@@ -208,16 +257,7 @@ void ArcProcessService::UpdateNspidToPidMap() {
     process_tree[entry.parent_pid()].push_back(entry.pid());
 
   // Find the ARC init process.
-  ProcessId arc_init_pid = kNullProcessId;
-  for (const base::ProcessEntry& entry : entry_list) {
-    // TODO(nya): Add more constraints to avoid mismatches.
-    std::string process_name =
-        !entry.cmd_line_args().empty() ? entry.cmd_line_args()[0] : "";
-    if (process_name == "/init") {
-      arc_init_pid = entry.pid();
-      break;
-    }
-  }
+  ProcessId arc_init_pid = GetArcInitProcessId(entry_list);
 
   // Enumerate all processes under ARC init and create nspid -> pid map.
   if (arc_init_pid != kNullProcessId) {
@@ -248,4 +288,19 @@ void ArcProcessService::UpdateNspidToPidMap() {
   }
 }
 
+// static
+ProcessId ArcProcessService::GetArcInitProcessId(
+    const base::ProcessIterator::ProcessEntries& entry_list) {
+  // Find the ARC init process.
+  for (const base::ProcessEntry& entry : entry_list) {
+    // TODO(nya): Add more constraints to avoid mismatches.
+    std::string process_name =
+        !entry.cmd_line_args().empty() ? entry.cmd_line_args()[0] : "";
+    if (process_name == "/init") {
+      return entry.pid();
+    }
+  }
+  return kNullProcessId;
+}
+
 }  // namespace arc