Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2025593003:
Show all system process in the task_manager. (Closed)
|
Created:
4 years, 6 months ago by Hsu-Cheng Modified:
4 years, 3 months ago CC:
chromium-reviews, elijahtaylor+arcwatch_chromium.org, yusukes+watch_chromium.org, hidehiko+watch_chromium.org, lhchavez+watch_chromium.org, oshima+watch_chromium.org, davemoore+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionShow all system process in the task_manager.
BUG=623508
Committed: https://crrev.com/bb5e75574b94d73eb19209adad14d8d810fb7f98
Cr-Commit-Position: refs/heads/master@{#415918}
Patch Set 1 #Patch Set 2 : Add missing member variable. #
Total comments: 33
Patch Set 3 : Define system process. Refine code according to review suggestion. #
Total comments: 8
Patch Set 4 : Consolidate some more redundant logic. #
Total comments: 6
Patch Set 5 : change CL description #
Total comments: 2
Patch Set 6 : Fix typo and naming. #
Total comments: 36
Patch Set 7 : First step of refactor. Eliminate pointers and migrate to PostTaskAndReplyWithResult(). Need more w… #
Total comments: 4
Patch Set 8 : Change the RefCounted target. Pass pid map to function to get rid of weakptr. #Patch Set 9 : change callback_forward.h to callback.h #Patch Set 10 : Move two functions to anonymous namespace. #
Total comments: 40
Patch Set 11 : Change NsPidToPidMap to composite. Move static functions to anonymous namespace. Some style change. #
Total comments: 6
Patch Set 12 : cl lint #
Total comments: 9
Patch Set 13 : git fetch, rebase and merge. fix code for review comments #Patch Set 14 : Fix chrome-style issue. Fix initialization bug. #
Total comments: 2
Patch Set 15 : fetch and rebase #
Total comments: 4
Patch Set 16 : Use '= default;' #
Total comments: 5
Patch Set 17 : Use weakptr to avoid potential issues. Fetch and rebase. #Patch Set 18 : chrome-style complains 'Complex constructor has an inlined body.' #Patch Set 19 : Remove redundant check. #
Messages
Total messages: 96 (42 generated)
Description was changed from ========== Show all system process in the task_manager. BUG=28434655 ========== to ========== Show all system process in the task_manager. BUG=28434655 ==========
Description was changed from ========== Show all system process in the task_manager. BUG=28434655 ========== to ========== Show all system process in the task_manager. BUG=b:28434655 ==========
Description was changed from ========== Show all system process in the task_manager. BUG=b:28434655 ========== to ========== Show all system process in the task_manager. BUG=b/28434655 ==========
hctsai@google.com changed reviewers: + cylee@chromium.org
Sorry I didn't receive the email so didn't notice the CL until now. it seems you need to use "Public+Mail Comments" so I can see email notification. 1. You should mention ARC in the CL description. 2. Please run trybot to ensure it didn't break unit tests. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:80: bool ArcProcessService::RequestSystemProcessList( nit: can you try to consolidate the logic here and in OnReceiveProcessList ? For example, creating a function like PostTaskToOwnThreadAndReply(task, callback). https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:94: return true; Always return true? https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:97: void ArcProcessService::FetchAndReturnSystemProcessList( Maybe GetArcSystemProcessList https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:103: ProcessId arc_init_pid = GetArcInitProcessId(entry_list); You should check if it's kNullProcessId before doing the next step. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:105: // Enumerate the child processes of ARC init for gaterhing ARC system procces nit: gathering nit: processes I think you should add comment somewhere to note your definition of "system process" is "direct children of init process" and why so. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:107: std::string process_name = why not get process_name inside the if at line 109? https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:25: // Call RequestAppProcessList() / RequestSystemProcessList on the main UI nit: RequestSystemProcessList() https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:56: bool RequestSystemProcessList(RequestProcessListCallback callback); Have you measured how much time does the function take ? If it's costly you should leave a comment here to note it's a slow function. Also please try to give "system process" a definition. It's not trivial from the name. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:89: void ArcProcessTaskProvider::UpdateSystemProcessList( The logic is almost identical to the original function. Is it possible to consolidate the two function? Also can you unify them to use either nspid or pid as key? https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:121: void ArcProcessTaskProvider::OnUpdateAppProcessList( I don't quite understand the benefit of separating UpdateAppProcessList and OnUpdateAppProcessList. Can you explain?
yusukes@chromium.org changed reviewers: + yusukes@chromium.org
drive-by https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:26: const char kSequenceToken[] = "arc_process_service"; s/const/constexpr/ https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:85: auto runner = worker_pool_->GetSequencedTaskRunner( Why does this have to be sequenced, btw? https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:109: if (entry.parent_pid() == arc_init_pid) { This will only detect direct children of the init process, right? Is that good enough? For example, installd might fork/execute dex2oat which might take a few minutes to finish worst case, but with this logic, the dex2oat process won't show up in Task Manager. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:113: ArcProcess arc_process = {child_nspid, child_pid, process_name, This no longer compiles once you rebase your Chromium copy to tot because r397310 made ArcProcess move-only. ArcProcess arc_process(child_nspid, child_pid, process_name, mojom::ProcessState::PERSISTENT); ret_processes->push_back(std::move(arc_process)); https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. Comment to the CL description: I think you should file a bug on Chromium side (just like https://bugs.chromium.org/p/chromium/issues/detail?id=616159 ) and use the bug# in BUG=. Rietveld cannot update b/. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:63: base::ProcessId GetArcInitProcessId( can be static
ping? what's the status of this change?
On 2016/06/22 20:00:30, Yusuke Sato wrote: > ping? what's the status of this change? sorry, worked on other tasks. will pick this up and finish it ASAP next week.
Please take another look. Thanks for the review and sorry for late response. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:26: const char kSequenceToken[] = "arc_process_service"; On 2016/06/02 21:50:11, Yusuke Sato wrote: > s/const/constexpr/ Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:80: bool ArcProcessService::RequestSystemProcessList( On 2016/06/02 20:58:45, cylee1 wrote: > nit: can you try to consolidate the logic here and in OnReceiveProcessList ? > For example, creating a function like PostTaskToOwnThreadAndReply(task, > callback). Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:85: auto runner = worker_pool_->GetSequencedTaskRunner( On 2016/06/02 21:50:11, Yusuke Sato wrote: > Why does this have to be sequenced, btw? The pool size here is 1 and this is for avoid running delayed jobs at the same time. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:94: return true; On 2016/06/02 20:58:45, cylee1 wrote: > Always return true? Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:97: void ArcProcessService::FetchAndReturnSystemProcessList( On 2016/06/02 20:58:44, cylee1 wrote: > Maybe GetArcSystemProcessList Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:103: ProcessId arc_init_pid = GetArcInitProcessId(entry_list); On 2016/06/02 20:58:45, cylee1 wrote: > You should check if it's kNullProcessId before doing the next step. Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:105: // Enumerate the child processes of ARC init for gaterhing ARC system procces On 2016/06/02 20:58:44, cylee1 wrote: > nit: gathering > nit: processes > > I think you should add comment somewhere to note your definition of "system > process" is "direct children of init process" and why so. Added system process definition in arc_process_service.h https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:107: std::string process_name = On 2016/06/02 20:58:44, cylee1 wrote: > why not get process_name inside the if at line 109? Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:109: if (entry.parent_pid() == arc_init_pid) { On 2016/06/02 21:50:11, Yusuke Sato wrote: > This will only detect direct children of the init process, right? Is that good > enough? For example, installd might fork/execute dex2oat which might take a few > minutes to finish worst case, but with this logic, the dex2oat process won't > show up in Task Manager. You are right. I miss the case. I will make a new change for this. TODO is added. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:113: ArcProcess arc_process = {child_nspid, child_pid, process_name, On 2016/06/02 21:50:11, Yusuke Sato wrote: > This no longer compiles once you rebase your Chromium copy to tot because > r397310 made ArcProcess move-only. > > ArcProcess arc_process(child_nspid, child_pid, process_name, > mojom::ProcessState::PERSISTENT); > ret_processes->push_back(std::move(arc_process)); Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/02 21:50:11, Yusuke Sato wrote: > Comment to the CL description: > I think you should file a bug on Chromium side (just like > https://bugs.chromium.org/p/chromium/issues/detail?id=616159 ) and use the bug# > in BUG=. Rietveld cannot update b/. Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:25: // Call RequestAppProcessList() / RequestSystemProcessList on the main UI On 2016/06/02 20:58:45, cylee1 wrote: > nit: RequestSystemProcessList() Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:56: bool RequestSystemProcessList(RequestProcessListCallback callback); On 2016/06/02 20:58:45, cylee1 wrote: > Have you measured how much time does the function take ? > If it's costly you should leave a comment here to note it's a slow function. > > Also please try to give "system process" a definition. It's not trivial from the > name. I didn't test it with stress scenario. The time needed is about 5 ~ 30ms under normal use depending on the system loading. I think this is costly and I will add a note for it. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:63: base::ProcessId GetArcInitProcessId( On 2016/06/02 21:50:11, Yusuke Sato wrote: > can be static Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:89: void ArcProcessTaskProvider::UpdateSystemProcessList( On 2016/06/02 20:58:45, cylee1 wrote: > The logic is almost identical to the original function. Is it possible to > consolidate the two function? > Also can you unify them to use either nspid or pid as key? Done. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:121: void ArcProcessTaskProvider::OnUpdateAppProcessList( On 2016/06/02 20:58:45, cylee1 wrote: > I don't quite understand the benefit of separating UpdateAppProcessList and > OnUpdateAppProcessList. > Can you explain? Consolidated the logic here. This is because the code would get more complex when ScheduleNext***Request() become a closure argument and wrap one more level into Request***ProcessList(). (Imagine how to merge OnUpdate***ProcessList into Request***ProcessList) Appreciate any hints if there is simpler design here. I stuck here for a while.
On 2016/06/29 10:33:57, Hsu-Cheng wrote: > Please take another look. Thanks for the review and sorry for late response. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:26: const char > kSequenceToken[] = "arc_process_service"; > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > s/const/constexpr/ > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:80: bool > ArcProcessService::RequestSystemProcessList( > On 2016/06/02 20:58:45, cylee1 wrote: > > nit: can you try to consolidate the logic here and in OnReceiveProcessList ? > > For example, creating a function like PostTaskToOwnThreadAndReply(task, > > callback). > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:85: auto runner = > worker_pool_->GetSequencedTaskRunner( > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > Why does this have to be sequenced, btw? > > The pool size here is 1 and this is for avoid running delayed jobs at the same > time. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:94: return true; > On 2016/06/02 20:58:45, cylee1 wrote: > > Always return true? > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:97: void > ArcProcessService::FetchAndReturnSystemProcessList( > On 2016/06/02 20:58:44, cylee1 wrote: > > Maybe GetArcSystemProcessList > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:103: ProcessId arc_init_pid = > GetArcInitProcessId(entry_list); > On 2016/06/02 20:58:45, cylee1 wrote: > > You should check if it's kNullProcessId before doing the next step. > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:105: // Enumerate the child > processes of ARC init for gaterhing ARC system procces > On 2016/06/02 20:58:44, cylee1 wrote: > > nit: gathering > > nit: processes > > > > I think you should add comment somewhere to note your definition of "system > > process" is "direct children of init process" and why so. > > Added system process definition in arc_process_service.h > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:107: std::string process_name > = > On 2016/06/02 20:58:44, cylee1 wrote: > > why not get process_name inside the if at line 109? > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:109: if (entry.parent_pid() > == arc_init_pid) { > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > This will only detect direct children of the init process, right? Is that good > > enough? For example, installd might fork/execute dex2oat which might take a > few > > minutes to finish worst case, but with this logic, the dex2oat process won't > > show up in Task Manager. > > You are right. I miss the case. I will make a new change for this. TODO is > added. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.cc:113: ArcProcess arc_process = > {child_nspid, child_pid, process_name, > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > This no longer compiles once you rebase your Chromium copy to tot because > > r397310 made ArcProcess move-only. > > > > ArcProcess arc_process(child_nspid, child_pid, process_name, > > mojom::ProcessState::PERSISTENT); > > ret_processes->push_back(std::move(arc_process)); > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > File chrome/browser/chromeos/arc/arc_process_service.h (right): > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.h:1: // Copyright 2016 The > Chromium Authors. All rights reserved. > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > Comment to the CL description: > > I think you should file a bug on Chromium side (just like > > https://bugs.chromium.org/p/chromium/issues/detail?id=616159 ) and use the > bug# > > in BUG=. Rietveld cannot update b/. > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.h:25: // Call > RequestAppProcessList() / RequestSystemProcessList on the main UI > On 2016/06/02 20:58:45, cylee1 wrote: > > nit: RequestSystemProcessList() > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.h:56: bool > RequestSystemProcessList(RequestProcessListCallback callback); > On 2016/06/02 20:58:45, cylee1 wrote: > > Have you measured how much time does the function take ? > > If it's costly you should leave a comment here to note it's a slow function. > > > > Also please try to give "system process" a definition. It's not trivial from > the > > name. > > I didn't test it with stress scenario. The time needed is about 5 ~ 30ms under > normal use depending on the system loading. I think this is costly and I will > add a note for it. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > chrome/browser/chromeos/arc/arc_process_service.h:63: base::ProcessId > GetArcInitProcessId( > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > can be static > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc > (right): > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:89: > void ArcProcessTaskProvider::UpdateSystemProcessList( > On 2016/06/02 20:58:45, cylee1 wrote: > > The logic is almost identical to the original function. Is it possible to > > consolidate the two function? > > Also can you unify them to use either nspid or pid as key? > > Done. > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:121: > void ArcProcessTaskProvider::OnUpdateAppProcessList( > On 2016/06/02 20:58:45, cylee1 wrote: > > I don't quite understand the benefit of separating UpdateAppProcessList and > > OnUpdateAppProcessList. > > Can you explain? > Consolidated the logic here. This is because the code would get more complex > when ScheduleNext***Request() become a closure argument and wrap one more level > into Request***ProcessList(). (Imagine how to merge OnUpdate***ProcessList into > Request***ProcessList) Appreciate any hints if there is simpler design here. I > stuck here for a while. Can you check my comment at b/28434655#comment9? I'm still not sure which spec you're trying to implement...
https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:116: // Procceses nit: spelling : Processes.. https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:121: std::string process_name = nit: can be const https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:126: ret_processes->push_back(std::move(arc_process)); Just learnt that you can probably just use the C++ magic ret_processes->emplace_back(child_nspid, child_pid, process_name, mojom::ProcessState::PERSISTENT); https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:103: UpdateProcessList(pid_to_sys_task_, processes); it's called pid_to_sys_task_ but its key is actually nspid ? https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:98: UpdateProcessList(pid_to_task, processes); Actually I mean why the additional call, instead of just copy the body of UpdateProcessList() to this function? Maybe you think scheduler.Run() shouldn't be part of "UpdateProcessList" logic? I don't mind just calling UpdateProcessList() in OnUpdateAppProcessList() and OnUpdateSystemProcessList() and call ScheduleNextRequest() at the end of these 2 functions. The functions ScheduleNextAppRequest() and ScheduleNextSystemRequest() looks a bit overkill to me. https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:108: weak_ptr_factory_.GetWeakPtr())); As I mentioned above, I think ScheduleNextAppRequest and ScheduleNextSystemRequest are overkill. If you do want to keep these 2 functions, you don't use weakptr here because there's no thread switching. You use weakptr only when you switch to another thread and when you come back the object may be already gone. In direct calls you just need member function pointer ? Anyway I still think it's easier to just call ScheduleNextRequest in OnUpdateXXProcessList() https://codereview.chromium.org/2025593003/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.h (right): https://codereview.chromium.org/2025593003/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.h:68: std::map<base::ProcessId, std::unique_ptr<ArcProcessTask>> pid_to_sys_task_; If I read the code correct, both of them use nspid as key. So I mean why one named nspid_to_task but the other named pid_to_sys_task ?
On 2016/06/29 15:52:38, Yusuke Sato wrote: > On 2016/06/29 10:33:57, Hsu-Cheng wrote: > > Please take another look. Thanks for the review and sorry for late response. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:26: const char > > kSequenceToken[] = "arc_process_service"; > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > s/const/constexpr/ > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:80: bool > > ArcProcessService::RequestSystemProcessList( > > On 2016/06/02 20:58:45, cylee1 wrote: > > > nit: can you try to consolidate the logic here and in OnReceiveProcessList ? > > > For example, creating a function like PostTaskToOwnThreadAndReply(task, > > > callback). > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:85: auto runner = > > worker_pool_->GetSequencedTaskRunner( > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > Why does this have to be sequenced, btw? > > > > The pool size here is 1 and this is for avoid running delayed jobs at the same > > time. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:94: return true; > > On 2016/06/02 20:58:45, cylee1 wrote: > > > Always return true? > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:97: void > > ArcProcessService::FetchAndReturnSystemProcessList( > > On 2016/06/02 20:58:44, cylee1 wrote: > > > Maybe GetArcSystemProcessList > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:103: ProcessId arc_init_pid > = > > GetArcInitProcessId(entry_list); > > On 2016/06/02 20:58:45, cylee1 wrote: > > > You should check if it's kNullProcessId before doing the next step. > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:105: // Enumerate the child > > processes of ARC init for gaterhing ARC system procces > > On 2016/06/02 20:58:44, cylee1 wrote: > > > nit: gathering > > > nit: processes > > > > > > I think you should add comment somewhere to note your definition of "system > > > process" is "direct children of init process" and why so. > > > > Added system process definition in arc_process_service.h > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:107: std::string > process_name > > = > > On 2016/06/02 20:58:44, cylee1 wrote: > > > why not get process_name inside the if at line 109? > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:109: if (entry.parent_pid() > > == arc_init_pid) { > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > This will only detect direct children of the init process, right? Is that > good > > > enough? For example, installd might fork/execute dex2oat which might take a > > few > > > minutes to finish worst case, but with this logic, the dex2oat process won't > > > show up in Task Manager. > > > > You are right. I miss the case. I will make a new change for this. TODO is > > added. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.cc:113: ArcProcess arc_process > = > > {child_nspid, child_pid, process_name, > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > This no longer compiles once you rebase your Chromium copy to tot because > > > r397310 made ArcProcess move-only. > > > > > > ArcProcess arc_process(child_nspid, child_pid, process_name, > > > mojom::ProcessState::PERSISTENT); > > > ret_processes->push_back(std::move(arc_process)); > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > File chrome/browser/chromeos/arc/arc_process_service.h (right): > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.h:1: // Copyright 2016 The > > Chromium Authors. All rights reserved. > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > Comment to the CL description: > > > I think you should file a bug on Chromium side (just like > > > https://bugs.chromium.org/p/chromium/issues/detail?id=616159 ) and use the > > bug# > > > in BUG=. Rietveld cannot update b/. > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.h:25: // Call > > RequestAppProcessList() / RequestSystemProcessList on the main UI > > On 2016/06/02 20:58:45, cylee1 wrote: > > > nit: RequestSystemProcessList() > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.h:56: bool > > RequestSystemProcessList(RequestProcessListCallback callback); > > On 2016/06/02 20:58:45, cylee1 wrote: > > > Have you measured how much time does the function take ? > > > If it's costly you should leave a comment here to note it's a slow function. > > > > > > Also please try to give "system process" a definition. It's not trivial from > > the > > > name. > > > > I didn't test it with stress scenario. The time needed is about 5 ~ 30ms under > > normal use depending on the system loading. I think this is costly and I will > > add a note for it. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > chrome/browser/chromeos/arc/arc_process_service.h:63: base::ProcessId > > GetArcInitProcessId( > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > can be static > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > > File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc > > (right): > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > > chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:89: > > void ArcProcessTaskProvider::UpdateSystemProcessList( > > On 2016/06/02 20:58:45, cylee1 wrote: > > > The logic is almost identical to the original function. Is it possible to > > > consolidate the two function? > > > Also can you unify them to use either nspid or pid as key? > > > > Done. > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > > chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:121: > > void ArcProcessTaskProvider::OnUpdateAppProcessList( > > On 2016/06/02 20:58:45, cylee1 wrote: > > > I don't quite understand the benefit of separating UpdateAppProcessList and > > > OnUpdateAppProcessList. > > > Can you explain? > > Consolidated the logic here. This is because the code would get more complex > > when ScheduleNext***Request() become a closure argument and wrap one more > level > > into Request***ProcessList(). (Imagine how to merge OnUpdate***ProcessList > into > > Request***ProcessList) Appreciate any hints if there is simpler design here. I > > stuck here for a while. > > Can you check my comment at b/28434655#comment9? I'm still not sure which spec > you're trying to implement... I was planning to gather all the information of all system processes from arc_process_service for the first step and simply show them on task manager by now. I will draft a document for choosing which option and initiate discussion about the possible ui change later on.
On 2016/06/30 04:11:49, Hsu-Cheng wrote: > On 2016/06/29 15:52:38, Yusuke Sato wrote: > > On 2016/06/29 10:33:57, Hsu-Cheng wrote: > > > Please take another look. Thanks for the review and sorry for late response. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:26: const char > > > kSequenceToken[] = "arc_process_service"; > > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > > s/const/constexpr/ > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:80: bool > > > ArcProcessService::RequestSystemProcessList( > > > On 2016/06/02 20:58:45, cylee1 wrote: > > > > nit: can you try to consolidate the logic here and in OnReceiveProcessList > ? > > > > For example, creating a function like PostTaskToOwnThreadAndReply(task, > > > > callback). > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:85: auto runner = > > > worker_pool_->GetSequencedTaskRunner( > > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > > Why does this have to be sequenced, btw? > > > > > > The pool size here is 1 and this is for avoid running delayed jobs at the > same > > > time. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:94: return true; > > > On 2016/06/02 20:58:45, cylee1 wrote: > > > > Always return true? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:97: void > > > ArcProcessService::FetchAndReturnSystemProcessList( > > > On 2016/06/02 20:58:44, cylee1 wrote: > > > > Maybe GetArcSystemProcessList > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:103: ProcessId > arc_init_pid > > = > > > GetArcInitProcessId(entry_list); > > > On 2016/06/02 20:58:45, cylee1 wrote: > > > > You should check if it's kNullProcessId before doing the next step. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:105: // Enumerate the > child > > > processes of ARC init for gaterhing ARC system procces > > > On 2016/06/02 20:58:44, cylee1 wrote: > > > > nit: gathering > > > > nit: processes > > > > > > > > I think you should add comment somewhere to note your definition of > "system > > > > process" is "direct children of init process" and why so. > > > > > > Added system process definition in arc_process_service.h > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:107: std::string > > process_name > > > = > > > On 2016/06/02 20:58:44, cylee1 wrote: > > > > why not get process_name inside the if at line 109? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:109: if > (entry.parent_pid() > > > == arc_init_pid) { > > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > > This will only detect direct children of the init process, right? Is that > > good > > > > enough? For example, installd might fork/execute dex2oat which might take > a > > > few > > > > minutes to finish worst case, but with this logic, the dex2oat process > won't > > > > show up in Task Manager. > > > > > > You are right. I miss the case. I will make a new change for this. TODO is > > > added. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.cc:113: ArcProcess > arc_process > > = > > > {child_nspid, child_pid, process_name, > > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > > This no longer compiles once you rebase your Chromium copy to tot because > > > > r397310 made ArcProcess move-only. > > > > > > > > ArcProcess arc_process(child_nspid, child_pid, process_name, > > > > mojom::ProcessState::PERSISTENT); > > > > ret_processes->push_back(std::move(arc_process)); > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > File chrome/browser/chromeos/arc/arc_process_service.h (right): > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.h:1: // Copyright 2016 The > > > Chromium Authors. All rights reserved. > > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > > Comment to the CL description: > > > > I think you should file a bug on Chromium side (just like > > > > https://bugs.chromium.org/p/chromium/issues/detail?id=616159 ) and use the > > > bug# > > > > in BUG=. Rietveld cannot update b/. > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.h:25: // Call > > > RequestAppProcessList() / RequestSystemProcessList on the main UI > > > On 2016/06/02 20:58:45, cylee1 wrote: > > > > nit: RequestSystemProcessList() > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.h:56: bool > > > RequestSystemProcessList(RequestProcessListCallback callback); > > > On 2016/06/02 20:58:45, cylee1 wrote: > > > > Have you measured how much time does the function take ? > > > > If it's costly you should leave a comment here to note it's a slow > function. > > > > > > > > Also please try to give "system process" a definition. It's not trivial > from > > > the > > > > name. > > > > > > I didn't test it with stress scenario. The time needed is about 5 ~ 30ms > under > > > normal use depending on the system loading. I think this is costly and I > will > > > add a note for it. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... > > > chrome/browser/chromeos/arc/arc_process_service.h:63: base::ProcessId > > > GetArcInitProcessId( > > > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > > > can be static > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > > > File > chrome/browser/task_management/providers/arc/arc_process_task_provider.cc > > > (right): > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > > > > chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:89: > > > void ArcProcessTaskProvider::UpdateSystemProcessList( > > > On 2016/06/02 20:58:45, cylee1 wrote: > > > > The logic is almost identical to the original function. Is it possible to > > > > consolidate the two function? > > > > Also can you unify them to use either nspid or pid as key? > > > > > > Done. > > > > > > > > > https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/task_man... > > > > chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:121: > > > void ArcProcessTaskProvider::OnUpdateAppProcessList( > > > On 2016/06/02 20:58:45, cylee1 wrote: > > > > I don't quite understand the benefit of separating UpdateAppProcessList > and > > > > OnUpdateAppProcessList. > > > > Can you explain? > > > Consolidated the logic here. This is because the code would get more complex > > > when ScheduleNext***Request() become a closure argument and wrap one more > > level > > > into Request***ProcessList(). (Imagine how to merge OnUpdate***ProcessList > > into > > > Request***ProcessList) Appreciate any hints if there is simpler design here. > I > > > stuck here for a while. > > > > Can you check my comment at b/28434655#comment9? I'm still not sure which spec > > you're trying to implement... > > I was planning to gather all the information of all system processes from > arc_process_service for the first step and simply show them on task manager by > now. > I will draft a document for choosing which option and initiate discussion about > the possible ui change later on.
https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:116: // Procceses On 2016/06/30 03:45:52, cylee1 wrote: > nit: spelling : Processes.. Done. https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:121: std::string process_name = On 2016/06/30 03:45:52, cylee1 wrote: > nit: can be const Done. https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.cc:126: ret_processes->push_back(std::move(arc_process)); On 2016/06/30 03:45:52, cylee1 wrote: > Just learnt that you can probably just use the C++ magic > ret_processes->emplace_back(child_nspid, child_pid, process_name, > mojom::ProcessState::PERSISTENT); magic indeed ... Done https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/40001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:103: UpdateProcessList(pid_to_sys_task_, processes); On 2016/06/30 03:45:52, cylee1 wrote: > it's called pid_to_sys_task_ but its key is actually nspid ? Done. https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:98: UpdateProcessList(pid_to_task, processes); On 2016/06/30 03:45:52, cylee1 wrote: > Actually I mean why the additional call, instead of just copy the body of > UpdateProcessList() to this function? Maybe you think scheduler.Run() shouldn't > be part of "UpdateProcessList" logic? > > I don't mind just calling UpdateProcessList() in OnUpdateAppProcessList() and > OnUpdateSystemProcessList() > and call ScheduleNextRequest() at the end of these 2 functions. > The functions ScheduleNextAppRequest() and ScheduleNextSystemRequest() looks a > bit overkill to me. explain later https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:108: weak_ptr_factory_.GetWeakPtr())); On 2016/06/30 03:45:52, cylee1 wrote: > As I mentioned above, I think ScheduleNextAppRequest and > ScheduleNextSystemRequest are overkill. > If you do want to keep these 2 functions, you don't use weakptr here because > there's no thread switching. You use weakptr only when you switch to another > thread and when you come back the object may be already gone. In direct calls > you just need member function pointer ? > Anyway I still think it's easier to just call ScheduleNextRequest in > OnUpdateXXProcessList() I am not sure if I get your idea. The ScheduleNextRequest() is to post the same task (RequestAppProcessList) again to update the process info in the future. And the OnUpdateAppProcessList() is a callback brought to arc_process_service. Both involves thread switching. Thus I think we still need to keep both weakptr, in case that either when arc_process_service is calling the callback or when next RequestAppProcessList is executed, the task provider itself is gone. The OnUpdateProcessList vs OnUpdateAppProcessList/OnUpdateSystemProcessList and ScheduleNextRequest vs ScheduleNextAppRequest/ScheduleNextSystemRequest is simply to extract common logic into one function. https://codereview.chromium.org/2025593003/diff/80001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.h (right): https://codereview.chromium.org/2025593003/diff/80001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.h:68: std::map<base::ProcessId, std::unique_ptr<ArcProcessTask>> pid_to_sys_task_; On 2016/06/30 03:45:52, cylee1 wrote: > If I read the code correct, both of them use nspid as key. So I mean why one > named nspid_to_task but the other named pid_to_sys_task ? Yes, you are right. It was in my old implementation that propagate only pid for system process. This is a wrong naming now. Fixed.
lhchavez@chromium.org changed reviewers: + lhchavez@chromium.org
drive-by https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); 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.
I have two requests: 1) The arc_process_service.cc file is getting harder and harder to review because of its excessive use of raw pointers. This is not really a your fault but previous reviewers' including myself, but could you remove all the raw pointer uses this time? 2) > and simply show them on task manager by now. I understand, but this *is* a user-visible change, right? I don't want to be too bureaucratic, but could you please at least get a verbal okay from your PM? Apparently no PM is cc'ed on the b/ bug either. https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/20001/chrome/browser/chromeos... chrome/browser/chromeos/arc/arc_process_service.h:1: // Copyright 2016 The Chromium Authors. All rights reserved. On 2016/06/29 10:33:57, Hsu-Cheng wrote: > On 2016/06/02 21:50:11, Yusuke Sato wrote: > > Comment to the CL description: > > I think you should file a bug on Chromium side (just like > > https://bugs.chromium.org/p/chromium/issues/detail?id=616159 ) and use the > bug# > > in BUG=. Rietveld cannot update b/. > > Done. Not yet done? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:16: #include "base/callback_forward.h" including _forward.h in .cc seems a bit weird, and this file actually use .Run(). Did you mean "base/callback.h" probably? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > 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. +1, although you probably have to make ArcProcessService a base::RefCounted<> class. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:97: base::Unretained(ret_processes)), remove https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:100: base::Owned(ret_processes))); remove https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:104: vector<ArcProcess>* ret_processes) { remove once you switch to PostTaskAndReplyWithResult. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:119: // to get the processes below. Can you mention the intalled/dex2oat issue as an example? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:125: !entry.cmd_line_args().empty() ? entry.cmd_line_args()[0] : ""; nit: can you factor this out as a function in anonymous namespace? line 298 is doing the same. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:155: auto raw_processes = new vector<mojom::RunningAppProcessInfoPtr>(); Any reason to convert it to vector at this point? (see also my comment at line 185) https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:158: auto ret_processes = new vector<ArcProcess>(); This has the same, potential memory leak issue as line 93. Let's switch to ...WithResult(). https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:170: weak_ptr_factory_.GetWeakPtr(), base::Owned(raw_processes), base::Passed(&mojo_processes) ? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:171: base::Unretained(ret_processes)), remove https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:174: base::Owned(ret_processes))); remove https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:179: const vector<ArcProcess>* ret_processes) { unique_ptr instead of a raw pointer https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:185: const vector<arc::mojom::RunningAppProcessInfoPtr>* raw_processes, Line 152 receives a smiliar type of object as value (with move), and this line does with a raw pointer (with Owned). This is a bit difficult to read, and IIUC, using a raw pointer like this is kind of discouraged in Chromium these days. Can you change this line to mojo::Array<arc::mojom::RunningAppProcessInfoPtr> raw_processes (or mojo_processes) and use base::Passed(&mojo_processes) at line 170? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:186: vector<ArcProcess>* ret_processes) { remove once you switch to ...WithResult(). https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:215: const vector<arc::mojom::RunningAppProcessInfoPtr>* raw_processes, mojo::Array<arc::mojom::RunningAppProcessInfoPtr> raw_processes (or mojo_processes) https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:216: vector<ArcProcess>* ret_processes) { Can you use unique_ptr instead of a raw pointer? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:265: std::set<ProcessId> visited; nit: (not your fault but..) remove std:: to be consistent with line 190. Or remove line 38-40 instead. I kind of prefer the latter as 'using std::xxx;' is uncommon in chrome/. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:10: nit: IWYU #include "base/macros.h" for DISALLOW_COPY_AND_ASSIGN? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:62: bool RequestAppProcessList(RequestProcessListCallback callback); nit: const RequestProcessListCallback& ? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:63: void RequestSystemProcessList(RequestProcessListCallback callback); same https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:14: #include "base/callback_forward.h" same https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:28: const int kUpdateSystemProcessListDelaySeconds = 3; constexpr https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.h (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.h:44: void ScheduleNextRequest(const base::Closure& task, const int delaySeconds); s/const int/int/
just one more style nit https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:170: weak_ptr_factory_.GetWeakPtr(), base::Owned(raw_processes), On 2016/06/30 19:46:51, Yusuke Sato wrote: > base::Passed(&mojo_processes) ? If you have to add a prefix to variables, I prefer to steer off "mojo_" and prefer using something like "instance_" or "arc_".
https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (left): https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:102: // just opted out from ARC. line 100- 103 are gone ? https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/60001/chrome/browser/task_man... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:108: weak_ptr_factory_.GetWeakPtr())); On 2016/06/30 09:56:24, Hsu-Cheng wrote: > On 2016/06/30 03:45:52, cylee1 wrote: > > As I mentioned above, I think ScheduleNextAppRequest and > > ScheduleNextSystemRequest are overkill. > > If you do want to keep these 2 functions, you don't use weakptr here because > > there's no thread switching. You use weakptr only when you switch to another > > thread and when you come back the object may be already gone. In direct calls > > you just need member function pointer ? > > Anyway I still think it's easier to just call ScheduleNextRequest in > > OnUpdateXXProcessList() > > I am not sure if I get your idea. The ScheduleNextRequest() is to post the same > task (RequestAppProcessList) again to update the process info in the future. And > the OnUpdateAppProcessList() is a callback brought to arc_process_service. Both > involves thread switching. Thus I think we still need to keep both weakptr, in > case that either when arc_process_service is calling the callback or when next > RequestAppProcessList is executed, the task provider itself is gone. > > The OnUpdateProcessList vs OnUpdateAppProcessList/OnUpdateSystemProcessList and > ScheduleNextRequest vs ScheduleNextAppRequest/ScheduleNextSystemRequest is > simply to extract common logic into one function. When OnUpdateAppProcessList is called, it has already switch back to the calling thread (back from the thread(s) handling RequestAppProcessList). Then you called OnUpdateProcessList(), in turn UpdateProcessList(). Both functions don't switch thread anymore, then you call scheduler.Run(); This closure is executed, done. So from here to this closure is called, there's no thread switching. It's only when you Post(Delayed)Task and back to the calling thread do you need to worry if the object has dead. Talk to you tomorrow if needed. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > 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. 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 ?? https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:155: auto raw_processes = new vector<mojom::RunningAppProcessInfoPtr>(); On 2016/06/30 19:46:51, Yusuke Sato wrote: > Any reason to convert it to vector at this point? (see also my comment at line > 185) I don't remember the exact reason, but I remember it's hard to bind vector<mojom::RunningAppProcessInfoPtr> to a callback, even if I used move(), Pass(), or anything alike. Compiler alway prohibit me from moving it . Sorry that I didn't find a solution then. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:179: const vector<ArcProcess>* ret_processes) { On 2016/06/30 19:46:51, Yusuke Sato wrote: > unique_ptr instead of a raw pointer If use PostTaskAndReplyWithResult, maybe the vector is passed as a const reference? The same below
https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); On 2016/06/30 21:02:39, cylee1 wrote: > On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > > 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. > > 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... hmm... Luis, any idea? > > 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. I was more concerned about code quality. Using raw pointers makes the code more difficult to modify (and review) IMO. > 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 ?? Typically the object passed from the first callback to the second one is std::unique_ptr, which automatically prevents memory leaks.
https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); On 2016/06/30 22:23:53, Yusuke Sato wrote: > On 2016/06/30 21:02:39, cylee1 wrote: > > On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > > > 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. > > > > 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... > > hmm... Luis, any idea? > > > > > 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. > > I was more concerned about code quality. Using raw pointers makes the code more > difficult to modify (and review) IMO. > > > 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 ?? > > Typically the object passed from the first callback to the second one is > std::unique_ptr, which automatically prevents memory leaks. > > 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.
On 2016/06/30 23:21:51, Luis Héctor Chávez wrote: > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new > vector<ArcProcess>(); > On 2016/06/30 22:23:53, Yusuke Sato wrote: > > On 2016/06/30 21:02:39, cylee1 wrote: > > > On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > > > > 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. > > > > > > 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... > > > > hmm... Luis, any idea? > > > > > > > > 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. > > > > I was more concerned about code quality. Using raw pointers makes the code > more > > difficult to modify (and review) IMO. > > > > > 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 ?? > > > > Typically the object passed from the first callback to the second one is > > std::unique_ptr, which automatically prevents memory leaks. > > > > > > 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. Argh, but WeakPtr still would complain for the first callback. Another option is to pass in the callback to the callback and have it call it on all return paths. I don't know which option is better :/
https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); > > 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 ?? > > Typically the object passed from the first callback to the second one is > std::unique_ptr, which automatically prevents memory leaks. I think I misspoke. My understanding is that PostTaskAndReplyWithResult ensures no memory leaks by forcing us to use RefCounted types.
On 2016/06/30 23:40:50, Yusuke Sato wrote: > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new > vector<ArcProcess>(); > > > 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 ?? > > > > Typically the object passed from the first callback to the second one is > > std::unique_ptr, which automatically prevents memory leaks. > > I think I misspoke. My understanding is that PostTaskAndReplyWithResult ensures > no memory leaks by forcing us to use RefCounted types. OK, new idea: all of the functions that need to run on other threads either don't need to be methods (like GetArcSystemProcessList) or just update a small part of the object state (like UpdateAndReturnProcessList). So how about you make the non-necessarily-methods standalone anonymous namespace functions and wrap the updatable object state in a RefCounted small object owned by ArcProcessService, and then use PostTaskAndReplyWithResult bound to the RefCounted object. That way we avoid using weakptrs illegaly and clean up the code. WDYT?
On 2016/07/01 16:13:06, Luis Héctor Chávez wrote: > On 2016/06/30 23:40:50, Yusuke Sato wrote: > > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > > > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = > new > > vector<ArcProcess>(); > > > > 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 ?? > > > > > > Typically the object passed from the first callback to the second one is > > > std::unique_ptr, which automatically prevents memory leaks. > > > > I think I misspoke. My understanding is that PostTaskAndReplyWithResult > ensures > > no memory leaks by forcing us to use RefCounted types. > > OK, new idea: all of the functions that need to run on other threads either > don't need to be methods (like GetArcSystemProcessList) or just update a small > part of the object state (like UpdateAndReturnProcessList). So how about you > make the non-necessarily-methods standalone anonymous namespace functions and > wrap the updatable object state in a RefCounted small object owned by > ArcProcessService, and then use PostTaskAndReplyWithResult bound to the > RefCounted object. That way we avoid using weakptrs illegaly and clean up the > code. > > WDYT? Yup. That's what I'm thinking about. I've thought of the direction for a while, but haven't think it clearly all the details (e.g., how to extract to RefCount typed object out). I remember there could be problem, but maybe we can overcome. Personally I think using Unretained (temporarily) in the "Post" part and switch to PostTaskAndReplyWithResult may be a quick workaround. Then we can do a refactoring afterwards. Or do you think we should fix them all in this CL?
https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); On 2016/06/30 23:21:51, Luis Héctor Chávez wrote: > On 2016/06/30 22:23:53, Yusuke Sato wrote: > > On 2016/06/30 21:02:39, cylee1 wrote: > > > On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > > > > 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. > > > > > > 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... > > > > hmm... Luis, any idea? > > > > > > > > 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. > > > > I was more concerned about code quality. Using raw pointers makes the code > more > > difficult to modify (and review) IMO. > > > > > 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 ?? > > > > Typically the object passed from the first callback to the second one is > > std::unique_ptr, which automatically prevents memory leaks. > > > > > > 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. Yes.. I remember I introduced CallbackRelay because I thought I can't use PostTaskAndReplyWithResult at that time. If using PostTaskAndReplyWithResult it's not needed. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new vector<ArcProcess>(); On 2016/06/30 23:40:50, Yusuke Sato wrote: > > > 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 ?? > > > > Typically the object passed from the first callback to the second one is > > std::unique_ptr, which automatically prevents memory leaks. > > I think I misspoke. My understanding is that PostTaskAndReplyWithResult ensures > no memory leaks by forcing us to use RefCounted types. 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.
On 2016/07/01 23:15:14, cylee1 wrote: > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new > vector<ArcProcess>(); > On 2016/06/30 23:21:51, Luis Héctor Chávez wrote: > > On 2016/06/30 22:23:53, Yusuke Sato wrote: > > > On 2016/06/30 21:02:39, cylee1 wrote: > > > > On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > > > > > 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. > > > > > > > > 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... > > > > > > hmm... Luis, any idea? > > > > > > > > > > > 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. > > > > > > I was more concerned about code quality. Using raw pointers makes the code > > more > > > difficult to modify (and review) IMO. > > > > > > > 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 ?? > > > > > > Typically the object passed from the first callback to the second one is > > > std::unique_ptr, which automatically prevents memory leaks. > > > > > > > > > > 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. > > Yes.. I remember I introduced CallbackRelay because I thought I can't use > PostTaskAndReplyWithResult at that time. If using PostTaskAndReplyWithResult > it's not needed. > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = new > vector<ArcProcess>(); > On 2016/06/30 23:40:50, Yusuke Sato wrote: > > > > 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 ?? > > > > > > Typically the object passed from the first callback to the second one is > > > std::unique_ptr, which automatically prevents memory leaks. > > > > I think I misspoke. My understanding is that PostTaskAndReplyWithResult > ensures > > no memory leaks by forcing us to use RefCounted types. > > 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. Ah okay, you're right. I just got confused by the code. Either way, please try to follow the Luis' suggestion.
On 2016/07/01 23:36:59, Yusuke Sato wrote: > On 2016/07/01 23:15:14, cylee1 wrote: > > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > > > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = > new > > vector<ArcProcess>(); > > On 2016/06/30 23:21:51, Luis Héctor Chávez wrote: > > > On 2016/06/30 22:23:53, Yusuke Sato wrote: > > > > On 2016/06/30 21:02:39, cylee1 wrote: > > > > > On 2016/06/30 16:46:03, Luis Héctor Chávez wrote: > > > > > > 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. > > > > > > > > > > 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... > > > > > > > > hmm... Luis, any idea? > > > > > > > > > > > > > > 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. > > > > > > > > I was more concerned about code quality. Using raw pointers makes the code > > > more > > > > difficult to modify (and review) IMO. > > > > > > > > > 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 ?? > > > > > > > > Typically the object passed from the first callback to the second one is > > > > std::unique_ptr, which automatically prevents memory leaks. > > > > > > > > > > > > > > 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. > > > > Yes.. I remember I introduced CallbackRelay because I thought I can't use > > PostTaskAndReplyWithResult at that time. If using PostTaskAndReplyWithResult > > it's not needed. > > > > > https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... > > chrome/browser/chromeos/arc/arc_process_service.cc:93: auto ret_processes = > new > > vector<ArcProcess>(); > > On 2016/06/30 23:40:50, Yusuke Sato wrote: > > > > > 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 ?? > > > > > > > > Typically the object passed from the first callback to the second one is > > > > std::unique_ptr, which automatically prevents memory leaks. > > > > > > I think I misspoke. My understanding is that PostTaskAndReplyWithResult > > ensures > > > no memory leaks by forcing us to use RefCounted types. > > > > 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. > > Ah okay, you're right. I just got confused by the code. Either way, please try > to follow the Luis' suggestion. I am working toward the direction you guys mentioned. @cylee1: when do you have time we can talk f2f through the change.
Description was changed from ========== Show all system process in the task_manager. BUG=b/28434655 ========== to ========== Show all system process in the task_manager. BUG=623508 ==========
https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:48: public base::RefCountedThreadSafe<ArcProcessService> { This is a bit problematic, since RefCounted needs your destructor to be private to avoid the object going away while there are still active references around[1]. The problem is that making the destructor private makes it ineligible for using it in an unique_ptr :'( Here's my original suggestion: class ArcProcessService : ... { ... private: class ProcessList : public base::RefcountedThreadSafe<ProcessList> { public: std::vector<ArcProcess> UpdateAndReturnProcessList( const std::vector<ArcProcess>& raw_processes); private: ~ProcessList(); ... std::map<base::ProcessId, base::ProcessId> nspid_to_pid_; DISALLOW_COPY_AND_ASSIGN(ProcessList); }; scoped_refptr<ProcessList> process_list_; }; That way the outside class can still be wrapped in an unique_ptr and the |nspid_to_pid_| map is safely used in only one thread. 1: https://cs.chromium.org/chromium/src/base/memory/ref_counted.h?q=base::RefCou...
https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:107: // to get the processes below. For example, intalled might nit: installd https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:162: mojo::Array<arc::mojom::RunningAppProcessInfoPtr> mojo_processes) { nit: if possible, rename all the |mojo_processes| to |instance_processes|. https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:85: static std::vector<ArcProcess> GetArcSystemProcessList(); Prefer having these two in the anonymous namespace in the .cc file
The CQ bit was checked by hctsai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...)
On 2016/07/06 20:28:12, Luis Héctor Chávez wrote: > https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_process_service.cc (right): > > https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_process_service.cc:107: // to get > the processes below. For example, intalled might > nit: installd > > https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_process_service.cc:162: > mojo::Array<arc::mojom::RunningAppProcessInfoPtr> mojo_processes) { > nit: if possible, rename all the |mojo_processes| to |instance_processes|. > > https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... > File chrome/browser/chromeos/arc/arc_process_service.h (right): > > https://codereview.chromium.org/2025593003/diff/120001/chrome/browser/chromeo... > chrome/browser/chromeos/arc/arc_process_service.h:85: static > std::vector<ArcProcess> GetArcSystemProcessList(); > Prefer having these two in the anonymous namespace in the .cc file Removed pointers and migrated the calls to PostTaskAndReplyWithResult(), added refptr class for binding and move two functions into anonymous namespace. The files become quite different after the refactoring. PTAL.
https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:37: std::string process_name = I know you didn't write this code, but can you improve it to avoid unnecessary allocations? static constexpr char kInitName[] = "/init"; for (const base::ProcessEntry& entry : entry_list) { if (entry.cmd_line_args().empty()) continue; const std::string& process_name = entry.cmd_line_args()[0]; if (process_name == kInitName) return entry.pid(); } https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:95: // Not intended to be used from the creating thread. Remove? https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:114: base::Bind(&ArcProcessService::Reset, weak_ptr_factory_.GetWeakPtr())); This is still problematic, since WeakPtrs are not designed to be used across threads. Is there any particular reason you avoided using the solution I proposed (having a ref-counted class that owns exclusive access to |nspid_to_pid_|), which does not have this problem? Doing it that way will also avoid all the (*pid_map) and inheriting from an STL container. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:70: class NSPidToPidMap : public PidMap, Favor composition over inheritance. std::map also lacks virtual dtor. This is fine in your case since you never cast this to std::map, but still it's a sign that it should be avoided. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:86: static std::vector<ArcProcess> UpdateAndReturnProcessList( prefer having private static methods as standalone functions in the anonymous namespace in the .cc. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:50: std::map<base::ProcessId, std::unique_ptr<ArcProcessTask>>& pid_to_task, conding guidelines say that parameters should either be const-references or raw pointers. Make this a pointer since you need to modify it. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:59: set<ProcessId> nspid_to_remove; std::unordered_set? https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:66: std::unique_ptr<ArcProcessTask>& task = pid_to_task[entry.nspid()]; const? Same in L79 https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:136: auto callback = base::Bind(&ArcProcessTaskProvider::OnUpdateSystemProcessList, unused? https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:143: arc_process_service->RequestSystemProcessList( be consistent: either have both this and RequestAppProcessList() retry when arc_process_service->RequestXxxProcessList() fails or neither. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.h (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.h:67: std::map<base::ProcessId, std::unique_ptr<ArcProcessTask>> nspid_to_task_; Can these two be std::unordered_map?
https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:221: // In case the process already dies so couldn't find corresponding pid. Could you keep this comments or re-write it ? I meant to explain why it's possible that it is not in nspid_to_pid_ (nspid is returned but there's a race so the process had died when we update the map). https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:50: base::ProcessId arc_init_pid = GetArcInitProcessId(entry_list); nit: can be const https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:66: const std::string process_name = nit: can move inside the if https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:112: heavy_task_runner_->PostTask( Are you sure SingleThreadTaskRunner work as intended in a dedicated thread? To me it looks like an interface and require a subclass to "implement" its real functionality (e.g., create new thread). You can check its usage in the "subclass" and "instantiation" section of this class on cs.chromium.org . I'm happy to know if I were wrong. You can use https://cs.chromium.org/chromium/src/base/threading/platform_thread.h?rcl=0&l... to print current thread id to verify it. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:114: base::Bind(&ArcProcessService::Reset, weak_ptr_factory_.GetWeakPtr())); On 2016/07/15 16:19:41, Luis Héctor Chávez wrote: > This is still problematic, since WeakPtrs are not designed to be used across > threads. Is there any particular reason you avoided using the solution I > proposed (having a ref-counted class that owns exclusive access to > |nspid_to_pid_|), which does not have this problem? Doing it that way will also > avoid all the (*pid_map) and inheriting from an STL container. To be more clear, you should not use (dereference) a WeakPtr on another thread other than the creating thread. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:118: nspid_to_pid_->clear(); Shouldn't we only modify the object in the dedicated thread to avoid race ? https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:165: for (const auto& entry : *pid_map) { I think you can create a reference to the dereferenced pointer to avoid writing *pid_map multiple times. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:190: NSPidToPidMap& pid_map, const ? https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:70: class NSPidToPidMap : public PidMap, On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > Favor composition over inheritance. std::map also lacks virtual dtor. This is > fine in your case since you never cast this to std::map, but still it's a sign > that it should be avoided. agree https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:108: weak_ptr_factory_.GetWeakPtr())); I'm still hesitated that if you need WeakPtr since there's no thread switching. Unretained(this) is enough? The same for other places Also I don't understand why you need to pass in the scheduler since OnUpdateProcessList is a synchronous function ! Just OnUpdateProcessList(..,..); ScheduleNextAppRequest(); ? https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:143: arc_process_service->RequestSystemProcessList( On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > be consistent: either have both this and RequestAppProcessList() retry when > arc_process_service->RequestXxxProcessList() fails or neither. RequestSystemProcessList never fail however (returns void). It doesn't talk to Android via IPC so the failure case is difference ...
https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:143: arc_process_service->RequestSystemProcessList( On 2016/07/15 23:17:03, cylee1 wrote: > On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > > be consistent: either have both this and RequestAppProcessList() retry when > > arc_process_service->RequestXxxProcessList() fails or neither. > > RequestSystemProcessList never fail however (returns void). It doesn't talk to > Android via IPC so the failure case is difference ... oh sorry, didn't notice that. disregard this then.
The CQ bit was checked by hctsai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
can you run git cl format && git cl lint? https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:49: std::string process_name = entry.cmd_line_args()[0]; nit: const std::string& https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:81: const std::string process_name = entry.cmd_line_args()[0]; nit: const std::string& https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:112: std::set<ProcessId> visited; can all the std::sets be std::unordered_sets and std::maps be std::unordered_maps?
The CQ bit was checked by hctsai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks for the hint. I only know the cl format before. https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/100001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:221: // In case the process already dies so couldn't find corresponding pid. On 2016/07/15 23:17:02, cylee1 wrote: > Could you keep this comments or re-write it ? > I meant to explain why it's possible that it is not in nspid_to_pid_ (nspid is > returned but there's a race so the process had died when we update the map). Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:37: std::string process_name = On 2016/07/15 16:19:41, Luis Héctor Chávez wrote: > I know you didn't write this code, but can you improve it to avoid unnecessary > allocations? > > static constexpr char kInitName[] = "/init"; > > for (const base::ProcessEntry& entry : entry_list) { > if (entry.cmd_line_args().empty()) > continue; > const std::string& process_name = entry.cmd_line_args()[0]; > if (process_name == kInitName) > return entry.pid(); > } Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:50: base::ProcessId arc_init_pid = GetArcInitProcessId(entry_list); On 2016/07/15 23:17:03, cylee1 wrote: > nit: can be const Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:66: const std::string process_name = On 2016/07/15 23:17:03, cylee1 wrote: > nit: can move inside the if Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:95: // Not intended to be used from the creating thread. On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > Remove? Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:112: heavy_task_runner_->PostTask( On 2016/07/15 23:17:03, cylee1 wrote: > Are you sure SingleThreadTaskRunner work as intended in a dedicated thread? > To me it looks like an interface and require a subclass to "implement" its real > functionality (e.g., create new thread). You can check its usage in the > "subclass" and "instantiation" section of this class on http://cs.chromium.org . I'm > happy to know if I were wrong. > > You can use > https://cs.chromium.org/chromium/src/base/threading/platform_thread.h?rcl=0&l... > to print current thread id to verify it. Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:114: base::Bind(&ArcProcessService::Reset, weak_ptr_factory_.GetWeakPtr())); On 2016/07/15 23:17:03, cylee1 wrote: > On 2016/07/15 16:19:41, Luis Héctor Chávez wrote: > > This is still problematic, since WeakPtrs are not designed to be used across > > threads. Is there any particular reason you avoided using the solution I > > proposed (having a ref-counted class that owns exclusive access to > > |nspid_to_pid_|), which does not have this problem? Doing it that way will > also > > avoid all the (*pid_map) and inheriting from an STL container. > > To be more clear, you should not use (dereference) a WeakPtr on another thread > other than the creating thread. I did move on this way. The NSPidToPidMap class is added and the methods are bound to it. Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:118: nspid_to_pid_->clear(); On 2016/07/15 23:17:03, cylee1 wrote: > Shouldn't we only modify the object in the dedicated thread to avoid race ? Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:165: for (const auto& entry : *pid_map) { On 2016/07/15 23:17:03, cylee1 wrote: > I think you can create a reference to the dereferenced pointer to avoid writing > *pid_map multiple times. Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:190: NSPidToPidMap& pid_map, On 2016/07/15 23:17:03, cylee1 wrote: > const ? Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:70: class NSPidToPidMap : public PidMap, On 2016/07/15 23:17:03, cylee1 wrote: > On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > > Favor composition over inheritance. std::map also lacks virtual dtor. This is > > fine in your case since you never cast this to std::map, but still it's a sign > > that it should be avoided. > > agree Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:86: static std::vector<ArcProcess> UpdateAndReturnProcessList( On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > prefer having private static methods as standalone functions in the anonymous > namespace in the .cc. Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:50: std::map<base::ProcessId, std::unique_ptr<ArcProcessTask>>& pid_to_task, On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > conding guidelines say that parameters should either be const-references or raw > pointers. Make this a pointer since you need to modify it. Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:59: set<ProcessId> nspid_to_remove; On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > std::unordered_set? Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:66: std::unique_ptr<ArcProcessTask>& task = pid_to_task[entry.nspid()]; On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > const? Same in L79 'task' invokes reset() below https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:108: weak_ptr_factory_.GetWeakPtr())); On 2016/07/15 23:17:03, cylee1 wrote: > I'm still hesitated that if you need WeakPtr since there's no thread switching. > Unretained(this) is enough? > The same for other places > > Also I don't understand why you need to pass in the scheduler since > OnUpdateProcessList is a synchronous function ! Just > > OnUpdateProcessList(..,..); > ScheduleNextAppRequest(); > > ? Removed the scheduler (and also the WeakPtr) https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:136: auto callback = base::Bind(&ArcProcessTaskProvider::OnUpdateSystemProcessList, On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > unused? Done. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:143: arc_process_service->RequestSystemProcessList( On 2016/07/15 23:43:46, Luis Héctor Chávez wrote: > On 2016/07/15 23:17:03, cylee1 wrote: > > On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > > > be consistent: either have both this and RequestAppProcessList() retry when > > > arc_process_service->RequestXxxProcessList() fails or neither. > > > > RequestSystemProcessList never fail however (returns void). It doesn't talk to > > Android via IPC so the failure case is difference ... > > oh sorry, didn't notice that. disregard this then. Acknowledged. https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.h (right): https://codereview.chromium.org/2025593003/diff/180001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.h:67: std::map<base::ProcessId, std::unique_ptr<ArcProcessTask>> nspid_to_task_; On 2016/07/15 16:19:42, Luis Héctor Chávez wrote: > Can these two be std::unordered_map? Done. https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:49: std::string process_name = entry.cmd_line_args()[0]; On 2016/07/25 18:34:11, Luis Héctor Chávez wrote: > nit: const std::string& Done. https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:81: const std::string process_name = entry.cmd_line_args()[0]; On 2016/07/25 18:34:11, Luis Héctor Chávez wrote: > nit: const std::string& Done. https://codereview.chromium.org/2025593003/diff/200001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:112: std::set<ProcessId> visited; On 2016/07/25 18:34:11, Luis Héctor Chávez wrote: > can all the std::sets be std::unordered_sets and std::maps be > std::unordered_maps? Done.
lgtm
The CQ bit was checked by hctsai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:73: // TODO(hctsai): For now, we only gather direct child process of init, need nit: indent: just align with the first "T" of "TODO" ? https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:81: const std::string& process_name = entry.cmd_line_args()[0]; nit: what would happen if cmd_line_args() is empty ? https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:213: heavy_task_thread_.Stop(); nit: according to some previous code reviewer, it's better to keep those cleanup code in the reverse order of their initializing order (in case they have dependencies). So plz move this to line 211 https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:75: base::ProcessId& operator[](base::ProcessId&& key) { return pidmap_[key]; } curious: why do you need both versions at line 75 and line 72 ? https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:155: weak_ptr_factory_.GetWeakPtr()), I still think you don't need weak_ptr here since ScheduleNextRequest is a synchronous function. Unretained(this) will be fine.
The CQ bit was checked by hctsai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:81: const std::string& process_name = entry.cmd_line_args()[0]; On 2016/08/01 10:14:23, cylee1 wrote: > nit: what would happen if cmd_line_args() is empty ? the empty check is at the first step of the loop https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:213: heavy_task_thread_.Stop(); On 2016/08/01 10:14:23, cylee1 wrote: > nit: according to some previous code reviewer, it's better to keep those cleanup > code in the reverse order of their initializing order (in case they have > dependencies). > So plz move this to line 211 Done. https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.h (right): https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.h:75: base::ProcessId& operator[](base::ProcessId&& key) { return pidmap_[key]; } On 2016/08/01 10:14:23, cylee1 wrote: > curious: why do you need both versions at line 75 and line 72 ? Yes, you are right, line 75 is redundant. The use case here is for: (*pid_map)[nspid] = pid; The nspid here is of type "const ProcessId". Line 75 is removed. https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/task_ma... File chrome/browser/task_management/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/220001/chrome/browser/task_ma... chrome/browser/task_management/providers/arc/arc_process_task_provider.cc:155: weak_ptr_factory_.GetWeakPtr()), On 2016/08/01 10:14:23, cylee1 wrote: > I still think you don't need weak_ptr here since ScheduleNextRequest is a > synchronous function. Unretained(this) will be fine. Done.
lgtm
https://codereview.chromium.org/2025593003/diff/260001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:210: nspid_to_pid_(new NSPidToPidMap), nit: new NSPidToPidMap(). That might be the reason why the explicit ctor and dtor are needed. https://codereview.chromium.org/2025593003/diff/260001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:277: ArcProcessService::NSPidToPidMap::NSPidToPidMap() {} nit: = default; same below.
hctsai@chromium.org changed reviewers: + afakhry@chromium.org, georgesak@chromium.org
Hi afakhry@ and georgesak@,
Please help owner review for:
chrome/browser/memory/tab_manager_delegate_chromeos.cc
chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc
chrome/browser/task_manager/providers/arc/arc_process_task_provider.h
Thanks!
On 2016/08/24 09:37:27, hctsai wrote: > Hi afakhry@ and georgesak@, > > Please help owner review for: > chrome/browser/memory/tab_manager_delegate_chromeos.cc > chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc > chrome/browser/task_manager/providers/arc/arc_process_task_provider.h > > Thanks! Will look at this shortly. Thanks!
https://codereview.chromium.org/2025593003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:210: nspid_to_pid_(new NSPidToPidMap), ping: new NSPidtoPidMap(). https://codereview.chromium.org/2025593003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:277: ArcProcessService::NSPidToPidMap::NSPidToPidMap() {} ping: NSPidToPidMap() = default; same below.
https://codereview.chromium.org/2025593003/diff/280001/chrome/browser/chromeo... File chrome/browser/chromeos/arc/arc_process_service.cc (right): https://codereview.chromium.org/2025593003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:210: nspid_to_pid_(new NSPidToPidMap), On 2016/08/25 16:53:21, Luis Héctor Chávez wrote: > ping: new NSPidtoPidMap(). Done. https://codereview.chromium.org/2025593003/diff/280001/chrome/browser/chromeo... chrome/browser/chromeos/arc/arc_process_service.cc:277: ArcProcessService::NSPidToPidMap::NSPidToPidMap() {} On 2016/08/25 16:53:21, Luis Héctor Chávez wrote: > ping: NSPidToPidMap() = default; same below. Done.
https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc:155: base::Unretained(this)), You should use a weak_ptr here. https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc:162: base::Unretained(this)), here too.
https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc:155: base::Unretained(this)), On 2016/08/26 15:55:30, afakhry wrote: > You should use a weak_ptr here. I had discussed it with hctsai@. This is actually a synchronous call without thread switching. The use of Closure is solely to get a unified parameter type to accommodate both RequestAppProcessList() and RequestSystemProcessList(). IMHO it's a bit overkill to use WeakPtr here. An alternative might be passing a raw member function pointer like: typedef void (ArcProcessTaskProvider::*scheduler_func_ptr)(); void ArcProcessTaskProvider::ScheduleNextRequest( scheduler_func_ptr task, ......) WDYT?
https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc:155: base::Unretained(this)), On 2016/08/29 10:51:09, cylee1 wrote: > On 2016/08/26 15:55:30, afakhry wrote: > > You should use a weak_ptr here. > > I had discussed it with hctsai@. This is actually a synchronous call without > thread switching. The use of Closure is solely to get a unified parameter type > to accommodate both RequestAppProcessList() and RequestSystemProcessList(). IMHO > it's a bit overkill to use WeakPtr here. > Hmm, it's not really an overkill since you already have the weak_ptr_factory_ and we're already using WeakPtrs everywhere else. > An alternative might be passing a raw member function pointer like: > typedef void (ArcProcessTaskProvider::*scheduler_func_ptr)(); > void ArcProcessTaskProvider::ScheduleNextRequest( > scheduler_func_ptr task, ......) > > WDYT? > Can you guarantee that there won't be a future change in the synchronous call to post a reply to ArcProcessTaskProvider::RequestAppProcessList() after the instance is gone? I'm paranoid of subtle bugs like these :D
https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... File chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc (right): https://codereview.chromium.org/2025593003/diff/300001/chrome/browser/task_ma... chrome/browser/task_manager/providers/arc/arc_process_task_provider.cc:155: base::Unretained(this)), On 2016/08/30 16:37:57, afakhry wrote: > On 2016/08/29 10:51:09, cylee1 wrote: > > On 2016/08/26 15:55:30, afakhry wrote: > > > You should use a weak_ptr here. > > > > I had discussed it with hctsai@. This is actually a synchronous call without > > thread switching. The use of Closure is solely to get a unified parameter type > > to accommodate both RequestAppProcessList() and RequestSystemProcessList(). > IMHO > > it's a bit overkill to use WeakPtr here. > > > > Hmm, it's not really an overkill since you already have the weak_ptr_factory_ > and we're already using WeakPtrs everywhere else. > > > An alternative might be passing a raw member function pointer like: > > typedef void (ArcProcessTaskProvider::*scheduler_func_ptr)(); > > void ArcProcessTaskProvider::ScheduleNextRequest( > > scheduler_func_ptr task, ......) > > > > WDYT? > > > > Can you guarantee that there won't be a future change in the synchronous call to > post a reply to ArcProcessTaskProvider::RequestAppProcessList() after the > instance is gone? I'm paranoid of subtle bugs like these :D I'll respect to your decision.
Both make sense to me and I agree to use weakptr to avoid potential problems. Thanks for raising this issue.
LGTM for chrome/browser/memory/tab_manager_delegate_chromeos.cc
task_manager lgtm
The CQ bit was checked by hctsai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hctsai@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from lhchavez@chromium.org, cylee@chromium.org Link to the patchset: https://codereview.chromium.org/2025593003/#ps320001 (title: "Use weakptr to avoid potential issues. Fetch and rebase.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hctsai@google.com to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hctsai@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cylee@chromium.org, afakhry@chromium.org, lhchavez@chromium.org, georgesak@chromium.org Link to the patchset: https://codereview.chromium.org/2025593003/#ps340001 (title: "chrome-style complains 'Complex constructor has an inlined body.'")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by hctsai@google.com
The patchset sent to the CQ was uploaded after l-g-t-m from cylee@chromium.org, afakhry@chromium.org, lhchavez@chromium.org, georgesak@chromium.org Link to the patchset: https://codereview.chromium.org/2025593003/#ps360001 (title: "Remove redundant check.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Show all system process in the task_manager. BUG=623508 ========== to ========== Show all system process in the task_manager. BUG=623508 ==========
Message was sent while issue was closed.
Committed patchset #19 (id:360001)
Message was sent while issue was closed.
Description was changed from ========== Show all system process in the task_manager. BUG=623508 ========== to ========== Show all system process in the task_manager. BUG=623508 Committed: https://crrev.com/bb5e75574b94d73eb19209adad14d8d810fb7f98 Cr-Commit-Position: refs/heads/master@{#415918} ==========
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/bb5e75574b94d73eb19209adad14d8d810fb7f98 Cr-Commit-Position: refs/heads/master@{#415918} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
