Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1173263004:
[tracing] Send unique tracing process id for cross-process memory dumps identification. (Closed)
|
Created:
5 years, 6 months ago by ssid Modified:
5 years, 6 months ago CC:
chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, erikwright+watch_chromium.org, nasko+codewatch_chromium.org, tracing+reviews_chromium.org, wfh+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Send unique tracing process id for cross-process memory dumps identification.
The memory shared between browser and renderer must be identified with
the same unique id for memory dumps in tracing. Currently the child
process is not aware of its unique child process id. But, the child
process id cannot be exposed in the child process (see discussion in
crrev.com/1155683009).
So, a hash of the child process id allocated by the browser process is
sent to child process along with begin tracing message and will be
exposed in the child process MemoryDumpManager. The allocator that
needs to create a global guid will use the id to identify memory dumps.
See the design doc https://goo.gl/ncMfUV for more details.
BUG=497726
Committed: https://crrev.com/a3e7c30a16e711b59226da21f993522ee47ac674
Cr-Commit-Position: refs/heads/master@{#336210}
Patch Set 1 #
Total comments: 6
Patch Set 2 : Fixing nits. #
Total comments: 12
Patch Set 3 : Addressing comments. #Patch Set 4 : Nits. #Patch Set 5 : Rebase. #
Total comments: 7
Patch Set 6 : Using hash of the id and not sending id to child process. #Patch Set 7 : Nits. #
Total comments: 11
Patch Set 8 : Using constant instead of hash #
Total comments: 3
Patch Set 9 : adding comments. #
Total comments: 5
Patch Set 10 : Changing back to hash of id as jam suggested. #
Total comments: 1
Patch Set 11 : Using reinterpret_cast instead of ascii conversion before hashing. #
Total comments: 2
Patch Set 12 : added size arg for hash. #
Total comments: 6
Patch Set 13 : Fixed DCHECK. #Patch Set 14 : fixed InvalidTracingProcessId value. #Patch Set 15 : Fixing DCHECK while setting id. #
Total comments: 1
Messages
Total messages: 47 (10 generated)
ssid@chromium.org changed reviewers: + primiano@chromium.org
picksi@google.com changed reviewers: + picksi@google.com
https://codereview.chromium.org/1173263004/diff/1/base/trace_event/memory_all... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1173263004/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:17: const char kReplaceChildIdStr[] = "$$"; Random thought: Are we ever going to want to add other 'generated' strings here? If so '$$' is not very extensible. If we used e.g. {P} for Process ID we could easily extend it to {T} for timestamp (etc). https://codereview.chromium.org/1173263004/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:18: size_t pos = guid_str.find(kReplaceChildIdStr); Does this code need to work fast? If so we could add a magic first character (E.g. a '*') that flags that this string contains a "replaceable" item. So if guid_str[0] != '*' we can avoid the string search. https://codereview.chromium.org/1173263004/diff/1/components/tracing/tracing_... File components/tracing/tracing_messages.h (right): https://codereview.chromium.org/1173263004/diff/1/components/tracing/tracing_... components/tracing/tracing_messages.h:38: int /* Unique child process id */) Is Process ID really an int? Is it not a uint or something?
PTAL. https://codereview.chromium.org/1173263004/diff/1/base/trace_event/memory_all... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1173263004/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:17: const char kReplaceChildIdStr[] = "$$"; On 2015/06/16 16:18:37, picksi wrote: > Random thought: Are we ever going to want to add other 'generated' strings here? > If so '$$' is not very extensible. If we used e.g. {P} for Process ID we could > easily extend it to {T} for timestamp (etc). primiano@ WDYT? https://codereview.chromium.org/1173263004/diff/1/base/trace_event/memory_all... base/trace_event/memory_allocator_dump_guid.cc:18: size_t pos = guid_str.find(kReplaceChildIdStr); On 2015/06/16 16:18:37, picksi wrote: > Does this code need to work fast? If so we could add a magic first character > (E.g. a '*') that flags that this string contains a "replaceable" item. So if > guid_str[0] != '*' we can avoid the string search. hm, The string length is going to be small, so i don't think that the search will make a difference. Also this will add extra logic for each allocator to add *. https://codereview.chromium.org/1173263004/diff/1/components/tracing/tracing_... File components/tracing/tracing_messages.h (right): https://codereview.chromium.org/1173263004/diff/1/components/tracing/tracing_... components/tracing/tracing_messages.h:38: int /* Unique child process id */) On 2015/06/16 16:18:37, picksi wrote: > Is Process ID really an int? Is it not a uint or something? It is created as int by the existing code.
primiano@chromium.org changed reviewers: + jam@chromium.org
LGTM with some comments. +jam for OWNER review. John, this is essentially a re-write after the discussion in crrev.com/1155683009 https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:14: int unique_child_process_id = -1; nit: g_ prefix Add also a kInvalidChildProcessId = -1 (See below for the reason) https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:16: uint64 InsertChildIdAndHash(std::string guid_str) { nit: s/InsertChildIdAndHash/ExpandChildProcessIdAndHash/ https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:20: DCHECK_NE(unique_child_process_id, -1); use kInvalid... instead of -1 https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:43: unique_child_process_id = child_process_id; can you add a DCHECK(child_process_id_ == kInvalidChildProcessId || child_process_id == kInvalidChildProcessId) essentially this checks that the CPID can be either set or reset to invalid, but not changed. https://codereview.chromium.org/1173263004/diff/20001/components/tracing/chil... File components/tracing/child_trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/20001/components/tracing/chil... components/tracing/child_trace_message_filter.cc:71: base::trace_event::MemoryAllocatorDumpGuid::SetUniqueChildProcessId( Can you also clear this at OnEndTracing below? https://codereview.chromium.org/1173263004/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1173263004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:141: base::trace_event::MemoryAllocatorDumpGuid::SetUniqueChildProcessId( It is already invalid above, right? You shouldn't need to call this at all.
Made changes. Thanks https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:14: int unique_child_process_id = -1; On 2015/06/19 13:47:49, Primiano Tucci wrote: > nit: g_ prefix > Add also a kInvalidChildProcessId = -1 (See below for the reason) Done. https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:16: uint64 InsertChildIdAndHash(std::string guid_str) { On 2015/06/19 13:47:49, Primiano Tucci wrote: > nit: s/InsertChildIdAndHash/ExpandChildProcessIdAndHash/ Done. https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:20: DCHECK_NE(unique_child_process_id, -1); On 2015/06/19 13:47:49, Primiano Tucci wrote: > use kInvalid... instead of -1 Done. https://codereview.chromium.org/1173263004/diff/20001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:43: unique_child_process_id = child_process_id; On 2015/06/19 13:47:49, Primiano Tucci wrote: > can you add a > > DCHECK(child_process_id_ == kInvalidChildProcessId || child_process_id == > kInvalidChildProcessId) > essentially this checks that the CPID can be either set or reset to invalid, but > not changed. Done. https://codereview.chromium.org/1173263004/diff/20001/components/tracing/chil... File components/tracing/child_trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/20001/components/tracing/chil... components/tracing/child_trace_message_filter.cc:71: base::trace_event::MemoryAllocatorDumpGuid::SetUniqueChildProcessId( On 2015/06/19 13:47:50, Primiano Tucci wrote: > Can you also clear this at OnEndTracing below? Done. https://codereview.chromium.org/1173263004/diff/20001/content/browser/tracing... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/1173263004/diff/20001/content/browser/tracing... content/browser/tracing/tracing_controller_impl.cc:141: base::trace_event::MemoryAllocatorDumpGuid::SetUniqueChildProcessId( On 2015/06/19 13:47:50, Primiano Tucci wrote: > It is already invalid above, right? You shouldn't need to call this at all. Hm, I was planning to use maybe 0 for browser process id. But it was useless. Forgot to remove this.
ssid@chromium.org changed reviewers: + dcheng@chromium.org
+dcheng for 2 line change in components/tracing/tracing_messages.h
https://codereview.chromium.org/1173263004/diff/80001/base/trace_event/memory... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1173263004/diff/80001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:18: uint64 ExpandChildProcessIdAndHash(std::string guid_str) { const std::string& https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... File components/tracing/child_trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... components/tracing/child_trace_message_filter.cc:63: int child_process_id) { I'm a bit confused: it looks like in the previous issue, we didn't want to send the process ID to the child? Does this code run in the browser process?
https://codereview.chromium.org/1173263004/diff/80001/base/trace_event/memory... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1173263004/diff/80001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:18: uint64 ExpandChildProcessIdAndHash(std::string guid_str) { On 2015/06/20 00:36:27, dcheng wrote: > const std::string& Note: the string is mangled below. If he passes a const& string, than he'll need to create a copy anyways to mangle it. is it worth it the extra line? https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... File components/tracing/child_trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... components/tracing/child_trace_message_filter.cc:63: int child_process_id) { On 2015/06/20 00:36:27, dcheng wrote: > I'm a bit confused: it looks like in the previous issue, we didn't want to send > the process ID to the child? Does this code run in the browser process? In the previous issue turned out we didn't want to "expose" the child process id (because clients might cache it, use it for futher child->browser IPC, and somebody might end up relying and trusting that). The deal was to send it but never expose it (keep it internal to expand the $$ string). See John's comment on goo.gl/ncMfUV. (Not sure why ssid@ switched reviewer instead of keep following up with John who was in the loop in the first discussion.)
https://codereview.chromium.org/1173263004/diff/80001/base/trace_event/memory... File base/trace_event/memory_allocator_dump_guid.cc (right): https://codereview.chromium.org/1173263004/diff/80001/base/trace_event/memory... base/trace_event/memory_allocator_dump_guid.cc:18: uint64 ExpandChildProcessIdAndHash(std::string guid_str) { On 2015/06/20 12:32:31, Primiano Tucci wrote: > On 2015/06/20 00:36:27, dcheng wrote: > > const std::string& > > Note: the string is mangled below. If he passes a const& string, than he'll need > to create a copy anyways to mangle it. is it worth it the extra line? Yes, instead of creating a copy, i passed std::string. https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... File components/tracing/child_trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... components/tracing/child_trace_message_filter.cc:63: int child_process_id) { On 2015/06/20 12:32:31, Primiano Tucci wrote: > On 2015/06/20 00:36:27, dcheng wrote: > > I'm a bit confused: it looks like in the previous issue, we didn't want to > send > > the process ID to the child? Does this code run in the browser process? > > In the previous issue turned out we didn't want to "expose" the child process id > (because clients might cache it, use it for futher child->browser IPC, and > somebody might end up relying and trusting that). > The deal was to send it but never expose it (keep it internal to expand the $$ > string). > See John's comment on goo.gl/ncMfUV. > > (Not sure why ssid@ switched reviewer instead of keep following up with John who > was in the loop in the first discussion.) Yes, as primiano pointed out, the issue was only not to expose the id. Here instead of exposing it is stored at the file which requires the id and used directly. I added dcheng@ for the components/tracing/tracing_messages.h file, since jam@ wasn't the owner. I mentioned in comment :)
Patchset #6 (id:100001) has been deleted
https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... File components/tracing/child_trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... components/tracing/child_trace_message_filter.cc:63: int child_process_id) { On 2015/06/20 13:07:24, ssid wrote: > On 2015/06/20 12:32:31, Primiano Tucci wrote: > > On 2015/06/20 00:36:27, dcheng wrote: > > > I'm a bit confused: it looks like in the previous issue, we didn't want to > > send > > > the process ID to the child? Does this code run in the browser process? > > > > In the previous issue turned out we didn't want to "expose" the child process > id > > (because clients might cache it, use it for futher child->browser IPC, and > > somebody might end up relying and trusting that). > > The deal was to send it but never expose it (keep it internal to expand the $$ > > string). > > See John's comment on goo.gl/ncMfUV. > > > > (Not sure why ssid@ switched reviewer instead of keep following up with John > who > > was in the loop in the first discussion.) > > Yes, as primiano pointed out, the issue was only not to expose the id. Here > instead of exposing it is stored at the file which requires the id and used > directly. > > I added dcheng@ for the components/tracing/tracing_messages.h file, since jam@ > wasn't the owner. I mentioned in comment :) My reading of the previous discussion was that the ID wouldn't be sent to the child process, instead some hash. For the same reasons as I mentioned earlier, we shouldn't send the process id to the child process, even if it's not (currently) being exposed.
On 2015/06/22 16:20:14, jam wrote: > https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... > File components/tracing/child_trace_message_filter.cc (right): > > https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... > components/tracing/child_trace_message_filter.cc:63: int child_process_id) { > On 2015/06/20 13:07:24, ssid wrote: > > On 2015/06/20 12:32:31, Primiano Tucci wrote: > > > On 2015/06/20 00:36:27, dcheng wrote: > > > > I'm a bit confused: it looks like in the previous issue, we didn't want to > > > send > > > > the process ID to the child? Does this code run in the browser process? > > > > > > In the previous issue turned out we didn't want to "expose" the child > process > > id > > > (because clients might cache it, use it for futher child->browser IPC, and > > > somebody might end up relying and trusting that). > > > The deal was to send it but never expose it (keep it internal to expand the > $$ > > > string). > > > See John's comment on goo.gl/ncMfUV. > > > > > > (Not sure why ssid@ switched reviewer instead of keep following up with John > > who > > > was in the loop in the first discussion.) > > > > Yes, as primiano pointed out, the issue was only not to expose the id. Here > > instead of exposing it is stored at the file which requires the id and used > > directly. > > > > I added dcheng@ for the components/tracing/tracing_messages.h file, since jam@ > > wasn't the owner. I mentioned in comment :) > > My reading of the previous discussion was that the ID wouldn't be sent to the > child process, instead some hash. For the same reasons as I mentioned earlier, > we shouldn't send the process id to the child process, even if it's not > (currently) being exposed. I am sorry, I guess the doc wasn't clear. Is it fine if I send Hash(id) to child process?
On 2015/06/22 16:25:49, ssid wrote: > On 2015/06/22 16:20:14, jam wrote: > > > https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... > > File components/tracing/child_trace_message_filter.cc (right): > > > > > https://codereview.chromium.org/1173263004/diff/80001/components/tracing/chil... > > components/tracing/child_trace_message_filter.cc:63: int child_process_id) { > > On 2015/06/20 13:07:24, ssid wrote: > > > On 2015/06/20 12:32:31, Primiano Tucci wrote: > > > > On 2015/06/20 00:36:27, dcheng wrote: > > > > > I'm a bit confused: it looks like in the previous issue, we didn't want > to > > > > send > > > > > the process ID to the child? Does this code run in the browser process? > > > > > > > > In the previous issue turned out we didn't want to "expose" the child > > process > > > id > > > > (because clients might cache it, use it for futher child->browser IPC, and > > > > somebody might end up relying and trusting that). > > > > The deal was to send it but never expose it (keep it internal to expand > the > > $$ > > > > string). > > > > See John's comment on goo.gl/ncMfUV. > > > > > > > > (Not sure why ssid@ switched reviewer instead of keep following up with > John > > > who > > > > was in the loop in the first discussion.) > > > > > > Yes, as primiano pointed out, the issue was only not to expose the id. Here > > > instead of exposing it is stored at the file which requires the id and used > > > directly. > > > > > > I added dcheng@ for the components/tracing/tracing_messages.h file, since > jam@ > > > wasn't the owner. I mentioned in comment :) > > > > My reading of the previous discussion was that the ID wouldn't be sent to the > > child process, instead some hash. For the same reasons as I mentioned earlier, > > we shouldn't send the process id to the child process, even if it's not > > (currently) being exposed. > > I am sorry, I guess the doc wasn't clear. > Is it fine if I send Hash(id) to child process? yep, that's fine, thanks.
primiano@chromium.org changed reviewers: - dcheng@chromium.org, jam@chromium.org
https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:85: // Sets the unique hash value for identifying child process to create cross Don't expose the "hash" thing here. Just clarify that is neither the PID nor the child process ID See below https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:88: void set_tracing_process_id(int id) { Can you make the setter private and use the "friend backdoor"? so it is explicit that only the MDMDelegate should set it? The way you use the "backdoor" is as follows (see CreateProcessDump as a pattern): - make this setter a private: member of MDManger - Create a set_tracing_process_id method in the protected section MemoryDumpManagerDelegate, which calls MDM::set_tracing_process_id - Call the proxy MDMDelegate::set_tracing_process_id method from child_trace_message_filter https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:94: int tracing_process_id() { +const https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:95: DCHECK_NE(tracing_process_id_, kInvalidTracingProcessId); add a comment saying that if you are requesting this you might be out of the scope of a trace https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:157: // The unique id of the child process for tracing. The value is expected to Can you clarify that this is neither the PID nor the child process ID but a hash that can be obtained via ... https://codereview.chromium.org/1173263004/diff/140001/content/browser/tracin... File content/browser/tracing/trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/140001/content/browser/tracin... content/browser/tracing/trace_message_filter.cc:17: tracing_process_id_(base::Hash(base::IntToString(child_process_id))), This should be a method of MDM. Otherwise there is no way for the browser-side dumpers to construct a corresponding string. Also add a comment explaining why we are hashing and a pointer to the discussion and design doc. Introduce a public static int MemoryDumpManager::ChildProcessIdToTracingProcessId(int child_process_id) method.
(Am I still supposed to review this? This disappeared from my list of reviews, so I lost track of it for a few days.)
On 2015/06/23 21:56:21, dcheng wrote: > (Am I still supposed to review this? This disappeared from my list of reviews, > so I lost track of it for a few days.) I removed the OWNERS after jam's comment in #18, as ssid is reworking the CL and I didn't want to spam /content owners with internal reviews mails. Essentially this is going to pass a hash(cpid) instead of the child process id itself via IPC Will readd when this CL is ready again. Thanks :)
Made changes, PTAL https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:85: // Sets the unique hash value for identifying child process to create cross On 2015/06/23 13:35:20, Primiano Tucci wrote: > Don't expose the "hash" thing here. Just clarify that is neither the PID nor the > child process ID > See below hm, base/ shouldn't really know about child process id. It is content's implementation. That is why I din't mention that in the comment. I changed the comments now, WDYT? https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:88: void set_tracing_process_id(int id) { On 2015/06/23 13:35:20, Primiano Tucci wrote: > Can you make the setter private and use the "friend backdoor"? so it is explicit > that only the MDMDelegate should set it? > The way you use the "backdoor" is as follows (see CreateProcessDump as a > pattern): > > - make this setter a private: member of MDManger > - Create a set_tracing_process_id method in the protected section > MemoryDumpManagerDelegate, which calls MDM::set_tracing_process_id > - Call the proxy MDMDelegate::set_tracing_process_id method from > child_trace_message_filter Done. https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:94: int tracing_process_id() { On 2015/06/23 13:35:20, Primiano Tucci wrote: > +const Done. https://codereview.chromium.org/1173263004/diff/140001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:95: DCHECK_NE(tracing_process_id_, kInvalidTracingProcessId); On 2015/06/23 13:35:20, Primiano Tucci wrote: > add a comment saying that if you are requesting this you might be out of the > scope of a trace Done. https://codereview.chromium.org/1173263004/diff/140001/content/browser/tracin... File content/browser/tracing/trace_message_filter.cc (right): https://codereview.chromium.org/1173263004/diff/140001/content/browser/tracin... content/browser/tracing/trace_message_filter.cc:17: tracing_process_id_(base::Hash(base::IntToString(child_process_id))), On 2015/06/23 13:35:20, Primiano Tucci wrote: > This should be a method of MDM. Otherwise there is no way for the browser-side > dumpers to construct a corresponding string. > Also add a comment explaining why we are hashing and a pointer to the discussion > and design doc. > > Introduce a public static int > MemoryDumpManager::ChildProcessIdToTracingProcessId(int child_process_id) > method. Done.
LGTM with some comments. Re-add jam when done https://codereview.chromium.org/1173263004/diff/160001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/160001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:440: return Hash(IntToString(child_process_id)); I recently had very bad experiences with base::Hash (See my post on chromium-dev). Since here we want to guarantee that the id is just != child_process_id so doesn't get mistakenly misused, can you make this just 1000 + child_process_id ? https://codereview.chromium.org/1173263004/diff/160001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1173263004/diff/160001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:85: // This is used by browser to hash child id to get the tracing_process_id Add the following comment here // Derives a tracing process id from a child process id. Child process ids // cannot be used directly in tracing for security reasons (see: discussion in // crrev.com/xxxxxx). This method is meant to be used when dumping cross-process // shared memory from a process which knows the child process id of its // endpoints. The value returned by this method is guaranteed to be equal to // the value returned by tracing_process_id() in the corresponding child process. https://codereview.chromium.org/1173263004/diff/160001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:89: int tracing_process_id() const { Add a comment saying: // Returns a unique id for the current process. The id can be retrieved only // by child processes and only when tracing is enabled. This is intended to // express cross-process sharing of memory dumps on the child-process side, // without having to know the self child process id.
ssid@chromium.org changed reviewers: + dcheng@chromium.org, jam@chromium.org
PTAL now, I have changed the CL to send a hash of the child id.
Some drive-by comments from London to Bangalore! https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:438: return 1000 + child_process_id; Can we use a named const to explain the 1000, e.g. 'kPreventIDAliasingOffset' ... or whatever it is. https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:97: // without having to know the self child process id. 'self child' looks like a typo? Do you just mean child?
https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:438: return 1000 + child_process_id; why is this guessable? i.e. why not a hash?
On 2015/06/24 16:38:15, jam wrote: > https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... > File base/trace_event/memory_dump_manager.cc (right): > > https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... > base/trace_event/memory_dump_manager.cc:438: return 1000 + child_process_id; > why is this guessable? i.e. why not a hash? We recently realized that the base::Hash function is not totally robust. Please see http://crbug.com/503593 . So, we think this is safer.
updated to use hash again..
lgtm
LGTM, ship it once dcheng approves https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:438: return 1000 + child_process_id; On 2015/06/24 11:12:43, picksi wrote: > Can we use a named const to explain the 1000, e.g. 'kPreventIDAliasingOffset' > ... or whatever it is. +1 https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1173263004/diff/180001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:97: // without having to know the self child process id. On 2015/06/24 11:12:43, picksi wrote: > 'self child' looks like a typo? Do you just mean child? This means here "the child process id of the child process itself" :) https://codereview.chromium.org/1173263004/diff/200001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/200001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:440: return Hash(IntToString(child_process_id)); Ok for the Hash, but do: Hash(reinterpret_cast<const char*>(&child_process_id), sizeof(child_process_id)); as that will spread out the state space more. I tested it and got the first collision after 260k elements, which should be good enough. If you convert to ascii, the state space is too small and collides more.
https://codereview.chromium.org/1173263004/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:439: return Hash(reinterpret_cast<const char*>(&child_process_id)); Don't you need to pass a length here? This is going to call the const std::string& version since std::string is implicitly convertible from char*. std::string's implicit conversion is going to read through bytes until it finds a \0, which is not necessarily in [&child_process_id, &child_process_id + sizeof(child_process_id))
Sorry, done. https://codereview.chromium.org/1173263004/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:439: return Hash(reinterpret_cast<const char*>(&child_process_id)); On 2015/06/24 19:05:36, dcheng wrote: > Don't you need to pass a length here? This is going to call the const > std::string& version since std::string is implicitly convertible from char*. > std::string's implicit conversion is going to read through bytes until it finds > a \0, which is not necessarily in [&child_process_id, &child_process_id + > sizeof(child_process_id)) Done.
https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:439: return Hash(reinterpret_cast<const char*>(&child_process_id), What happens if this just happens to be -1? https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:99: DCHECK_NE(tracing_process_id_, kInvalidTracingProcessId); Convention is DCHECK_NE(expected, actual). The same is true of all other [D]CHECK macros that take two arguments.
https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:439: return Hash(reinterpret_cast<const char*>(&child_process_id), On 2015/06/24 20:34:06, dcheng wrote: > What happens if this just happens to be -1? Please see ChildProcessHost::kInvalidUniqueID, and the comment on GenerateChildProcessUniqueId(). It only generates id starting from 1 and increments, only positive values. So, id generated for a child process can't be -1, even in single-process mode. But this relies on the fact that kInvalidUniqueID is defined as -1! https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:99: DCHECK_NE(tracing_process_id_, kInvalidTracingProcessId); On 2015/06/24 20:34:06, dcheng wrote: > Convention is DCHECK_NE(expected, actual). The same is true of all other > [D]CHECK macros that take two arguments. Done.
https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:439: return Hash(reinterpret_cast<const char*>(&child_process_id), On 2015/06/25 at 02:35:49, ssid wrote: > On 2015/06/24 20:34:06, dcheng wrote: > > What happens if this just happens to be -1? > > Please see ChildProcessHost::kInvalidUniqueID, and the comment on GenerateChildProcessUniqueId(). It only generates id starting from 1 and increments, only positive values. So, id generated for a child process can't be -1, even in single-process mode. > But this relies on the fact that kInvalidUniqueID is defined as -1! I don't understand: how are you guaranteeing that no integer value of child_process_id hashes to -1?
Patchset #14 (id:280001) has been deleted
PTAL. https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/240001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:439: return Hash(reinterpret_cast<const char*>(&child_process_id), On 2015/06/25 02:45:39, dcheng wrote: > On 2015/06/25 at 02:35:49, ssid wrote: > > On 2015/06/24 20:34:06, dcheng wrote: > > > What happens if this just happens to be -1? > > > > Please see ChildProcessHost::kInvalidUniqueID, and the comment on > GenerateChildProcessUniqueId(). It only generates id starting from 1 and > increments, only positive values. So, id generated for a child process can't be > -1, even in single-process mode. > > But this relies on the fact that kInvalidUniqueID is defined as -1! > > I don't understand: how are you guaranteeing that no integer value of > child_process_id hashes to -1? I see what you mean now. Thanks I just realized I forgot to change the constant corresponding to the hash value. Fixed it now.
ipc changes lgtm
The CQ bit was checked by primiano@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from primiano@chromium.org, jam@chromium.org Link to the patchset: https://codereview.chromium.org/1173263004/#ps320001 (title: "Fixing DCHECK while setting id.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1173263004/320001
Message was sent while issue was closed.
Committed patchset #15 (id:320001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/a3e7c30a16e711b59226da21f993522ee47ac674 Cr-Commit-Position: refs/heads/master@{#336210}
Message was sent while issue was closed.
A revert of this CL (patchset #15 id:320001) has been created in https://codereview.chromium.org/1211883003/ by wittman@chromium.org. The reason for reverting is: Appears to regress sizes PERF_REGRESS: chrome-si/initializers (14.29%), chrome-si/initializers (14.29%) https://build.chromium.org/p/chromium/builders/Linux%20x64/builds/5461.
Message was sent while issue was closed.
https://codereview.chromium.org/1173263004/diff/320001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/1173263004/diff/320001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:138: MemoryDumpManager::ChildProcessIdToTracingProcessId(-1); FYI, this introduces a static initializer. Sorry I didn't catch this. |
