Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
681733003:
Removed the ProcessHandle from the RendererClosedDetails payload (Closed)
|
Created:
6 years, 1 month ago by sramajay Modified:
6 years ago CC:
chromium-reviews, darin-cc_chromium.org, nasko+codewatch_chromium.org, jam, creis+watch_chromium.org, arunprasadr, no sievers, Alexei Svitkine (slow), rohitrao (ping after 24h) Base URL:
https://chromium.googlesource.com/chromium/src.git@master Project:
chromium Visibility:
Public. |
DescriptionRemove the ProcessHandle from the RendererClosedDetails payload
NOTIFICATION_RENDERER_PROCESS_CLOSED is sent out after the closing the render
process. ProcessHandle passed via this notification is invalid and useless. This
cl is to remove it from the notification payload.
MachBroker is relying on this notification to do clean-up of its map between
ProcessHandle and mach_port_t. Since ProcessHandle is invalid, there is a TODO
to handle it in proper way, which is also fixed as it is dependent.
Each Child Process (Renderer, Plugin) has its own id generated out of
ChildProcessHostImpl[1], which is a common unique id generator. Refer [2] for
documentation of this ID related to Render Process.
[1] https://code.google.com/p/chromium/codesearch#chromium/src/content/common/child_process_host_impl.cc&l=219&rcl=1415723309
[2] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/browser/render_process_host.h&sq=package:chromium&rcl=1415723309&l=141
Related CLs - https://codereview.chromium.org/644473004/ and https://codereview.chromium.org/662103002/
BUG=432486
Committed: https://crrev.com/52ac072bc8501be29ce92f8aac8343bfc525c851
Cr-Commit-Position: refs/heads/master@{#306407}
Patch Set 1 : Removed ProcessHandle from the RendererClosedDetails #Patch Set 2 : Fixed the mac unit test cases #Patch Set 3 : Fixed mac issues #
Total comments: 8
Patch Set 4 : Fixed review comments #
Total comments: 8
Patch Set 5 : Used range-based loop #
Total comments: 7
Patch Set 6 : Removed InvalidatePid and modified process id map #
Total comments: 8
Patch Set 7 : Optimized mach_map_ traversal #
Total comments: 2
Patch Set 8 : Corrected Indentation #Patch Set 9 : Fixed chromeos dependency #
Total comments: 7
Patch Set 10 : Updated child_process_id to child_process_host_id #
Total comments: 2
Patch Set 11 : Corrected spelling #Patch Set 12 : Fixed browser_test failures for chromeos #
Total comments: 12
Patch Set 13 : Fixed review comments #Messages
Total messages: 54 (16 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
sramajay@cisco.com changed reviewers: + asvitkine@chromium.org, creis@chromium.org, lazyboy@chromium.org, sievers@chromium.org
PTAL. Note- Macx64 is failing on PartitionAllocTest.RepeatedReturnNull, in which 6GB of memory allocation is expected to fail in the test case but it is actually passing. Please suggest what needs to be done for the test failure.
Thanks for following up here. You have a lot of reviewers listed, so please say what you want each reviewer to look at. I'm generally happy with removing the process handle from the notification if it's not reliable. I'm not a good reviewer for the Mac issue, though. Perhaps rohitrao@, whose TODO was there? https://codereview.chromium.org/681733003/diff/80001/content/browser/child_pr... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/681733003/diff/80001/content/browser/child_pr... content/browser/child_process_launcher.cc:7: #include <string> Is this necessary? https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... content/browser/mach_broker_mac.mm:276: for (ChildProcessIdMap::iterator it = child_process_id_map_.begin(); Why not have the map go the other way? Then you can look up the handle from child_process_id (without iterating), and erase it from the map by child_process_id after calling InvalidatePid. https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... File content/browser/mach_broker_mac_unittest.cc (right): https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... content/browser/mach_broker_mac_unittest.cc:54: AddPlaceholderForPid(1, 1); Please add tests for the new behavior you're adding as well. https://codereview.chromium.org/681733003/diff/80001/content/public/browser/r... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/681733003/diff/80001/content/public/browser/r... content/public/browser/render_process_host.h:8: #include <string> Is this necessary?
sramajay@cisco.com changed reviewers: + rohitrao@chromium.org - asvitkine@chromium.org, lazyboy@chromium.org, sievers@chromium.org
Reviewers list was based on last review and added Alexei as he was mac expert. Now, modified the reviewers list and added rohitrao. https://codereview.chromium.org/681733003/diff/80001/content/browser/child_pr... File content/browser/child_process_launcher.cc (right): https://codereview.chromium.org/681733003/diff/80001/content/browser/child_pr... content/browser/child_process_launcher.cc:7: #include <string> On 2014/11/10 19:57:25, Charlie Reis wrote: > Is this necessary? Done. https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... content/browser/mach_broker_mac.mm:276: for (ChildProcessIdMap::iterator it = child_process_id_map_.begin(); On 2014/11/10 19:57:25, Charlie Reis wrote: > Why not have the map go the other way? Then you can look up the handle from > child_process_id (without iterating), and erase it from the map by > child_process_id after calling InvalidatePid. InvalidatePid is called from other BrowserChildProcessHostDisconnected() and BrowserChildProcessCrashed(), in which ProcessHandle is passed on. If i change as per above comment, then iteration has to be done inside InvalidatePid(). Iteration cannot be avoided. https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... File content/browser/mach_broker_mac_unittest.cc (right): https://codereview.chromium.org/681733003/diff/80001/content/browser/mach_bro... content/browser/mach_broker_mac_unittest.cc:54: AddPlaceholderForPid(1, 1); On 2014/11/10 19:57:25, Charlie Reis wrote: > Please add tests for the new behavior you're adding as well. Done. https://codereview.chromium.org/681733003/diff/80001/content/public/browser/r... File content/public/browser/render_process_host.h (right): https://codereview.chromium.org/681733003/diff/80001/content/public/browser/r... content/public/browser/render_process_host.h:8: #include <string> On 2014/11/10 19:57:25, Charlie Reis wrote: > Is this necessary? Done.
Thanks for adding the test. A few comments below. Would it be possible to add a bug number for this? It's generally preferred to have one, and that helps with tracking when CLs get reverted (sometimes through no fault of their own). Also, did the Mac failures go away? Maybe they were unrelated, though I don't see a mac_x64_rel try job in the latest patch. https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac.mm:276: for (ChildProcessIdMap::iterator it = child_process_id_map_.begin(); Ok, but let's use the new range-based for loops now that we have them: http://chromium-cpp.appspot.com/ https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... File content/browser/mach_broker_mac_unittest.cc (right): https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac_unittest.cc:28: int ChildProcessMapCount(int child_process_id) { nit: GetChildProcessCount https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac_unittest.cc:49: // Add a placeholder for PID 1 and child process id 50 nit: End sentence with period, here and below. https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac_unittest.cc:85: EXPECT_EQ(100u, broker_.TaskForPid(1)); This line is just repeating the test above. It would be more effective to test this instead: EXPECT_EQ(1, GetChildProcessCount(50));
rohitrao@chromium.org changed reviewers: + mark@chromium.org
Adding Mark for the Mac parts, since I'm several years removed from this code.
sramajay@cisco.com changed reviewers: - rohitrao@chromium.org
Can you explain more about what this is doing, what the different IDs are, during what lifetimes they’re valid and guaranteed unique, etc.?
rsesek@chromium.org changed reviewers: + rsesek@chromium.org
On 2014/11/12 15:27:17, Mark Mentovai wrote: > Can you explain more about what this is doing, what the different IDs are, > during what lifetimes they’re valid and guaranteed unique, etc.? NOTIFICATION_RENDERER_PROCESS_CLOSED is sent out after the closing the render process. ProcessHandle passed via this notification is invalid & useless. This cl is to remove it from the notification payload. MachBroker is relying on this notification to do clean-up of its map between ProcessHandle and mach_port_t. Since ProcessHandle is invalid, there is a TODO to handle it in proper way, which is also fixed as it is dependent. Regarding the ID, each Child Process (Renderer, Plugin) has its own id generated out of ChildProcessHostImpl[1], which is a common unique id generator. Refer [2] for documentation of this ID related to Render Process. P.S - I am investigating the reason for the failure of mach_x64_rel (looks not related to this patch) [1] https://code.google.com/p/chromium/codesearch#chromium/src/content/common/chi... [2] https://code.google.com/p/chromium/codesearch#chromium/src/content/public/bro...
Incorporated the comemnts. mac_x64_rel is failing on wtf_unittests.RepeatedReturnNull. It is passing locally in Mac i5 (x64). Unable to figure out the reason for the failure in Try server. Any clue? https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac.mm:276: for (ChildProcessIdMap::iterator it = child_process_id_map_.begin(); On 2014/11/12 00:44:19, Charlie Reis wrote: > Ok, but let's use the new range-based for loops now that we have them: > http://chromium-cpp.appspot.com/ Done. https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... File content/browser/mach_broker_mac_unittest.cc (right): https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac_unittest.cc:28: int ChildProcessMapCount(int child_process_id) { On 2014/11/12 00:44:19, Charlie Reis wrote: > nit: GetChildProcessCount Done. https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac_unittest.cc:49: // Add a placeholder for PID 1 and child process id 50 On 2014/11/12 00:44:19, Charlie Reis wrote: > nit: End sentence with period, here and below. Done. https://codereview.chromium.org/681733003/diff/100001/content/browser/mach_br... content/browser/mach_broker_mac_unittest.cc:85: EXPECT_EQ(100u, broker_.TaskForPid(1)); On 2014/11/12 00:44:19, Charlie Reis wrote: > This line is just repeating the test above. It would be more effective to test > this instead: > EXPECT_EQ(1, GetChildProcessCount(50)); Done.
https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... content/browser/mach_broker_mac.mm:217: InvalidatePid(data.handle); Why don’t you get rid of InvalidatePid and use InvalidateChildProcessId everywhere? data.id carries the child process ID. Line 221 too. https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... content/browser/mach_broker_mac.mm:276: for (const auto& it : child_process_id_map_) { You need to assert that the lock is held to be able to walk this map, but that won’t work because the lock isn’t taken until InvalidatePid(). https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... content/browser/mach_broker_mac.mm:277: if (it.second == child_process_id) { It seems like you really want a bidirectional map, but since we don’t have that, two maps? But if you restructure things to only have InvalidateChildProcessId and get rid of InvalidatePid as suggested, you can probably do this with a single map.
https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... content/browser/mach_broker_mac.mm:217: InvalidatePid(data.handle); On 2014/11/13 14:00:10, Mark Mentovai wrote: > Why don’t you get rid of InvalidatePid and use InvalidateChildProcessId > everywhere? data.id carries the child process ID. Line 221 too. Acknowledged. https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... content/browser/mach_broker_mac.mm:276: for (const auto& it : child_process_id_map_) { On 2014/11/13 14:00:10, Mark Mentovai wrote: > You need to assert that the lock is held to be able to walk this map, but that > won’t work because the lock isn’t taken until InvalidatePid(). Ok. I can remove the InvalidatePid with the above mentioned change, in which case i can apply the lock before using the map. https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... content/browser/mach_broker_mac.mm:277: if (it.second == child_process_id) { On 2014/11/13 14:00:10, Mark Mentovai wrote: > It seems like you really want a bidirectional map, but since we don’t have that, > two maps? > > But if you restructure things to only have InvalidateChildProcessId and get rid > of InvalidatePid as suggested, you can probably do this with a single map. Its not only InvalidatePid(), but FinalizePid() is called with ProcessHandle & mach_prot_t obtained from the kernel, for which we need the ProcessHandle in the map. We can't have a single map. Your comments please.
https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/120001/content/browser/mach_br... content/browser/mach_broker_mac.mm:277: if (it.second == child_process_id) { On 2014/11/13 15:45:28, sramajay wrote: > On 2014/11/13 14:00:10, Mark Mentovai wrote: > > It seems like you really want a bidirectional map, but since we don’t have > that, > > two maps? > > > > But if you restructure things to only have InvalidateChildProcessId and get > rid > > of InvalidatePid as suggested, you can probably do this with a single map. > > Its not only InvalidatePid(), but FinalizePid() is called with ProcessHandle & > mach_prot_t obtained from the kernel, for which we need the ProcessHandle in the > map. We can't have a single map. Your comments please. Then it sounds like we should have a map from PID to child process ID to be used by FinalizePid and TaskForPid, and a map from child process ID to {mach_port_t, pid}. I don’t see a good reason to have two parallel maps that share the same key, which is what your change has done. Use a structure for the value type instead. On the other hand, if you do need to access things using a child process ID key sometimes and a PID key other times, it’s reasonable to have a map deal with that for you, rather than manually iterating over a single map’s values.
Patchset #6 (id:140001) has been deleted
After removing InvalidatePid() and by swapping the ChildProcessIdMap, I am able to handle all scenarios. Please take a look.
https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... File content/browser/mach_broker_mac.h (right): https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.h:118: // Mutex that guards |mach_map_|. It also guards child_process_id_map_. https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.mm:118: base::AutoLock lock(broker_->GetLock()); I would move this lock into FinalizePid(), replacing the lock_.AssertAcquired() in that function. If you do this, also adjust the comment in the header file for FinalizePid(). https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.mm:264: child_process_id_map_.find(child_process_id); Continuation lines get a 4-space indent, not a 2-space one. https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.mm:269: kern_return_t kr = mach_port_deallocate(mach_task_self(), You should use mach_map_.find() and handle the case where the PID isn’t in mach_map_ by skipping the mach_port_deallocate() and mach_map_.erase(). Since you’ll have an iterator, you can also use it with the erase() on line 272, instead of having to do the same map lookup twice. It will happen that you’ll get an entry for child_process_id in child_process_id_map_ but no entry for the pid in mach_map_ if FinalizePid() doesn’t get called before the child process dies.
PTAL https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... File content/browser/mach_broker_mac.h (right): https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.h:118: // Mutex that guards |mach_map_|. On 2014/11/17 19:17:46, Mark Mentovai wrote: > It also guards child_process_id_map_. Done. https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.mm:118: base::AutoLock lock(broker_->GetLock()); On 2014/11/17 19:17:46, Mark Mentovai wrote: > I would move this lock into FinalizePid(), replacing the lock_.AssertAcquired() > in that function. If you do this, also adjust the comment in the header file for > FinalizePid(). I would take this suggestion as a new CL as it is not only FinalizePid() but it is the same for even other methods AddPlaceholderFodPid() and EnsureRunning(). And more over this change is a standalone enhancement. https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.mm:264: child_process_id_map_.find(child_process_id); On 2014/11/17 19:17:46, Mark Mentovai wrote: > Continuation lines get a 4-space indent, not a 2-space one. Done. https://codereview.chromium.org/681733003/diff/160001/content/browser/mach_br... content/browser/mach_broker_mac.mm:269: kern_return_t kr = mach_port_deallocate(mach_task_self(), On 2014/11/17 19:17:46, Mark Mentovai wrote: > You should use mach_map_.find() and handle the case where the PID isn’t in > mach_map_ by skipping the mach_port_deallocate() and mach_map_.erase(). Since > you’ll have an iterator, you can also use it with the erase() on line 272, > instead of having to do the same map lookup twice. > > It will happen that you’ll get an entry for child_process_id in > child_process_id_map_ but no entry for the pid in mach_map_ if FinalizePid() > doesn’t get called before the child process dies. Done.
https://codereview.chromium.org/681733003/diff/180001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/180001/content/browser/mach_br... content/browser/mach_broker_mac.mm:270: kern_return_t kr = mach_port_deallocate(mach_task_self(), Indentation in this block.
Patchset #8 (id:200001) has been deleted
PTAL https://codereview.chromium.org/681733003/diff/180001/content/browser/mach_br... File content/browser/mach_broker_mac.mm (right): https://codereview.chromium.org/681733003/diff/180001/content/browser/mach_br... content/browser/mach_broker_mac.mm:270: kern_return_t kr = mach_port_deallocate(mach_task_self(), On 2014/11/18 14:20:19, Mark Mentovai wrote: > Indentation in this block. Done.
LGTM
The CQ bit was checked by sramajay@cisco.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/681733003/220001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
On 2014/11/18 14:56:17, Mark Mentovai wrote:
> LGTM
Presubmit is failing with below message, Could you let me know what is the
issue?
"Missing LGTM from an OWNER for these files:"
You need an LG from a reviewer listed in content/OWNERS, like creis.
Patchset #9 (id:240001) has been deleted
I have missed the ChromeOS changes for the ProcessHandle removal. Fixed now. Please take a look.
creis@chromium.org changed reviewers: + jamescook@chromium.org
Thanks for sticking with this. content/ LGTM, but you'll need an owner to review chrome/browser/chromeos/memory. Perhaps jamescook@?
What is the overall goal of this change? It's not clear to me from the commit message or the linked bug. Is it fixing an issue? Refactoring for some other issue?
On 2014/11/19 18:55:51, James Cook wrote: > What is the overall goal of this change? It's not clear to me from the commit > message or the linked bug. Is it fixing an issue? Refactoring for some other > issue? I have updated the commit message with more details. It is a clean-up to remove the ProcessHandle from the RendererClosedDetails payload of NOTIFICATION_RENDERER_PROCESS_CLOSED. ProcessHandle will be invalid during this notification. This has been changed with https://codereview.chromium.org/613173004 CL. This CL removes ProcessHandle payload and moves all the dependency to rely on the Child Process Id (which is a unique id generated for each process). Oom relies on this notification, so instead of depending of the ProcessHandle, i am changing it to Child Process Id.
https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:82: int child_process_id; I'm confused. This code only runs on Posix, where process_handle.h says that base::ProcessHandle and base::ProcessId are the same thing. Is this |child_process_id| referring to a different process than the one from |renderer_handle|? Do you need both of these? Or is |child_process_id| not a pid? If that's the case then the name is super-confusing. Perhaps it should be something like child_host_id or child_process_host_id. https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:115: // Returns a list of child process ids from |stats_list| which unique pids. which -> with? which are? https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:141: // This lock is for pid_to_oom_score_ and focus_tab_pid_. Fix this comment, or comment on the individual member variables which lock protects them.
https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:82: int child_process_id; On 2014/11/20 16:50:35, James Cook wrote: > I'm confused. This code only runs on Posix, where process_handle.h says that > base::ProcessHandle and base::ProcessId are the same thing. Is this > |child_process_id| referring to a different process than the one from > |renderer_handle|? Do you need both of these? > > Or is |child_process_id| not a pid? If that's the case then the name is > super-confusing. Perhaps it should be something like child_host_id or > child_process_host_id. Note: It's pretty common in the browser process to refer to the RenderProcessHost ID as "process_id," for better or worse. That said, in cases like this where it's used alongside the ProcessHandle, it would be less ambiguous to call it child_process_host_id.
PTAL https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:82: int child_process_id; On 2014/11/20 17:07:45, Charlie Reis wrote: > On 2014/11/20 16:50:35, James Cook wrote: > > I'm confused. This code only runs on Posix, where process_handle.h says that > > base::ProcessHandle and base::ProcessId are the same thing. Is this > > |child_process_id| referring to a different process than the one from > > |renderer_handle|? Do you need both of these? > > > > Or is |child_process_id| not a pid? If that's the case then the name is > > super-confusing. Perhaps it should be something like child_host_id or > > child_process_host_id. > > Note: It's pretty common in the browser process to refer to the > RenderProcessHost ID as "process_id," for better or worse. That said, in cases > like this where it's used alongside the ProcessHandle, it would be less > ambiguous to call it child_process_host_id. child_process_id is not a pid, it is just a unique auto generated number given to all child processes including plugin process. Yes. It makes sense to rename it as child_process_host_id. https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:115: // Returns a list of child process ids from |stats_list| which unique pids. On 2014/11/20 16:50:35, James Cook wrote: > which -> with? which are? Done. https://codereview.chromium.org/681733003/diff/260001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:141: // This lock is for pid_to_oom_score_ and focus_tab_pid_. On 2014/11/20 16:50:35, James Cook wrote: > Fix this comment, or comment on the individual member variables which lock > protects them. Done.
LGTM with nit. Apologies for the delay in reviewing this. https://codereview.chromium.org/681733003/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/681733003/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:147: // Holds the focussed tab's child process host id. nit: focussed -> focused
https://codereview.chromium.org/681733003/diff/280001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/681733003/diff/280001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:147: // Holds the focussed tab's child process host id. On 2014/11/22 00:38:46, James Cook wrote: > nit: focussed -> focused Done.
The CQ bit was checked by sramajay@cisco.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/681733003/300001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
browser_tests were failing because RenderProcessHost::FromID has been called from FILE Thread. In all the instances, we are having access to both child process host id and ProcessHandle. So using a pair to hold both. PTAL.
LGTM with nits. Nice patch. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.cc (right): https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.cc:461: focused_tab_process_info_.second, chrome::kLowestRendererOomScore); nit: Please pull this out into a local variable (like "pid" or "process_handle" so the reader is reminded what "second" refers to. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.cc:498: it = oom_score_map_.find(focused_tab_process_info_.first); This is fine because the lines above show that first means. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.cc:645: for (auto process_info : process_infos) { nit: consider "const auto&" here to avoid copying each pair. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:119: // Returns a list of child process host ids and ProcessHandle from nit: ProcessHandles https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:121: // returns the first child process host id and correspoding pid. This implies nit: corresponding https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager_unittest.cc (right): https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager_unittest.cc:179: stats.child_process_host_id = 100; nit: This would be better if all the numbers were unique, like 100/101 for the first pair and 200/201 for the second pair.
https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.cc (right): https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.cc:461: focused_tab_process_info_.second, chrome::kLowestRendererOomScore); On 2014/12/01 19:25:58, James Cook wrote: > nit: Please pull this out into a local variable (like "pid" or "process_handle" > so the reader is reminded what "second" refers to. Done. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.cc:498: it = oom_score_map_.find(focused_tab_process_info_.first); On 2014/12/01 19:25:58, James Cook wrote: > This is fine because the lines above show that first means. Acknowledged. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.cc:645: for (auto process_info : process_infos) { On 2014/12/01 19:25:58, James Cook wrote: > nit: consider "const auto&" here to avoid copying each pair. Done. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager.h (right): https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:119: // Returns a list of child process host ids and ProcessHandle from On 2014/12/01 19:25:58, James Cook wrote: > nit: ProcessHandles Done. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager.h:121: // returns the first child process host id and correspoding pid. This implies On 2014/12/01 19:25:58, James Cook wrote: > nit: corresponding Done. https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... File chrome/browser/chromeos/memory/oom_priority_manager_unittest.cc (right): https://codereview.chromium.org/681733003/diff/320001/chrome/browser/chromeos... chrome/browser/chromeos/memory/oom_priority_manager_unittest.cc:179: stats.child_process_host_id = 100; On 2014/12/01 19:25:58, James Cook wrote: > nit: This would be better if all the numbers were unique, like 100/101 for the > first pair and 200/201 for the second pair. Done.
The CQ bit was checked by sramajay@cisco.com
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/681733003/340001
Message was sent while issue was closed.
Committed patchset #13 (id:340001)
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/52ac072bc8501be29ce92f8aac8343bfc525c851 Cr-Commit-Position: refs/heads/master@{#306407} |
