Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2041583003:
[tracing] Introduce "allowed_dump_modes" for memory dump config (Closed)
|
Created:
4 years, 6 months ago by ssid Modified:
4 years, 6 months ago CC:
chromium-reviews, tracing+reviews_chromium.org, wfh+watch_chromium.org, jam, darin-cc_chromium.org, caseq Base URL:
https://chromium.googlesource.com/chromium/src.git@background_config Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[tracing] Introduce "allowed_dump_modes" for memory dump config
The "allowed_dump_modes" contains a list of dump modes that are allowed
for memory dumps in the current session. This is to prevent any other
components from triggering explicit dumps with unexpected details. Trace
config now controls the modes allowed. If not mentioned then by default
all the modes are allowed.
BUG=617208
TBR=oysteine@chromium.org
Committed: https://crrev.com/40638de0bd1dbd66662c09b95bdd1df09ab9258c
Cr-Commit-Position: refs/heads/master@{#401382}
Patch Set 1 : #Patch Set 2 : #
Total comments: 7
Patch Set 3 : Use session state #Patch Set 4 : rebase on 2049143002 #
Total comments: 14
Patch Set 5 : Fixes and rebase. #
Total comments: 4
Patch Set 6 : Keep TracingController check. Added comment and bulid fix. #Patch Set 7 : Add check #Patch Set 8 : rebase. #
Total comments: 10
Patch Set 9 : fix enum order. #Patch Set 10 : Change mode to int. #Dependent Patchsets: Messages
Total messages: 51 (18 generated)
Description was changed from ========== [tracing] Introduce a allowed_dump_modes for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components form triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. BUG=617208 ========== to ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components form triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. BUG=617208 ==========
Description was changed from ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components form triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. BUG=617208 ========== to ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components from triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. If not mentioned then by default all the modes are allowed. BUG=617208 ==========
Patchset #1 (id:1) has been deleted
ssid@chromium.org changed reviewers: + primiano@chromium.org
ptal, thanks
Thanks for this. Some comments below. As a general comment, can we make the set logic such as if the set if empty (which is the default) all modes are allowed, instead you restrict if not empty. This would avoid to put the knowledge of the modes in the trace config ctor. https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:120: return allowed_dump_modes_.find(dump_mode) != allowed_dump_modes_.end(); - Let's move this to the .cc file, there is no need to inline this. - Can we make this such as: if the set if empty, then everything is allowed, otherwise only what's in the set? - I think .count(dump_mode) != 0 is a bit more legible and probably even more performant, as it doesn't have to return an iterator. https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:283: explicit PeriodicGlobalDumpTimer(MemoryDumpManager* mdm); uh why cannot we rely on MDM being a singleton? https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:362: std::set<MemoryDumpLevelOfDetail> allowed_dump_modes_; Can me put ths into the SessionState. Every time in the past we tried to add state to MDM we ended up with some bug on corner cases when stopping/starting trace. With SessionState we know that everything is inside there, so either all the session state logic is buggy or not. (And when I say "we ended up in bug" I mean mostly code that I wrote myself :) ) Actually wait, it's already there: the sessionstate contains a copy of the traceconfig. The traceconfig contains the allowed dump modes.
On 2016/06/08 15:48:30, Primiano Tucci wrote: > Thanks for this. Some comments below. > As a general comment, can we make the set logic such as if the set if empty > (which is the default) all modes are allowed, instead you restrict if not empty. > This would avoid to put the knowledge of the modes in the trace config ctor. > I think this logic should be part of trace config and is already in place. MDM just reads config and does not do additional checks. https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:120: return allowed_dump_modes_.find(dump_mode) != allowed_dump_modes_.end(); On 2016/06/08 15:48:30, Primiano Tucci wrote: > - Let's move this to the .cc file, there is no need to inline this. Fixed. > - Can we make this such as: if the set if empty, then everything is allowed, > otherwise only what's in the set? This is already done in trace_config file where this kind of default behavior is set for periodic dumps. Please see comment in trace_config.cc#545. > - I think .count(dump_mode) != 0 is a bit more legible and probably even more > performant, as it doesn't have to return an iterator. Done. https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:283: explicit PeriodicGlobalDumpTimer(MemoryDumpManager* mdm); On 2016/06/08 15:48:30, Primiano Tucci wrote: > uh why cannot we rely on MDM being a singleton? I did not want to do MDM::GetInstance() multiple times at each timer call also makes code longer . If you prefer keeping Get calls here, i will remove this change. https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:362: std::set<MemoryDumpLevelOfDetail> allowed_dump_modes_; On 2016/06/08 15:48:30, Primiano Tucci wrote: > Can me put ths into the SessionState. Every time in the past we tried to add > state to MDM we ended up with some bug on corner cases when stopping/starting > trace. With SessionState we know that everything is inside there, so either all > the session state logic is buggy or not. > (And when I say "we ended up in bug" I mean mostly code that I wrote myself :) ) > > Actually wait, it's already there: the sessionstate contains a copy of the > traceconfig. The traceconfig contains the allowed dump modes. Sorry, fixed this! I should be reading session state here.
Patchset #4 (id:80001) has been deleted
https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2041583003/diff/40001/base/trace_event/memory... base/trace_event/memory_dump_manager.h:283: explicit PeriodicGlobalDumpTimer(MemoryDumpManager* mdm); On 2016/06/08 18:04:17, ssid wrote: > On 2016/06/08 15:48:30, Primiano Tucci wrote: > > uh why cannot we rely on MDM being a singleton? > > I did not want to do MDM::GetInstance() multiple times at each timer call also > makes code longer . If you prefer keeping Get calls here, i will remove this > change. GetInstance is pretty fast and generally nothing compared to the cost of all the other things we do to create our dumps. I think it just makes it the code easier to read. https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:716: return session_state_->memory_dump_config().allowed_dump_modes.count( - Take the AutoLock lock(lock_); session_state is always accessed under the lock. Don't worry about RequestGlobalMemoryDump locking twice, It's ok, once you hit this you are out of the fast-path (we established we are going to spend time doing a dump, in which case a lock is not influent) - You need a if(!session_state) return false, otherwise this will crash if you call this outside of a tracing session https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:83: std::set<MemoryDumpLevelOfDetail> modes(kDefaultAllowedDumpModes, Hmm I'll make this a bit more self-maintaining and do: std::set<MemoryDumpLevelOfDetail> all_modes; for (auto mode = MemoryDumpLevelOfDetails::FIRST; mode <= ..::LAST; mode = static_cast<LevelOfDetail>(static_cast<uint32_t>(mode) + 1)) { all_modes.add(i) } I know that the double cast is now awesome, but better than having to remember to edit two places. https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:86: DCHECK_EQ(static_cast<uint32_t>(MemoryDumpLevelOfDetail::LAST), at which point you get rid of the dcheck https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.cc:417: base::trace_event::TraceConfig::MemoryDumpConfig()); ehm don't you have to pass memory_config here instead? This is what I said where I worry that we forget to plumb the traceconfig fully. Can we add a cross-check? In the BenchmarkMemoryLight_TracingEnabledCallback can we if (preset == MEMORY_LIGHT) CHEKCK_FALSE(MDM::GetInstance::IsDumpModeAllowed(DETAILED)) https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:916: if (!base::trace_event::MemoryDumpManager::GetInstance()->IsDumpModeAllowed( why do we need this extra check here? Can we let it fail only in one place? Is this only a perf optimization?
Patchset #5 (id:120001) has been deleted
Patchset #5 (id:140001) has been deleted
Thanks for your comments, ptal. https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/memor... File base/trace_event/memory_dump_manager.cc (right): https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/memor... base/trace_event/memory_dump_manager.cc:716: return session_state_->memory_dump_config().allowed_dump_modes.count( On 2016/06/09 18:46:39, Primiano Tucci wrote: > - Take the AutoLock lock(lock_); session_state is always accessed under the > lock. > Don't worry about RequestGlobalMemoryDump locking twice, It's ok, once you hit > this you are out of the fast-path (we established we are going to spend time > doing a dump, in which case a lock is not influent) > - You need a if(!session_state) return false, otherwise this will crash if you > call this outside of a tracing session Fixed the lock. Also renamed the session_state getter function which reads without lock. Did not add the check because none calls this when tracing is not enabled and wanted it to fail. The request calls checks for tracing enabled first before calling. https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:83: std::set<MemoryDumpLevelOfDetail> modes(kDefaultAllowedDumpModes, On 2016/06/09 18:46:39, Primiano Tucci wrote: > Hmm I'll make this a bit more self-maintaining and do: > > std::set<MemoryDumpLevelOfDetail> all_modes; > for (auto mode = MemoryDumpLevelOfDetails::FIRST; mode <= ..::LAST; mode = > static_cast<LevelOfDetail>(static_cast<uint32_t>(mode) + 1)) { > all_modes.add(i) > } > > I know that the double cast is now awesome, but better than having to remember > to edit two places. didn't know I could iterate here, thanks. https://codereview.chromium.org/2041583003/diff/100001/base/trace_event/trace... base/trace_event/trace_config.cc:86: DCHECK_EQ(static_cast<uint32_t>(MemoryDumpLevelOfDetail::LAST), On 2016/06/09 18:46:39, Primiano Tucci wrote: > at which point you get rid of the dcheck Done. https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.cc:417: base::trace_event::TraceConfig::MemoryDumpConfig()); On 2016/06/09 18:46:39, Primiano Tucci wrote: > ehm don't you have to pass memory_config here instead? > This is what I said where I worry that we forget to plumb the traceconfig fully. > > Can we add a cross-check? In the BenchmarkMemoryLight_TracingEnabledCallback can > we > if (preset == MEMORY_LIGHT) > CHEKCK_FALSE(MDM::GetInstance::IsDumpModeAllowed(DETAILED)) Done. Yeah I think this should be fixed when we can have trace config uploaded from finch. we will have better control. https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:916: if (!base::trace_event::MemoryDumpManager::GetInstance()->IsDumpModeAllowed( On 2016/06/09 18:46:39, Primiano Tucci wrote: > why do we need this extra check here? Can we let it fail only in one place? > Is this only a perf optimization? It is a public method of TracingControllerImpl and anyone outside codebase can get instance and trigger dump. I was also paranoid about some child process sending the dump request ipc message. Essentially this method could be called without MDM.
Patchset #5 (id:160001) has been deleted
Patchset #5 (id:180001) has been deleted
Patchset #5 (id:200001) has been deleted
LGTM % the change in /tracing_controller_impl.cc which IMHO should be removed https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:916: if (!base::trace_event::MemoryDumpManager::GetInstance()->IsDumpModeAllowed( On 2016/06/09 21:34:15, ssid wrote: > On 2016/06/09 18:46:39, Primiano Tucci wrote: > > why do we need this extra check here? Can we let it fail only in one place? > > Is this only a perf optimization? > > It is a public method of TracingControllerImpl and anyone outside codebase can > get instance and trigger dump. I was also paranoid about some child process > sending the dump request ipc message. Essentially this method could be called > without MDM. Yes what I am saying is that this checks seems superfluos. Even if you pass this you are going to hit the MDM that will NACK the dump few moments later. Also you should never just return without invoking the callback. Just remove these two lines. in the worst case if a child process send the IPC and we get here, they will receive a nack. (unless I'm not thinking to something here) IIRC even in the case where a child process doesn't receive the traceconfig by mistake, upon a call to RequestGlobalDump, the child process will not create the local dump, but will wait for the browser to ask it do dump. In other word, even for dumps initiated by the child process, the flow is: ReqGLobalDUmp->(child)->RequestGlobalDump->(browser)->CreateLocalDump->(all childs) https://codereview.chromium.org/2041583003/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2041583003/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:125: const scoped_refptr<MemoryDumpSessionState>& session_state_for_testing() oh yes thanks. I always wanted to remove this from the public API. for_testing makes perfect sense. https://codereview.chromium.org/2041583003/diff/220001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/2041583003/diff/220001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.cc:55: CHECK(!dump_manager->IsDumpModeAllowed( maybe add a comment saying: "Safety check to make sure the memory-infra restrictions are properly propagated via TraceConfig" or something like that.
I still haven't removed the tracing controller condition. I think it is necessary not superfluos. https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:916: if (!base::trace_event::MemoryDumpManager::GetInstance()->IsDumpModeAllowed( On 2016/06/10 12:31:57, Primiano Tucci wrote: > On 2016/06/09 21:34:15, ssid wrote: > > On 2016/06/09 18:46:39, Primiano Tucci wrote: > > > why do we need this extra check here? Can we let it fail only in one place? > > > Is this only a perf optimization? > > > > It is a public method of TracingControllerImpl and anyone outside codebase can > > get instance and trigger dump. I was also paranoid about some child process > > sending the dump request ipc message. Essentially this method could be called > > without MDM. > > Yes what I am saying is that this checks seems superfluos. Even if you pass this > you are going to hit the MDM that will NACK the dump few moments later. Also you > should never just return without invoking the callback. MDM can't nack the dump if someone calls TracingControllerImpl::RequestGlobalMemoryDump. It assumes MDM has accepted the dump at this point. If I take skia and use: TracingControllerImpl::GetInstance()->RequestGlobalMemoryDump(DETAILED) This would still go through without checking MDM. > Just remove these two lines. in the worst case if a child process send the IPC > and we get here, they will receive a nack. (unless I'm not thinking to something > here) > > IIRC even in the case where a child process doesn't receive the traceconfig by > mistake, upon a call to RequestGlobalDump, the child process will not create the > local dump, but will wait for the browser to ask it do dump. In other word, even > for dumps initiated by the child process, the flow is: > ReqGLobalDUmp->(child)->RequestGlobalDump->(browser)->CreateLocalDump->(all > childs) Yes, but this flow does not include browser MDM::RequestGlobalDump which actually checks for allowed modes. This flow bypasses the MDM::Request of the browser https://codereview.chromium.org/2041583003/diff/220001/base/trace_event/memor... File base/trace_event/memory_dump_manager.h (right): https://codereview.chromium.org/2041583003/diff/220001/base/trace_event/memor... base/trace_event/memory_dump_manager.h:125: const scoped_refptr<MemoryDumpSessionState>& session_state_for_testing() On 2016/06/10 12:31:57, Primiano Tucci wrote: > oh yes thanks. I always wanted to remove this from the public API. > for_testing makes perfect sense. Acknowledged. https://codereview.chromium.org/2041583003/diff/220001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/2041583003/diff/220001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.cc:55: CHECK(!dump_manager->IsDumpModeAllowed( On 2016/06/10 12:31:57, Primiano Tucci wrote: > maybe add a comment saying: > "Safety check to make sure the memory-infra restrictions are properly propagated > via TraceConfig" > or something like that. Done.
ssid@chromium.org changed reviewers: + dgozman@chromium.org, oysteine@chromium.org
+dgozman For fixing devtools unittest. Note that this change need not be documented in devtools protocol yet. This is because memory dump config in the devtools config is treated as black box string and once the config gets stable the devtools protocol will be updated. +oysteine For changes in background tracing file.
[+caseq FYI] devtools lgtm
https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:916: if (!base::trace_event::MemoryDumpManager::GetInstance()->IsDumpModeAllowed( On 2016/06/10 18:23:47, ssid wrote: > On 2016/06/10 12:31:57, Primiano Tucci wrote: > > On 2016/06/09 21:34:15, ssid wrote: > > > On 2016/06/09 18:46:39, Primiano Tucci wrote: > > > > why do we need this extra check here? Can we let it fail only in one > place? > > > > Is this only a perf optimization? > > > > > > It is a public method of TracingControllerImpl and anyone outside codebase > can > > > get instance and trigger dump. I was also paranoid about some child process > > > sending the dump request ipc message. Essentially this method could be > called > > > without MDM. > > > > Yes what I am saying is that this checks seems superfluos. Even if you pass > this > > you are going to hit the MDM that will NACK the dump few moments later. Also > you > > should never just return without invoking the callback. > > MDM can't nack the dump if someone calls > TracingControllerImpl::RequestGlobalMemoryDump. It assumes MDM has accepted the > dump at this point. > If I take skia and use: > TracingControllerImpl::GetInstance()->RequestGlobalMemoryDump(DETAILED) > This would still go through without checking MDM. > > > Just remove these two lines. in the worst case if a child process send the IPC > > and we get here, they will receive a nack. (unless I'm not thinking to > something > > here) > > > > IIRC even in the case where a child process doesn't receive the traceconfig by > > mistake, upon a call to RequestGlobalDump, the child process will not create > the > > local dump, but will wait for the browser to ask it do dump. In other word, > even > > for dumps initiated by the child process, the flow is: > > ReqGLobalDUmp->(child)->RequestGlobalDump->(browser)->CreateLocalDump->(all > > childs) > > Yes, but this flow does not include browser MDM::RequestGlobalDump which > actually checks for allowed modes. This flow bypasses the MDM::Request of the > browser Ok fine. But let's instead make this a CHECK in MDM::CreateProcessDump (on top of the more graceful NACK in MDM::RequestGlobalDump). Nobody should use TracingControllerImpl directly. But I agree with you that it is technically possible, in which case we should just crash the world.
Thanks, made suggested change. https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... File content/browser/tracing/tracing_controller_impl.cc (right): https://codereview.chromium.org/2041583003/diff/100001/content/browser/tracin... content/browser/tracing/tracing_controller_impl.cc:916: if (!base::trace_event::MemoryDumpManager::GetInstance()->IsDumpModeAllowed( On 2016/06/13 19:56:54, Primiano Tucci wrote: > On 2016/06/10 18:23:47, ssid wrote: > > On 2016/06/10 12:31:57, Primiano Tucci wrote: > > > On 2016/06/09 21:34:15, ssid wrote: > > > > On 2016/06/09 18:46:39, Primiano Tucci wrote: > > > > > why do we need this extra check here? Can we let it fail only in one > > place? > > > > > Is this only a perf optimization? > > > > > > > > It is a public method of TracingControllerImpl and anyone outside codebase > > can > > > > get instance and trigger dump. I was also paranoid about some child > process > > > > sending the dump request ipc message. Essentially this method could be > > called > > > > without MDM. > > > > > > Yes what I am saying is that this checks seems superfluos. Even if you pass > > this > > > you are going to hit the MDM that will NACK the dump few moments later. Also > > you > > > should never just return without invoking the callback. > > > > MDM can't nack the dump if someone calls > > TracingControllerImpl::RequestGlobalMemoryDump. It assumes MDM has accepted > the > > dump at this point. > > If I take skia and use: > > TracingControllerImpl::GetInstance()->RequestGlobalMemoryDump(DETAILED) > > This would still go through without checking MDM. > > > > > Just remove these two lines. in the worst case if a child process send the > IPC > > > and we get here, they will receive a nack. (unless I'm not thinking to > > something > > > here) > > > > > > IIRC even in the case where a child process doesn't receive the traceconfig > by > > > mistake, upon a call to RequestGlobalDump, the child process will not create > > the > > > local dump, but will wait for the browser to ask it do dump. In other word, > > even > > > for dumps initiated by the child process, the flow is: > > > ReqGLobalDUmp->(child)->RequestGlobalDump->(browser)->CreateLocalDump->(all > > > childs) > > > > Yes, but this flow does not include browser MDM::RequestGlobalDump which > > actually checks for allowed modes. This flow bypasses the MDM::Request of the > > browser > > Ok fine. But let's instead make this a CHECK in MDM::CreateProcessDump (on top > of the more graceful NACK in MDM::RequestGlobalDump). > Nobody should use TracingControllerImpl directly. But I agree with you that it > is technically possible, in which case we should just crash the world. Done.
On 2016/06/14 01:26:11, ssid wrote: > Thanks, made suggested change. re-LGTM thanks
oysteine@ ptal thanks.
On 2016/06/14 18:30:26, ssid wrote: > oysteine@ ptal thanks. friendly ping.
Description was changed from ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components from triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. If not mentioned then by default all the modes are allowed. BUG=617208 ========== to ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components from triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. If not mentioned then by default all the modes are allowed. BUG=617208 TBR=oysteine@chromium.org ==========
On 2016/06/16 23:54:21, ssid wrote: > On 2016/06/14 18:30:26, ssid wrote: > > oysteine@ ptal thanks. > > friendly ping. TBRing oysteine@ since primiano is happy with this change. It makes a safety check for background mode.
The CQ bit was checked by ssid@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041583003/260001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-gn/bui...) ios-simulator on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-gn on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-gn/...) mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
Apologies for not seeing this before, BlinkOn got in the way last week. https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/memor... File base/trace_event/memory_dump_request_args.h (right): https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/memor... base/trace_event/memory_dump_request_args.h:31: enum class MemoryDumpLevelOfDetail : uint32_t { Why do you need to explicitly specify the storage type? https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/memor... base/trace_event/memory_dump_request_args.h:47: FIRST = BACKGROUND, Nit: "FIRST" should probably be first in the list, to make it more obvious that it should get updated if someone prepends something. https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/trace... base/trace_event/trace_config.cc:83: mode = static_cast<MemoryDumpLevelOfDetail>(static_cast<uint32_t>(mode) + This seems a little awkward. Maybe just make 'mode' an int, for less back-and-forth casting? https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.cc:55: // Safety check to make sure the memory-infra restrictions are properly I'm a bit confused by this. Why does it matter whether or not the detailed LOD is allowed, at this point?
(As a general rule, if I don't respond to a codereview email ping, please IM ping me :) ).
On 2016/06/20 17:03:04, Oystein wrote: > (As a general rule, if I don't respond to a codereview email ping, please IM > ping me :) ). Sorry about this. I couldn't ping you because I never got to office early enough to ping you on MUN time zone. Email was the only way.. https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/memor... File base/trace_event/memory_dump_request_args.h (right): https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/memor... base/trace_event/memory_dump_request_args.h:31: enum class MemoryDumpLevelOfDetail : uint32_t { On 2016/06/20 17:02:18, Oystein wrote: > Why do you need to explicitly specify the storage type? Because I am casting this later, so thought it is safer to specify the type here as well. https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/memor... base/trace_event/memory_dump_request_args.h:47: FIRST = BACKGROUND, On 2016/06/20 17:02:18, Oystein wrote: > Nit: "FIRST" should probably be first in the list, to make it more obvious that > it should get updated if someone prepends something. Done. https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/trace... base/trace_event/trace_config.cc:83: mode = static_cast<MemoryDumpLevelOfDetail>(static_cast<uint32_t>(mode) + On 2016/06/20 17:02:18, Oystein wrote: > This seems a little awkward. Maybe just make 'mode' an int, for less > back-and-forth casting? This is awkward at only one place. MemoryDumpLevelOfDetail is used in MemoryDumpArgs and MemoryDumpRequestArgs and TraceConfig. So, if i change it to int only in trace config, then I would have to cast when the trace config is used. I would also have to add cast at the StringToMemoryDumpLevelOfDetail and MemoryDumpLevelOfDetailToString since these functions are used at other places. It will also be inconsistent since it is int here and enum elsewhere. If I change MemoryDumpLevelOfDetail to int, I would have to edit each dump provider to check differently with new definition. It will be a huge change. https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.cc:55: // Safety check to make sure the memory-infra restrictions are properly On 2016/06/20 17:02:18, Oystein wrote: > I'm a bit confused by this. Why does it matter whether or not the detailed LOD > is allowed, at this point? So, this is here to ensure that all this code in this CL is working. Suppose there was a mistake and the trace config did not get propagated to renderer process or the correct trace config wasn't given background controller and a heavy dump could be triggered then we might end up uploading sensitive data. So, this check would crash here instead of uploading the data.
https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/trace... File base/trace_event/trace_config.cc (right): https://codereview.chromium.org/2041583003/diff/280001/base/trace_event/trace... base/trace_event/trace_config.cc:83: mode = static_cast<MemoryDumpLevelOfDetail>(static_cast<uint32_t>(mode) + Sorry, I just understood what you said. I made mode as uint and it looks slightly better.
https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... File content/browser/tracing/background_tracing_manager_impl.cc (right): https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... content/browser/tracing/background_tracing_manager_impl.cc:55: // Safety check to make sure the memory-infra restrictions are properly On 2016/06/20 19:12:06, ssid wrote: > On 2016/06/20 17:02:18, Oystein wrote: > > I'm a bit confused by this. Why does it matter whether or not the detailed LOD > > is allowed, at this point? > > So, this is here to ensure that all this code in this CL is working. > Suppose there was a mistake and the trace config did not get propagated to > renderer process or the correct trace config wasn't given background controller > and a heavy dump could be triggered then we might end up uploading sensitive > data. > So, this check would crash here instead of uploading the data. Yup this is essentially my paranoy in action. It's quite easy to screw up the trace config plumbing, and this is a final cross-check to make chrome suicide if we do that (at least with memory-infra stuff). I prefer getting bugs assigned for crashes than having to write privacy screwup postmortems :)
On 2016/06/21 06:50:49, Primiano Tucci wrote: > https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... > File content/browser/tracing/background_tracing_manager_impl.cc (right): > > https://codereview.chromium.org/2041583003/diff/280001/content/browser/tracin... > content/browser/tracing/background_tracing_manager_impl.cc:55: // Safety check > to make sure the memory-infra restrictions are properly > On 2016/06/20 19:12:06, ssid wrote: > > On 2016/06/20 17:02:18, Oystein wrote: > > > I'm a bit confused by this. Why does it matter whether or not the detailed > LOD > > > is allowed, at this point? > > > > So, this is here to ensure that all this code in this CL is working. > > Suppose there was a mistake and the trace config did not get propagated to > > renderer process or the correct trace config wasn't given background > controller > > and a heavy dump could be triggered then we might end up uploading sensitive > > data. > > So, this check would crash here instead of uploading the data. > > Yup this is essentially my paranoy in action. It's quite easy to screw up the > trace config plumbing, and this is a final cross-check to make chrome suicide if > we do that (at least with memory-infra stuff). > I prefer getting bugs assigned for crashes than having to write privacy screwup > postmortems :) I'm all for paranoia in these cases, it's just that the check itself seems odd to me and not really a final cross-check so much. Does checking for detailed modes being allowed when you request a dump really give a strong assurance that a detail dump isn't being received later? An assert on a non-background dump being received by the callback would make more sense, no..? Even the TraceLog knows whether PII whitelisting is being done, so we should be able to check this anywhere. Anyway, not a big deal, probably just me being fuzzy on the memory-infra bits here. lgtm!
On 2016/06/21 12:40:22, Oystein wrote: > I'm all for paranoia in these cases, it's just that the check itself seems odd > to me and not really a final cross-check so much. Does checking for detailed > modes being allowed when you request a dump really give a strong assurance that > a detail dump isn't being received later? So we have that check in MemoryDumpManager but that assumes that we have passed the right TraceConfig to that. This one essentially makes sure that the MDM will never allow any detailed dump until tracing is ended. > Anyway, not a big deal, probably just me being fuzzy on the memory-infra bits > here. lgtm! Not too familiar with the code here. If there is a better place feel free to suggest the move :)
> I'm all for paranoia in these cases, it's just that the check itself seems odd > to me and not really a final cross-check so much. Does checking for detailed > modes being allowed when you request a dump really give a strong assurance that > a detail dump isn't being received later? An assert on a non-background dump > being received by the callback would make more sense, no..? Even the TraceLog > knows whether PII whitelisting is being done, so we should be able to check this > anywhere. Yes I agree that this is not a final cross check. But i do not see any better way to do this. I wish something in base knows that we are doing a background tracing session. I don't think TraceLog knows about background session, nor does memory dump manager. We do not have a concept of "background tracing" in base. Memory dump manager only knows about dump modes and we have a background dump mode. It follows what trace config tells. It's a bit awkward to place recording mode = "background" or something in trace config. This is the major cause for having extra code in background controller, which should be in memory infra. > Anyway, not a big deal, probably just me being fuzzy on the memory-infra bits > here. lgtm! I think most of these should be removed if finch supported trace config. But, the part left would be to figure out when to trigger dumps, which seems tricky.
On 2016/06/21 19:04:14, ssid wrote: > > I'm all for paranoia in these cases, it's just that the check itself seems odd > > to me and not really a final cross-check so much. Does checking for detailed > > modes being allowed when you request a dump really give a strong assurance > that > > a detail dump isn't being received later? An assert on a non-background dump > > being received by the callback would make more sense, no..? Even the TraceLog > > knows whether PII whitelisting is being done, so we should be able to check > this > > anywhere. > > Yes I agree that this is not a final cross check. But i do not see any better > way to do this. I wish something in base knows that we are doing a background > tracing session. I don't think TraceLog knows about background session, nor does > memory dump manager. We do not have a concept of "background tracing" in base. > Memory dump manager only knows about dump modes and we have a background dump > mode. It follows what trace config tells. It's a bit awkward to place recording > mode = "background" or something in trace config. This is the major cause for > having extra code in background controller, which should be in memory infra. > Could the Memory dump manager check the TraceLog for the presence of a trace event filtering predicate? i.e. whether PII whitelisting is in place. > > Anyway, not a big deal, probably just me being fuzzy on the memory-infra bits > > here. lgtm! > > I think most of these should be removed if finch supported trace config. But, > the part left would be to figure out when to trigger dumps, which seems tricky.
> Could the Memory dump manager check the TraceLog for the presence of a trace > event filtering predicate? i.e. whether PII whitelisting is in place. Sounds like the filtering predicate should filter only the background dumps. I will look into this issue and clean up this code if necessary.
I was looking at the argument filtering code. It is enabled when "enable_argument_filter" flag is passed in trace config, Background tracing config does not seem to pass this flag anywhere. I am not sure how the TraceLog knows about the "background" session. Could you please elaborate little more? I am not sure if you are talking about the event filtering code in https://codereview.chromium.org/1923533004/ which has not landed yet. Also it looks like if this enable_argument_filter flag is passed the memory dumps would not get filtered in, since the args whitelist does not include "memory-infra" and "dumps". On 2016/06/21 21:45:55, ssid wrote: > > Could the Memory dump manager check the TraceLog for the presence of a trace > > event filtering predicate? i.e. whether PII whitelisting is in place. > > Sounds like the filtering predicate should filter only the background dumps. I > will look into this issue and clean up this code if necessary.
On 2016/06/22 00:07:13, ssid wrote: > I was looking at the argument filtering code. It is enabled when > "enable_argument_filter" flag is passed in trace config, Background tracing > config does not seem to pass this flag anywhere. > I am not sure how the TraceLog knows about the "background" session. Could you > please elaborate little more? > I am not sure if you are talking about the event filtering code in > https://codereview.chromium.org/1923533004/ which has not landed yet. > > Also it looks like if this enable_argument_filter flag is passed the memory > dumps would not get filtered in, since the args whitelist does not include > "memory-infra" and "dumps". > background_tracing_manager_impl.cc::407: if (requires_anonymized_data_) trace_config.EnableArgumentFilter(); Sorry if my point wasn't very clear before: I mean that you could assert on the presence of the predicate. If it's set, then that means in practice we're in a field-trial initiated background tracing session, and should not be receiving any detailed dumps. > On 2016/06/21 21:45:55, ssid wrote: > > > Could the Memory dump manager check the TraceLog for the presence of a trace > > > event filtering predicate? i.e. whether PII whitelisting is in place. > > > > Sounds like the filtering predicate should filter only the background dumps. I > > will look into this issue and clean up this code if necessary.
> > background_tracing_manager_impl.cc::407: > if (requires_anonymized_data_) > trace_config.EnableArgumentFilter(); > > Sorry if my point wasn't very clear before: I mean that you could assert on the > presence of the predicate. If it's set, then that means in practice we're in a > field-trial initiated background tracing session, and should not be receiving > any detailed dumps. > Thanks, I have created another CL to fix this issue. https://codereview.chromium.org/2092463002
The CQ bit was checked by ssid@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dgozman@chromium.org, primiano@chromium.org Link to the patchset: https://codereview.chromium.org/2041583003/#ps320001 (title: "Change mode to int.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2041583003/320001
Message was sent while issue was closed.
Description was changed from ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components from triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. If not mentioned then by default all the modes are allowed. BUG=617208 TBR=oysteine@chromium.org ========== to ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components from triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. If not mentioned then by default all the modes are allowed. BUG=617208 TBR=oysteine@chromium.org ==========
Message was sent while issue was closed.
Committed patchset #10 (id:320001)
Message was sent while issue was closed.
Description was changed from ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components from triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. If not mentioned then by default all the modes are allowed. BUG=617208 TBR=oysteine@chromium.org ========== to ========== [tracing] Introduce "allowed_dump_modes" for memory dump config The "allowed_dump_modes" contains a list of dump modes that are allowed for memory dumps in the current session. This is to prevent any other components from triggering explicit dumps with unexpected details. Trace config now controls the modes allowed. If not mentioned then by default all the modes are allowed. BUG=617208 TBR=oysteine@chromium.org Committed: https://crrev.com/40638de0bd1dbd66662c09b95bdd1df09ab9258c Cr-Commit-Position: refs/heads/master@{#401382} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/40638de0bd1dbd66662c09b95bdd1df09ab9258c Cr-Commit-Position: refs/heads/master@{#401382} |
