Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2680123003:
Multi-Process Tracking Support (Closed)
DescriptionMulti-Process Tracking Support
Fully support having multiple processes write information to the same breadcrumbs file.
This involves some process-specific operations plus the ability for a controlling process to clean-up after its dead children.
BUG=620813
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng
Review-Url: https://codereview.chromium.org/2680123003
Cr-Commit-Position: refs/heads/master@{#457502}
Committed: https://chromium.googlesource.com/chromium/src/+/815054b5918e1d631c4208b06b83d1e833ef6240
Patch Set 1 #Patch Set 2 : added process_data, similar to global_data, and background task-runner support #Patch Set 3 : rebased (and associated fixes) #Patch Set 4 : wire actual process launch/exit to global tracker #
Total comments: 2
Patch Set 5 : rebased #
Total comments: 7
Patch Set 6 : refactor process-info tracking; add process-info to module tracking #
Total comments: 14
Patch Set 7 : added defined process-phase recording and process-exit callback support #
Total comments: 10
Patch Set 8 : record process exit inside content_main #Patch Set 9 : addressed review comments by manzagop #Patch Set 10 : record command-line when a process is started #
Total comments: 15
Patch Set 11 : rebased #Patch Set 12 : addressed review comments by manzagop #
Total comments: 1
Patch Set 13 : addressed review comments by manzagop #
Total comments: 12
Patch Set 14 : comment improvements #Patch Set 15 : rebased #Patch Set 16 : move tracking from target_process to sandbox_win #
Dependent Patchsets: Messages
Total messages: 126 (81 generated)
The CQ bit was checked by bcwhite@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...
I haven't yet tied this into Chrome to have the process tracking be called but the tracking code is ready for a first look. CreateSnapshot is unchanged, just moved to its proper location in the .cc file.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by bcwhite@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by bcwhite@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.
Description was changed from ========== Track process begin and exit. BUG=620813 ========== to ========== Track process begin and exit. BUG=620813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by bcwhite@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: Try jobs failed on following builders: chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Tracking is now functional. With this, it could be enabled in other processes using the same file. I still need to study how the data comes _out_ ...
bcwhite@chromium.org changed reviewers: + manzagop@chromium.org
The CQ bit was checked by bcwhite@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: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
Cool stuff! To make sure I followed, let me summarize what I think this is doing. - the CL enables tracking all chrome processes using a single allocator - all processes are free to read everything, but they can only write their own thread, activity and module data. - there are now both global data and process data key/value mappings. Global activity data is meant to be used (written?) by all processes? https://codereview.chromium.org/2680123003/diff/60001/sandbox/win/src/target_... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/2680123003/diff/60001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:164: base::debug::GlobalActivityTracker::RecordProcessLaunchIfEnabled( Will these ever be recorded as exiting? I guess we always make a Process? Eg https://cs.chromium.org/chromium/src/content/common/sandbox_win.cc?type=cs&q=... https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:1270: void GlobalActivityTracker::RecordProcessLaunch(ProcessId process_id) {} Nothing to do here? Do we need the function? https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:1294: void GlobalActivityTracker::RecordProcessExitImpl(ProcessId process_id, IIUC this is more like "CleanUpPossibleLeftovers"? https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.h:439: int64_t create_stamp; // The time of creation. Is this the creation time of the process or of the ActivityData?
Side thought: what happens when a process crashes. Will the cleanup be invoked? Will it be invoked late enough for scraping to happen?
> To make sure I followed, let me summarize what I think this is doing. > > - the CL enables tracking all chrome processes using a single allocator It enabled support for such but currently only the Browser process is enabling the tracker. It'll have to be turned on for other processes as desired. It isn't strictly necessary to use a single allocator but it's likely easiest and, so long as this isn't going beyond Canary/Dev, it might be okay even for Renderer processes. If this ever pushes further, untrusted processes will have to write to parallel files, but I think that'll be fairly easy when the time comes. > - all processes are free to read everything, but they can only write their own > thread, activity and module data. Right. Though come to think of it, I'm sure the module data isn't keyed to a specific process so I'll need to update that, too. > - there are now both global data and process data key/value mappings. Global > activity data is meant to be used (written?) by all processes? Yes. Chrome version, for example, would be good global data. Things that change between processes would be better written to the process data so it can be differentiated somehow during upload.
On 2017/02/17 20:21:37, manzagop wrote: > Side thought: what happens when a process crashes. Will the cleanup be invoked? > Will it be invoked late enough for scraping to happen? If any process but the browser crashes, it'll get cleaned up by the browser process and thus the cleanup will run. If the browser process crashes, then there will be no cleanup you basically get the state of everything as it was when it crashed.
https://codereview.chromium.org/2680123003/diff/60001/sandbox/win/src/target_... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/2680123003/diff/60001/sandbox/win/src/target_... sandbox/win/src/target_process.cc:164: base::debug::GlobalActivityTracker::RecordProcessLaunchIfEnabled( On 2017/02/17 19:39:54, manzagop wrote: > Will these ever be recorded as exiting? > > I guess we always make a Process? Eg > https://cs.chromium.org/chromium/src/content/common/sandbox_win.cc?type=cs&q=... Yes, but it's recognized in a different file. In fact, there's no common place for all launch/reap activities. :-( https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:1270: void GlobalActivityTracker::RecordProcessLaunch(ProcessId process_id) {} On 2017/02/17 19:39:54, manzagop wrote: > Nothing to do here? Do we need the function? I anticipate needing this information, especially if we go to multi-file. I may add some tracking in here for now just so that we can check that every launch is detected. https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.cc:1294: void GlobalActivityTracker::RecordProcessExitImpl(ProcessId process_id, On 2017/02/17 19:39:54, manzagop wrote: > IIUC this is more like "CleanUpPossibleLeftovers"? Will do. https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.h:439: int64_t create_stamp; // The time of creation. On 2017/02/17 19:39:54, manzagop wrote: > Is this the creation time of the process or of the ActivityData? Of the ActivityData.
> > - all processes are free to read everything, but they can only write their own > > thread, activity and module data. > > Right. Though come to think of it, I'm sure the module data isn't keyed to a > specific process so I'll need to update that, too. The load address will be different. > > - there are now both global data and process data key/value mappings. Global > > activity data is meant to be used (written?) by all processes? > > Yes. Chrome version, for example, would be good global data. Things that > change between processes would be better written to the process data so it can > be differentiated somehow during upload. We use a lock to access that data. Does that mean we shouldn't write from multiple processes?
On 2017/02/17 20:26:51, bcwhite wrote: > On 2017/02/17 20:21:37, manzagop wrote: > > Side thought: what happens when a process crashes. Will the cleanup be > invoked? > > Will it be invoked late enough for scraping to happen? > > If any process but the browser crashes, it'll get cleaned up by the browser > process and thus the cleanup will run. Can you say more about what triggers the cleanup? Is it that Process's Terminate or WaitForExit will be called?
WRT the current CL, did you mean to add anything to it or should I complete the review? https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/80001/base/debug/activity_tra... base/debug/activity_tracker.h:439: int64_t create_stamp; // The time of creation. On 2017/02/17 20:31:53, bcwhite wrote: > On 2017/02/17 19:39:54, manzagop wrote: > > Is this the creation time of the process or of the ActivityData? > > Of the ActivityData. I remember Siggi saying process id is not unique due to reuse. It's possible for a new process to be launched with the same identifier after an old one exited. It sounds like we need to harden the process identifier?
> > Yes. Chrome version, for example, would be good global data. Things that > > change between processes would be better written to the process data so it can > > be differentiated somehow during upload. > > We use a lock to access that data. Does that mean we shouldn't write from > multiple processes? Each process has independent space tied only to that process (but shared between all threads of the process, hence the lock).
> > > Side thought: what happens when a process crashes. Will the cleanup be > > invoked? > > > Will it be invoked late enough for scraping to happen? > > > > If any process but the browser crashes, it'll get cleaned up by the browser > > process and thus the cleanup will run. > > Can you say more about what triggers the cleanup? Is it that Process's > Terminate or WaitForExit will be called? Assuming that process A launches process B then when B exits for whatever reason (normal or crash), A will notice and do the cleanup.
> WRT the current CL, did you mean to add anything to it or should I complete the > review? I'm on the train so won't be editing code until next week.
On 2017/02/17 21:48:08, bcwhite wrote: > > > Yes. Chrome version, for example, would be good global data. Things that > > > change between processes would be better written to the process data so it > can > > > be differentiated somehow during upload. > > > > We use a lock to access that data. Does that mean we shouldn't write from > > multiple processes? > > Each process has independent space tied only to that process (but shared between > all threads of the process, hence the lock). That applies to the global data key/value store?
I'm thinking that "process lifetime" should be recorded by the GlobalTracker using an enum that is written to the process_data. I know you've already got some like MAIN_LOOP, etc. Upon proper shutdown, this will get set to EXITED_CLEANLY. If, when cleaning up after a process, it is detected that one didn't exit cleanly, a user-defined callback will be run from the worker thread. That'll allow you to do whatever you want to do with the process data before it gets cleaned up. Sound useful?
On 2017/02/17 22:01:36, manzagop wrote: > On 2017/02/17 21:48:08, bcwhite wrote: > > > > Yes. Chrome version, for example, would be good global data. Things that > > > > change between processes would be better written to the process data so it > > can > > > > be differentiated somehow during upload. > > > > > > We use a lock to access that data. Does that mean we shouldn't write from > > > multiple processes? > > > > Each process has independent space tied only to that process (but shared > between > > all threads of the process, hence the lock). > > That applies to the global data key/value store? Global is global. It's shared among all processes. But its also safe for all process to write.
On 2017/02/17 21:49:36, bcwhite wrote: > > > > Side thought: what happens when a process crashes. Will the cleanup be > > > invoked? > > > > Will it be invoked late enough for scraping to happen? > > > > > > If any process but the browser crashes, it'll get cleaned up by the browser > > > process and thus the cleanup will run. > > > > Can you say more about what triggers the cleanup? Is it that Process's > > Terminate or WaitForExit will be called? > > Assuming that process A launches process B then when B exits for whatever reason > (normal or crash), A will notice and do the cleanup. I mean concretely, what code path will lead to RecordProcessExitImpl? Is it through Process::*?
On 2017/02/17 22:04:07, manzagop wrote: > On 2017/02/17 21:49:36, bcwhite wrote: > > > > > Side thought: what happens when a process crashes. Will the cleanup be > > > > invoked? > > > > > Will it be invoked late enough for scraping to happen? > > > > > > > > If any process but the browser crashes, it'll get cleaned up by the > browser > > > > process and thus the cleanup will run. > > > > > > Can you say more about what triggers the cleanup? Is it that Process's > > > Terminate or WaitForExit will be called? > > > > Assuming that process A launches process B then when B exits for whatever > reason > > (normal or crash), A will notice and do the cleanup. > > I mean concretely, what code path will lead to RecordProcessExitImpl? Is it > through > Process::*? As far as I know, yes, but we could find others in the future.
The CQ bit was checked by bcwhite@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: Try jobs failed on following builders: cast_shell_linux on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT, no build URL)
The CQ bit was checked by bcwhite@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.
Okay, I think it's ready for proper review.
The CQ bit was checked by bcwhite@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...
Nice! A few comments. I'm not sure of the mechanic for handling child process crashes. For cases caught by our crash handler, we want it to be able to collect the data before it's cleaned. For the other cases, we want to know it wasn't caught. Sidethought: the recording seems to be getting to a point where a design doc to explain some intents might be worth it, wdyt? Side request: could you expand the CL's description a bit? https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:118: create_stamp = stamp; Do you need/want to set data_id so that ProcessInfo::OwningProcessId doesn't hit the if on l.127? https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:132: return id == info->data_id.load(std::memory_order_seq_cst); Can you say more about this operation? IIUC there are 2 operations on the variable: 1) a release store from 0 to nonzero, in ProcessInfo::Release_Initialize 2) a memset from non-zero to zero, when the PMA reference block is freed Can there be an issue wrt operation 2? Can you start zero'ing the pid/timestamp before zero'ing the data_id? https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:1279: DCHECK(!base::ContainsKey(known_processes_, process_id)); This is possible due to pid recycling. The map should contain ProcessInfo? https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:1003: void CleanupAfterProcess(ProcessId process_id, int64_t exit_stamp); I'm still not sure how we call CleanupAfterProcess if the child crashes. Eg, for renderers, do we need to add a call in RenderProcessHost::ProcessDied or the like? https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:1041: // The collection of processes being tracked. This is not persistent information. Should it be? The reason would be to avoid the time window after we've posted CleanupAfterProcess but before the task was processed. If we crash during that time window, we'll think the other process is still running. https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:471: // is written last, atomically, to release all the other values. Re: the "global" activity data, I don't think this supports multiple writer processes. They'll each have their own memory_ and may step on each other's toes? If that's indeed the case, we could leave that for a subsequent CL. https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:908: // Get the general thread information. Loading of "process_id" is guaranteed Update comment about motivation for process_id being last? https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:927: // If the data ID has changed then the tracker has exited and the memory Same question about the freeing operation: do we need to atomically clear the data id before we clear the contents? https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:1322: CleanupAfterProcess(process_id, now_stamp, exit_code); Same as other comment, pid recycling may be an issue.
Description was changed from ========== Track process begin and exit. BUG=620813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Multi-Process Tracking Support Fully support having multiple processes write information to the same breadcrumbs file. This involves some process-specific operations plus the ability for a controlling process to clean-up after its dead children. BUG=620813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ==========
The CQ bit was checked by bcwhite@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...
> I'm not sure of the mechanic for handling child process crashes. For cases > caught by our crash handler, we want it to be able to collect the data before > it's cleaned. For the other cases, we want to know it wasn't caught. You can. The callback is there just for this reason. All the process's records are still available so you can create a global analyzer on the file and iterate through the processes until you find the matching one. I can make that more efficient if need be. I don't think it's practical to not free the data, however, as otherwise a long running browser could fill the file. > Sidethought: the recording seems to be getting to a point where a design doc to > explain some intents might be worth it, wdyt? Probably. :-) > Side request: could you expand the CL's description a bit? Done. https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:118: create_stamp = stamp; On 2017/02/22 20:44:15, manzagop wrote: > Do you need/want to set data_id so that ProcessInfo::OwningProcessId doesn't hit > the if on l.127? All three already have valid, non-zero values. This just points two of them at a different process. The data_id can remain as it is. https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:132: return id == info->data_id.load(std::memory_order_seq_cst); On 2017/02/22 20:44:15, manzagop wrote: > Can you say more about this operation? > > IIUC there are 2 operations on the variable: > 1) a release store from 0 to nonzero, in ProcessInfo::Release_Initialize > 2) a memset from non-zero to zero, when the PMA reference block is freed > > Can there be an issue wrt operation 2? Can you start zero'ing the pid/timestamp > before zero'ing the data_id? It could since memset doesn't define the order in which it clears memory. And even if it did, the writes are non-atomic so who knows what the compiler and CPU might do with it. I think I'll have to fix this inside the PMA to ensure the first word always gets cleared first. https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:1279: DCHECK(!base::ContainsKey(known_processes_, process_id)); On 2017/02/22 20:44:15, manzagop wrote: > This is possible due to pid recycling. The map should contain ProcessInfo? It's possible only if there was no corresponding RecordProcessExit for the previous process of the same ID. Every Launch should be matched by an Exit call. This tells me if I'm missing something. But I should make the insert safe. https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:1003: void CleanupAfterProcess(ProcessId process_id, int64_t exit_stamp); On 2017/02/22 20:44:15, manzagop wrote: > I'm still not sure how we call CleanupAfterProcess if the child crashes. Eg, for > renderers, do we need to add a call in RenderProcessHost::ProcessDied or the > like? Shouldn't. The base Process code deals with the cleanup of child processes and it updates the tracker. https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.h:1041: // The collection of processes being tracked. On 2017/02/22 20:44:15, manzagop wrote: > This is not persistent information. Should it be? > > The reason would be to avoid the time window after we've posted > CleanupAfterProcess but before the task was processed. If we crash during that > time window, we'll think the other process is still running. That race would continue to exist because the process could exit but the browser might crash before updating the list. In any case it is possible to find a process that has "exited" among the crumbs but that'll be recognizable during analysis because that process will have a "phase" value indicating where it was at the time. Perhaps more problematic is a browser crash where the subprocesses continue to operate. The crumbs would hold state information from after the browser death, perhaps including a clean exit (though no clean-up). https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:471: // is written last, atomically, to release all the other values. On 2017/02/22 20:44:16, manzagop wrote: > Re: the "global" activity data, I don't think this supports multiple writer > processes. They'll each have their own memory_ and may step on each other's > toes? > > If that's indeed the case, we could leave that for a subsequent CL. Yeah, that's a problem. And, unfortunately, it's a lock-free problem since cross-process locks aren't universal. <shudder> I'd love to do just a simple cross-process spin-lock. Easy enough to do and these aren't written frequently enough to need something super efficient... but fatally flawed if the process holding the lock happens to crash. Gotta think about this one. Is there a need for "global data"? Is "process data" sufficient? https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:908: // Get the general thread information. Loading of "process_id" is guaranteed On 2017/02/22 20:44:16, manzagop wrote: > Update comment about motivation for process_id being last? Done. https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:927: // If the data ID has changed then the tracker has exited and the memory On 2017/02/22 20:44:16, manzagop wrote: > Same question about the freeing operation: do we need to atomically clear the > data id before we clear the contents? Acknowledged. https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:1322: CleanupAfterProcess(process_id, now_stamp, exit_code); On 2017/02/22 20:44:15, manzagop wrote: > Same as other comment, pid recycling may be an issue. In this case, any new process will have a "create_stamp" that is after the passed "now_stamp". There is code in Cleanup to ignore any process created after the known one was reaped. Comment added.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@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.
A few nits. https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:118: create_stamp = stamp; On 2017/02/22 22:13:02, bcwhite wrote: > On 2017/02/22 20:44:15, manzagop wrote: > > Do you need/want to set data_id so that ProcessInfo::OwningProcessId doesn't > hit > > the if on l.127? > > All three already have valid, non-zero values. This just points two of them at > a different process. The data_id can remain as it is. Is that because calling Release_Initialize is a precondition? If so, wdyt of documenting the precondition with a DCHECK on data_id? https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:132: return id == info->data_id.load(std::memory_order_seq_cst); On 2017/02/22 22:13:02, bcwhite wrote: > On 2017/02/22 20:44:15, manzagop wrote: > > Can you say more about this operation? > > > > IIUC there are 2 operations on the variable: > > 1) a release store from 0 to nonzero, in ProcessInfo::Release_Initialize > > 2) a memset from non-zero to zero, when the PMA reference block is freed > > > > Can there be an issue wrt operation 2? Can you start zero'ing the > pid/timestamp > > before zero'ing the data_id? > > It could since memset doesn't define the order in which it clears memory. And > even if it did, the writes are non-atomic so who knows what the compiler and CPU > might do with it. > > I think I'll have to fix this inside the PMA to ensure the first word always > gets cleared first. I'm totally going to forget about this! :) Could you create a bug for it? https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:471: // is written last, atomically, to release all the other values. On 2017/02/22 22:13:04, bcwhite wrote: > On 2017/02/22 20:44:16, manzagop wrote: > > Re: the "global" activity data, I don't think this supports multiple writer > > processes. They'll each have their own memory_ and may step on each other's > > toes? > > > > If that's indeed the case, we could leave that for a subsequent CL. > > Yeah, that's a problem. And, unfortunately, it's a lock-free problem since > cross-process locks aren't universal. <shudder> > > I'd love to do just a simple cross-process spin-lock. Easy enough to do and > these aren't written frequently enough to need something super efficient... but > fatally flawed if the process holding the lock happens to crash. > > Gotta think about this one. > > Is there a need for "global data"? Is "process data" sufficient? > I'd say let's ignore for now. It conceptually sounds useful, but I can't think of an immediate use for it.. https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.cc:1288: << " with no corresponding exit."; Add UMA metric so we get visibility? https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.cc:1295: known_processes_.insert(std::make_pair(process_id, std::move(cmd))); 'cmd' is const so I think std::move has no effect. https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.h:938: ActivityUserData& global_data() { return global_data_; } So we remove this to avoid cross-process issues? https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker_unittest.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker_unittest.cc:115: ASSERT_EQ(sizeof(buffer) - 24, data.available_); Is 24 because OwningProcess::kExpectedInstanceSize? If so, use that for improved readability? https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker_unittest.cc:115: ASSERT_EQ(sizeof(buffer) - 24, data.available_); Use a expected_available variable to make it more readable / less error prone / easier to reorder? https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... File base/process/launch_win.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... base/process/launch_win.cc:100: base::debug::GlobalActivityTracker* tracker = Use your *IfEnabled counterpart? https://codereview.chromium.org/2680123003/diff/180001/sandbox/win/src/target... File sandbox/win/src/target_process.cc (right): https://codereview.chromium.org/2680123003/diff/180001/sandbox/win/src/target... sandbox/win/src/target_process.cc:164: base::debug::GlobalActivityTracker* tracker = Use your *IfEnabled counterpart?
The CQ bit was checked by bcwhite@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.
The CQ bit was checked by bcwhite@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...
https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:118: create_stamp = stamp; On 2017/02/24 15:56:35, manzagop wrote: > On 2017/02/22 22:13:02, bcwhite wrote: > > On 2017/02/22 20:44:15, manzagop wrote: > > > Do you need/want to set data_id so that ProcessInfo::OwningProcessId doesn't > > hit > > > the if on l.127? > > > > All three already have valid, non-zero values. This just points two of them > at > > a different process. The data_id can remain as it is. > > Is that because calling Release_Initialize is a precondition? If so, wdyt of > documenting the precondition with a DCHECK on data_id? Done. https://codereview.chromium.org/2680123003/diff/100001/base/debug/activity_tr... base/debug/activity_tracker.cc:132: return id == info->data_id.load(std::memory_order_seq_cst); On 2017/02/24 15:56:35, manzagop wrote: > On 2017/02/22 22:13:02, bcwhite wrote: > > On 2017/02/22 20:44:15, manzagop wrote: > > > Can you say more about this operation? > > > > > > IIUC there are 2 operations on the variable: > > > 1) a release store from 0 to nonzero, in ProcessInfo::Release_Initialize > > > 2) a memset from non-zero to zero, when the PMA reference block is freed > > > > > > Can there be an issue wrt operation 2? Can you start zero'ing the > > pid/timestamp > > > before zero'ing the data_id? > > > > It could since memset doesn't define the order in which it clears memory. And > > even if it did, the writes are non-atomic so who knows what the compiler and > CPU > > might do with it. > > > > I think I'll have to fix this inside the PMA to ensure the first word always > > gets cleared first. > > I'm totally going to forget about this! :) Could you create a bug for it? It's already out for review: https://codereview.chromium.org/2709113003 https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/120001/base/debug/activity_tr... base/debug/activity_tracker.cc:471: // is written last, atomically, to release all the other values. On 2017/02/24 15:56:35, manzagop wrote: > On 2017/02/22 22:13:04, bcwhite wrote: > > On 2017/02/22 20:44:16, manzagop wrote: > > > Re: the "global" activity data, I don't think this supports multiple writer > > > processes. They'll each have their own memory_ and may step on each other's > > > toes? > > > > > > If that's indeed the case, we could leave that for a subsequent CL. > > > > Yeah, that's a problem. And, unfortunately, it's a lock-free problem since > > cross-process locks aren't universal. <shudder> > > > > I'd love to do just a simple cross-process spin-lock. Easy enough to do and > > these aren't written frequently enough to need something super efficient... > but > > fatally flawed if the process holding the lock happens to crash. > > > > Gotta think about this one. > > > > Is there a need for "global data"? Is "process data" sufficient? > > > > I'd say let's ignore for now. It conceptually sounds useful, but I can't think > of an immediate use for it.. Acknowledged. https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.cc:1288: << " with no corresponding exit."; On 2017/02/24 15:56:35, manzagop wrote: > Add UMA metric so we get visibility? Okay, but I'll do it as a separate CL so as to not bring too many people into this rather large change. https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.cc:1295: known_processes_.insert(std::make_pair(process_id, std::move(cmd))); On 2017/02/24 15:56:35, manzagop wrote: > 'cmd' is const so I think std::move has no effect. Done. https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.h:938: ActivityUserData& global_data() { return global_data_; } On 2017/02/24 15:56:35, manzagop wrote: > So we remove this to avoid cross-process issues? I've done so in another CL that will follow this one. https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker_unittest.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker_unittest.cc:115: ASSERT_EQ(sizeof(buffer) - 24, data.available_); On 2017/02/24 15:56:35, manzagop wrote: > Is 24 because OwningProcess::kExpectedInstanceSize? If so, use that for improved > readability? Done. https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... File base/process/launch_win.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... base/process/launch_win.cc:100: base::debug::GlobalActivityTracker* tracker = On 2017/02/24 15:56:35, manzagop wrote: > Use your *IfEnabled counterpart? Some of the parameters can be expensive to generate so it's better if that generation is within the if statement.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'll do a final pass tomorrow. https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/debug/activity_tr... base/debug/activity_tracker.cc:1288: << " with no corresponding exit."; On 2017/03/06 16:33:51, bcwhite wrote: > On 2017/02/24 15:56:35, manzagop wrote: > > Add UMA metric so we get visibility? > > Okay, but I'll do it as a separate CL so as to not bring too many people into > this rather large change. nit: put a TODO https://codereview.chromium.org/2680123003/diff/220001/base/debug/activity_tr... File base/debug/activity_tracker_unittest.cc (right): https://codereview.chromium.org/2680123003/diff/220001/base/debug/activity_tr... base/debug/activity_tracker_unittest.cc:119: ASSERT_EQ(space - 24, data.available_); Ah I meant you can: space -= 24; ASSERT_EQ(space, data.available_); That way you avoid the long chain at l.143 which is harder to maintain.
The CQ bit was checked by bcwhite@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.
Only nits! lgtm https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... File base/process/launch_win.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... base/process/launch_win.cc:100: base::debug::GlobalActivityTracker* tracker = On 2017/03/06 16:33:52, bcwhite wrote: > On 2017/02/24 15:56:35, manzagop wrote: > > Use your *IfEnabled counterpart? > > Some of the parameters can be expensive to generate so it's better if that > generation is within the if statement. Probably worth a comment. https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.cc:88: // This can fail if a another thread has just taken it. It isassumed that typo: isassumed https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.cc:1393: // searching nit: formatting https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:331: // global data. All updates must be done from the same thread. We should mention that ActivityUserData supports a single writer process. https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:674: const void* GetBaseAddress(); nit: const method? https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:746: // records its important to ensure that what is returned was created before typo: its -> it's https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:1118: // A lock that is used to procect access to the following fields. typo: procect
The CQ bit was checked by bcwhite@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: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bcwhite@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: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... File base/process/launch_win.cc (right): https://codereview.chromium.org/2680123003/diff/180001/base/process/launch_wi... base/process/launch_win.cc:100: base::debug::GlobalActivityTracker* tracker = On 2017/03/07 22:08:57, manzagop wrote: > On 2017/03/06 16:33:52, bcwhite wrote: > > On 2017/02/24 15:56:35, manzagop wrote: > > > Use your *IfEnabled counterpart? > > > > Some of the parameters can be expensive to generate so it's better if that > > generation is within the if statement. > > Probably worth a comment. Done. (in the activity_tracker.h file) https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... File base/debug/activity_tracker.cc (right): https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.cc:88: // This can fail if a another thread has just taken it. It isassumed that On 2017/03/07 22:08:57, manzagop wrote: > typo: isassumed Done. https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.cc:1393: // searching On 2017/03/07 22:08:57, manzagop wrote: > nit: formatting Done. https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... File base/debug/activity_tracker.h (right): https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:331: // global data. All updates must be done from the same thread. On 2017/03/07 22:08:57, manzagop wrote: > We should mention that ActivityUserData supports a single writer process. Done. https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:674: const void* GetBaseAddress(); On 2017/03/07 22:08:57, manzagop wrote: > nit: const method? Done. https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:746: // records its important to ensure that what is returned was created before On 2017/03/07 22:08:57, manzagop wrote: > typo: its -> it's Done. https://codereview.chromium.org/2680123003/diff/240001/base/debug/activity_tr... base/debug/activity_tracker.h:1118: // A lock that is used to procect access to the following fields. On 2017/03/07 22:08:57, manzagop wrote: > typo: procect Done.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org Link to the patchset: https://codereview.chromium.org/2680123003/#ps280001 (title: "rebased")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
bcwhite@chromium.org changed reviewers: + avi@chromium.org, mark@chromium.org, pennymac@google.com
Calls to track process begin/exit need to be added to various places in the code when processes begin and exit. There doesn't seem to be a single point for all of them. +mark: base/process/* +avi: content/app/* +pennymac: sandbox/win/*
base/process OK, LGTM
content lgtm
pennymac@chromium.org changed reviewers: + wfh@chromium.org - pennymac@google.com
On 2017/03/09 18:49:41, Avi wrote: > content lgtm Hi Brian, Process-startup tracking support FTW! I'm going to get wfh on here for his opinion on the sandbox-related breadcrumb. He looks at startup stability and stats all the time, so I want him in the loop on this (also to ensure anything we add is most helpful and not duplication). My quick take on your change in target_process.cc is that we probably want to move it out into src\content\common\sandbox_win.cc, StartSandboxedProcess(). This is where a bunch of TRACE_EVENTs and UMA_HISTOGRAM already live. This is also where creating and setting up the suspended process is controlled from, and where execution is finally handed off via ResumeThread. Cheers, Penny
On 2017/03/09 19:32:14, penny wrote: > On 2017/03/09 18:49:41, Avi wrote: > > content lgtm > > Hi Brian, > > Process-startup tracking support FTW! > > I'm going to get wfh on here for his opinion on the sandbox-related breadcrumb. > He looks at startup stability and stats all the time, so I want him in the loop > on this (also to ensure anything we add is most helpful and not duplication). > > My quick take on your change in target_process.cc is that we probably want to > move it out into src\content\common\sandbox_win.cc, StartSandboxedProcess(). > This is where a bunch of TRACE_EVENTs and UMA_HISTOGRAM already live. This is > also where creating and setting up the suspended process is controlled from, and > where execution is finally handed off via ResumeThread. > > Cheers, > Penny (Update: We definitely would want the breadcrumbs in our Chrome-specific code, and not in sandbox code that is used by downstream consumers.)
yes I agree with pennymac, moving this out to content::StartSandboxedProcess would be best here.
The CQ bit was checked by bcwhite@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...
On 2017/03/09 19:36:55, Will Harris wrote: > yes I agree with pennymac, moving this out to content::StartSandboxedProcess > would be best here. Done!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)
pennymac@chromium.org changed reviewers: + pennymac@chromium.org - wfh@chromium.org
On 2017/03/10 17:39:18, bcwhite wrote: > On 2017/03/09 19:36:55, Will Harris wrote: > > yes I agree with pennymac, moving this out to content::StartSandboxedProcess > > would be best here. > > Done! LGTM - content/common/sandbox_win.cc
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from manzagop@chromium.org, mark@chromium.org, avi@chromium.org Link to the patchset: https://codereview.chromium.org/2680123003/#ps300001 (title: "move tracking from target_process to sandbox_win")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)
The CQ bit was checked by bcwhite@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win10_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win10_chromium_x6...)
The CQ bit was checked by bcwhite@chromium.org
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": 300001, "attempt_start_ts": 1489684898911240,
"parent_rev": "42561dc4b39708c610a0149fb61b6f17c0e21f8e", "commit_rev":
"815054b5918e1d631c4208b06b83d1e833ef6240"}
Message was sent while issue was closed.
Description was changed from ========== Multi-Process Tracking Support Fully support having multiple processes write information to the same breadcrumbs file. This involves some process-specific operations plus the ability for a controlling process to clean-up after its dead children. BUG=620813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng ========== to ========== Multi-Process Tracking Support Fully support having multiple processes write information to the same breadcrumbs file. This involves some process-specific operations plus the ability for a controlling process to clean-up after its dead children. BUG=620813 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.win:win10_chromium_x64_rel_ng Review-Url: https://codereview.chromium.org/2680123003 Cr-Commit-Position: refs/heads/master@{#457502} Committed: https://chromium.googlesource.com/chromium/src/+/815054b5918e1d631c4208b06b83... ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001) as https://chromium.googlesource.com/chromium/src/+/815054b5918e1d631c4208b06b83... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
