Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2772283003:
Make url_request_context dumps include context names (Closed)
|
Created:
3 years, 9 months ago by xunjieli Modified:
3 years, 8 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org, vmpstr+watch_chromium.org, wfh+watch_chromium.org, net-reviews_chromium.org, bnc+watch_chromium.org, tracing+reviews_chromium.org, jkarlin Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake url_request_context dumps include context names
Background dumps do not include URLRequestContext's names because
string attributes are not allowed. SlowReports only has background
dumps. Without the name of the context, we don't know where the contexts
come from.
This CL includes the context names in the names for the allocator dumps.
BUG=705053, 669108
Review-Url: https://codereview.chromium.org/2772283003
Cr-Commit-Position: refs/heads/master@{#460116}
Committed: https://chromium.googlesource.com/chromium/src/+/c4da1de6f90c7149e7b2e6db4ee66a84b355988a
Patch Set 1 #
Total comments: 8
Patch Set 2 : address comments #Patch Set 3 : self #Patch Set 4 : self #Patch Set 5 : use %s #Patch Set 6 : change filtering to avoid name clash #
Total comments: 2
Patch Set 7 : Address comments #Patch Set 8 : Revert back to an explicit list #Messages
Total messages: 48 (33 generated)
Description was changed from ========== temp BUG= ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attrs are not allowed. This makes data from SlowReports difficult to understand. This CL includes the context names in the names for the allocator dumps. BUG=705053 ==========
xunjieli@chromium.org changed reviewers: + jkarlin@chromium.org, ssid@chromium.org
Description was changed from ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attrs are not allowed. This makes data from SlowReports difficult to understand. This CL includes the context names in the names for the allocator dumps. BUG=705053 ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attrs are not allowed. This makes data from SlowReports difficult to understand. This CL includes the context names in the names for the allocator dumps. BUG=705053 ==========
Ssid@ : can I do sth like this?
Description was changed from ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attrs are not allowed. This makes data from SlowReports difficult to understand. This CL includes the context names in the names for the allocator dumps. BUG=705053 ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attrs are not allowed. This makes data from SlowReports difficult to understand. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053 ==========
Description was changed from ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attrs are not allowed. This makes data from SlowReports difficult to understand. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053 ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053 ==========
Description was changed from ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053 ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053,669108 ==========
The CQ bit was checked by xunjieli@chromium.org 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...
ssid@chromium.org changed reviewers: + primiano@chromium.org
Sure, we can get this kind of context. +primiano for owner. https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:76: "net/url_request_context/%s/0x?", I'd say just add all the strings here instead of stripping string of the known types. Other providers just list them here. Maybe worth changing this pattern, but for this cl lets be consistent. https://codereview.chromium.org/2772283003/diff/1/net/url_request/url_request... File net/url_request/url_request_context.cc (right): https://codereview.chromium.org/2772283003/diff/1/net/url_request/url_request... net/url_request/url_request_context.cc:149: name_.c_str(), reinterpret_cast<uintptr_t>(this)); Are we sure that we know all the possible names here? Do you mind changing the URLRequestContext::set_name to take const char* instead of std::string and |name_| also as const char*? set_name(std::string) would generally mean users can generate strings custom strings and we might not notice it. set_name(const char*) with a comment saying only string literals are allowed as names, would be future proof here.
Thanks, PTAL. https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:76: "net/url_request_context/%s/0x?", On 2017/03/27 18:43:15, ssid wrote: > I'd say just add all the strings here instead of stripping string of the known > types. > Other providers just list them here. Maybe worth changing this pattern, but for > this cl lets be consistent. Done. Though it feels a bit verbose to list all the permutations. https://codereview.chromium.org/2772283003/diff/1/net/url_request/url_request... File net/url_request/url_request_context.cc (right): https://codereview.chromium.org/2772283003/diff/1/net/url_request/url_request... net/url_request/url_request_context.cc:149: name_.c_str(), reinterpret_cast<uintptr_t>(this)); On 2017/03/27 18:43:15, ssid wrote: > Are we sure that we know all the possible names here? > Do you mind changing the URLRequestContext::set_name to take const char* instead > of std::string and |name_| also as const char*? > set_name(std::string) would generally mean users can generate strings custom > strings and we might not notice it. > set_name(const char*) with a comment saying only string literals are allowed as > names, would be future proof here. Done.
The CQ bit was checked by xunjieli@chromium.org 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...
Sorry, lgtm with the previous version of whitelist.cc change. Lets just use the "%s". Thanks! https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:76: "net/url_request_context/%s/0x?", On 2017/03/27 19:24:55, xunjieli wrote: > On 2017/03/27 18:43:15, ssid wrote: > > I'd say just add all the strings here instead of stripping string of the known > > types. > > Other providers just list them here. Maybe worth changing this pattern, but > for > > this cl lets be consistent. > > Done. Though it feels a bit verbose to list all the permutations. Ahh, sorry I see why you did this now. I thought it would add only 6 strings here. missed the combinations.
https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:188: const std::string names[] = {"system", "proxy", "app_request", nit: /s/std::string/const char* const/ s/names/kRequestNames/
xunjieli@chromium.org changed reviewers: + mmenke@chromium.org - jkarlin@chromium.org
+mmenke@: PTAL ProfileIOData and friends. Thank you. https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:76: "net/url_request_context/%s/0x?", On 2017/03/27 20:15:11, ssid wrote: > On 2017/03/27 19:24:55, xunjieli wrote: > > On 2017/03/27 18:43:15, ssid wrote: > > > I'd say just add all the strings here instead of stripping string of the > known > > > types. > > > Other providers just list them here. Maybe worth changing this pattern, but > > for > > > this cl lets be consistent. > > > > Done. Though it feels a bit verbose to list all the permutations. > > Ahh, sorry I see why you did this now. I thought it would add only 6 strings > here. missed the combinations. Done.
The CQ bit was checked by xunjieli@chromium.org 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...
Description was changed from ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053,669108 ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. SlowReports only has background dumps. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053,669108 ==========
Description was changed from ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. SlowReports only has background dumps. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053,669108 ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. SlowReports only has background dumps. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053,669108 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
profiles/ LGTM On 2017/03/27 20:27:15, xunjieli wrote: > +mmenke@: PTAL ProfileIOData and friends. Thank you. > > https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... > File base/trace_event/memory_infra_background_whitelist.cc (right): > > https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... > base/trace_event/memory_infra_background_whitelist.cc:76: > "net/url_request_context/%s/0x?", > On 2017/03/27 20:15:11, ssid wrote: > > On 2017/03/27 19:24:55, xunjieli wrote: > > > On 2017/03/27 18:43:15, ssid wrote: > > > > I'd say just add all the strings here instead of stripping string of the > > known > > > > types. > > > > Other providers just list them here. Maybe worth changing this pattern, > but > > > for > > > > this cl lets be consistent. > > > > > > Done. Though it feels a bit verbose to list all the permutations. > > > > Ahh, sorry I see why you did this now. I thought it would add only 6 strings > > here. missed the combinations. > > Done.
The CQ bit was checked by xunjieli@chromium.org 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...
ssid@: PTAL at diff between PS 4 & 5. I changed how the filtering is done in IsMemoryAllocatorDumpNameWhitelisted() by matching everything between the two slashes. There was a name clash between "main" and "main_media" in the previous approach which caused TracingBrowserTest.TestBackgroundMemoryInfra to fail on the bot.
https://codereview.chromium.org/2772283003/diff/100001/base/trace_event/memor... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/100001/base/trace_event/memor... base/trace_event/memory_infra_background_whitelist.cc:190: stripped_str.replace(replace_start, replace_end - pos, "%s"); No this is exactly what we are trying to avoid in this file. The reason why we whitelist is to avoid reporting PII data of the users. At some point in future if you decide to make the "name" of url_request as the url name, then this would end up uploading the url names which is a violation of the user privacy. The original patch did it correctly with all the possible strings listed here. Also please look at the nit i added later: https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... The issue with "main" and "main_media" can be solved by adding "main/" and "main_media/" I guess?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, PTAL. https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... base/trace_event/memory_infra_background_whitelist.cc:188: const std::string names[] = {"system", "proxy", "app_request", On 2017/03/27 20:26:02, ssid wrote: > nit: /s/std::string/const char* const/ > s/names/kRequestNames/ Done. https://codereview.chromium.org/2772283003/diff/100001/base/trace_event/memor... File base/trace_event/memory_infra_background_whitelist.cc (right): https://codereview.chromium.org/2772283003/diff/100001/base/trace_event/memor... base/trace_event/memory_infra_background_whitelist.cc:190: stripped_str.replace(replace_start, replace_end - pos, "%s"); On 2017/03/27 23:02:34, ssid wrote: > No this is exactly what we are trying to avoid in this file. The reason why we > whitelist is to avoid reporting PII data of the users. At some point in future > if you decide to make the "name" of url_request as the url name, then this would > end up uploading the url names which is a violation of the user privacy. > The original patch did it correctly with all the possible strings listed here. > Also please look at the nit i added later: > https://codereview.chromium.org/2772283003/diff/1/base/trace_event/memory_inf... > > The issue with "main" and "main_media" can be solved by adding "main/" and > "main_media/" I guess? Done. Just to clarify: URLRequestContext is the thing that makes URLRequests. The Context won't ever have a url attached to it.
The CQ bit was checked by xunjieli@chromium.org 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: This issue passed the CQ dry run.
Hmm sorry folks but I have to push back a bit on memory_infra_background_whitelist.cc as it's becoming a bit too hard to follow the various expansion and the syntax. I have no concerns about exposing the extra contexts, is just the "how" which is making this file IMHO a bit too odd. I am fine with: - just have the combinatorial explosion in the list to unblock this CL - come up with a more explicit way to express this whitelist. I drafted something here: https://gist.github.com/primiano/7bd2c53f8c3637c53c39f1f10a8750fe
On 2017/03/28 14:24:34, Primiano Tucci wrote: > Hmm sorry folks but I have to push back a bit on > memory_infra_background_whitelist.cc as it's becoming a bit too hard to follow > the various expansion and the syntax. > I have no concerns about exposing the extra contexts, is just the "how" which is > making this file IMHO a bit too odd. > I am fine with: > - just have the combinatorial explosion in the list to unblock this CL > - come up with a more explicit way to express this whitelist. I drafted > something here: > > https://gist.github.com/primiano/7bd2c53f8c3637c53c39f1f10a8750fe Done. Thanks. I have reverted back to using the explicit list. This CL is to investigate a possible memory performance problem seen in SlowReports (crbug.com/705053)
The CQ bit was checked by xunjieli@chromium.org 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 a lot, base/trace_event/memory_infra_background_whitelist.cc lgtm
The CQ bit was unchecked by xunjieli@chromium.org
The CQ bit was checked by xunjieli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ssid@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2772283003/#ps140001 (title: "Revert back to an explicit list")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 140001, "attempt_start_ts": 1490714361726380,
"parent_rev": "9c66d5a07a780764fa797115ee4b57b9f40e4f41", "commit_rev":
"c4da1de6f90c7149e7b2e6db4ee66a84b355988a"}
Message was sent while issue was closed.
Description was changed from ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. SlowReports only has background dumps. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053,669108 ========== to ========== Make url_request_context dumps include context names Background dumps do not include URLRequestContext's names because string attributes are not allowed. SlowReports only has background dumps. Without the name of the context, we don't know where the contexts come from. This CL includes the context names in the names for the allocator dumps. BUG=705053,669108 Review-Url: https://codereview.chromium.org/2772283003 Cr-Commit-Position: refs/heads/master@{#460116} Committed: https://chromium.googlesource.com/chromium/src/+/c4da1de6f90c7149e7b2e6db4ee6... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/c4da1de6f90c7149e7b2e6db4ee6... |
