Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1525023003:
Distinguish in the browser between renderer crashes and kills (Closed)
|
Created:
5 years ago by mnaganov (inactive) Modified:
4 years, 11 months ago CC:
chromium-reviews, darin-cc_chromium.org, jam, android-webview-reviews_chromium.org, sadrul, kalyank, Peter Wen, Yaron Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDistinguish in the browser between renderer crashes and kills
Use a pipe between a browser and a renderer, where the renderer
writes something in case of a termination from a catchable
signal. In case when the renderer is killed by the low memory
killer, nothing gets written into the pipe.
The browser then terminates itself in a fashion similar
to the renderer.
BUG=546009
Committed: https://crrev.com/f322c8fea0852d346caba09574a6b0774721f7ca
Cr-Commit-Position: refs/heads/master@{#368251}
Patch Set 1 #Patch Set 2 : Finalized, RFC #
Total comments: 8
Patch Set 3 : Comments addressed #Patch Set 4 : Add kDisableRendererBackgrounding flag #Patch Set 5 : Implemented detection of a volunatry renderer exit #
Total comments: 12
Patch Set 6 : Comments addressed #
Total comments: 2
Patch Set 7 : Sort out the case of wrong context size #
Total comments: 2
Patch Set 8 : Torne's comment addressed #
Total comments: 4
Patch Set 9 : Use base::SyncSocket #
Total comments: 6
Patch Set 10 : Comments addressed #Patch Set 11 : Remove std::move for a value in return #Messages
Total messages: 55 (18 generated)
mnaganov@chromium.org changed reviewers: + rsesek@chromium.org, torne@chromium.org
This is a working prototype (modulo actual browser process termination code) for determining whether a renderer has been killed or did it crash. Please tell me what do you think before I finalize it.
Description was changed from ========== [WIP] Distinguish in the browser between renderer crashes and kills ========== to ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing is written into the pipe. The browser then terminates itself in a fashion similar to the renderer. ==========
torne@: Please take a look.
https://codereview.chromium.org/1525023003/diff/20001/android_webview/browser... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/1525023003/diff/20001/android_webview/browser... android_webview/browser/aw_content_browser_client.cc:494: if (crash_signal_file.IsValid()) { Is this ever expected to not be valid? https://codereview.chromium.org/1525023003/diff/20001/android_webview/crash_r... File android_webview/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/1525023003/diff/20001/android_webview/crash_r... android_webview/crash_reporter/aw_microdump_crash_reporter.cc:47: DCHECK(dump_fd_ == -1); Seems like we should have something at a higher level to stop us trying to do this in multiprocess mode, as well as the dcheck here.. https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:894: g_microdump->set_crash_handler(WriteSignalCodeToPipe); When does the crash handler set her get called? After the dump has been generated? Ideally we would want to write the signal to the pipe as early as possible, in case we fail somehow while doing the dump.. https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:63: bool SocketPair(int* fd1, int* fd2) { Don't we have a utility somewhere that does this? Seems like this should be reusable :)
Description was changed from ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing is written into the pipe. The browser then terminates itself in a fashion similar to the renderer. ========== to ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing is written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ==========
Description was changed from ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing is written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ========== to ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing is written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ==========
Description was changed from ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing is written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ========== to ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing gets written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ==========
https://codereview.chromium.org/1525023003/diff/20001/android_webview/browser... File android_webview/browser/aw_content_browser_client.cc (right): https://codereview.chromium.org/1525023003/diff/20001/android_webview/browser... android_webview/browser/aw_content_browser_client.cc:494: if (crash_signal_file.IsValid()) { On 2015/12/16 14:48:35, Torne wrote: > Is this ever expected to not be valid? If socket pair creation fails, I suppose. https://codereview.chromium.org/1525023003/diff/20001/android_webview/crash_r... File android_webview/crash_reporter/aw_microdump_crash_reporter.cc (right): https://codereview.chromium.org/1525023003/diff/20001/android_webview/crash_r... android_webview/crash_reporter/aw_microdump_crash_reporter.cc:47: DCHECK(dump_fd_ == -1); On 2015/12/16 14:48:35, Torne wrote: > Seems like we should have something at a higher level to stop us trying to do > this in multiprocess mode, as well as the dcheck here.. I've already done that in my previous patch: https://codereview.chromium.org/1518913002/. Unless the microdump engine is initialized with "webview" process type (which only happens in singleprocess mode), microdumps on demand are turned off (base::debug::SetDumpWithoutCrashingFunction remains unset). https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:894: g_microdump->set_crash_handler(WriteSignalCodeToPipe); On 2015/12/16 14:48:35, Torne wrote: > When does the crash handler set her get called? After the dump has been > generated? Ideally we would want to write the signal to the pipe as early as > possible, in case we fail somehow while doing the dump.. Before generating the dump, actually. In fact, the handler can even prevent it by returning "true". Only "FilterCallback" gets called earlier than "CrashHandler", but on the other hand, the filter doesn't receive the crash context. Although we don't use the context right now, but it might be useful for passing the signal number to the browser. https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/20001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:63: bool SocketPair(int* fd1, int* fd2) { On 2015/12/16 14:48:35, Torne wrote: > Don't we have a utility somewhere that does this? Seems like this should be > reusable :) It's good you've asked! I initially copied this code from ipc_channel_posix.cc, but now I have figured out that it's actually exported from there and can be reused here.
Ugh -- it seems that when we destroy the last WebView, as some tests do, the renderer may just happily exit, which is then handled as if it was killed. I need to check how can we distinguish a voluntary renderer exit from a kill.
Copied a bit more code from CrashDumpManager, the tests should be good now.
On 2015/12/17 01:15:52, mnaganov wrote: > Copied a bit more code from CrashDumpManager, the tests should be good now.
On 2015/12/17 16:15:06, Peter Wen wrote: > On 2015/12/17 01:15:52, mnaganov wrote: > > Copied a bit more code from CrashDumpManager, the tests should be good now.
wnwen@chromium.org changed reviewers: + wnwen@chromium.org
Sorry, my rietveld interface just spasmed. Seems like you're working from an older version of crash_dump_manager_android?
On 2015/12/17 16:16:29, Peter Wen wrote: > Sorry, my rietveld interface just spasmed. > > Seems like you're working from an older version of crash_dump_manager_android? Sorry, I don't understand the question. The goal is to produce a different crash manager for WebView. In WebView, we use "microdumps" that simply output a small crash dump into Android log, thus we don't need to deal with crash dump files. Second, CrashDumpManager uses ApplicationStatus, which doesn't work for WebView. Third, on a child process crash / exit, we may also need to terminate the browser process. I worked from CrashDumpManager because our manager also needs to listen to child process exit notifications, but it deals with them in a different manner. I took the ToT version of it.
This can be very useful for chrome as well. Do you think it can be incorporated into chrome proper through crash_dump_manager_android rather than a new crash dump manager? See http://crbug.com/510327. Please ignore my prior comment, misread and didn't realize you were adding a new crash dump manager.
wnwen@chromium.org changed reviewers: - wnwen@chromium.org
On 2015/12/17 16:27:06, Peter Wen wrote: > This can be very useful for chrome as well. Do you think it can be incorporated > into chrome proper through crash_dump_manager_android rather than a new crash > dump manager? See http://crbug.com/510327. > Some functionality (like listening to the notifications) can for sure be extracted into a base class. Although it's quite trivial in fact. I'll consider that later. > Please ignore my prior comment, misread and didn't realize you were adding a new > crash dump manager.
wnwen@chromium.org changed reviewers: + wnwen@chromium.org
Chatted offline with Mikhail. Webview plans to use this new crash dump manager which only deals in a pipe between renderer and browser to determine renderer exit status. I'll see about implementing this in chrome later. @mnaganov - Are you planning to add stats to see how this works out? i.e. what exit statuses are being written to the pipe and how often it exits normally vs killed? https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:890: } else if (!process_type.empty()) { I presume this will capture all webview renderer processes and nothing else? Not sure if webview also has gpu processes etc. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:86: base::ScopedFD pipe(pipe_fd); nit: not necessary here, can move further down if exiting right after. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:95: // that it didn't crash, thus we need to perform a clean exit. Please add the implied OOM SIGKILL to be clear for posterity what "didn't crash" means. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:102: void CrashMicroDumpManager::BrowserChildProcessHostDisconnected( Same as above, are these necessary in webview? https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:126: // minidump_fd we kept open. Old comments. :) https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:133: if (rph->FastShutdownStarted()) { Does webview use fast shutdown? If so this may result in termination status being MAX_ENUM when it's actually normal and expected.
Thanks, Peter! > @mnaganov - Are you planning to add stats to see how this works out? i.e. what exit statuses are being written to the pipe and how often it exits normally vs killed? Currently we don't have UMA in WebView. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/app/breakpad_linux.cc:890: } else if (!process_type.empty()) { On 2015/12/17 16:47:09, Peter Wen wrote: > I presume this will capture all webview renderer processes and nothing else? Not > sure if webview also has gpu processes etc. WebView can only have renderer child processes. This is guarded in AwContentBrowserClient::AppendExtraCommandLineSwitches. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:86: base::ScopedFD pipe(pipe_fd); On 2015/12/17 16:47:10, Peter Wen wrote: > nit: not necessary here, can move further down if exiting right after. It's necessary to close the pipe fd in the case if it was a voluntary child process exit. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:95: // that it didn't crash, thus we need to perform a clean exit. On 2015/12/17 16:47:09, Peter Wen wrote: > Please add the implied OOM SIGKILL to be clear for posterity what "didn't crash" > means. Done. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:102: void CrashMicroDumpManager::BrowserChildProcessHostDisconnected( On 2015/12/17 16:47:09, Peter Wen wrote: > Same as above, are these necessary in webview? You are right. I missed the comment on BrowserChildProcessObserver that "render processes cannot be observed through this interface". Removed. https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:126: // minidump_fd we kept open. On 2015/12/17 16:47:09, Peter Wen wrote: > Old comments. :) Changed to pipe_fd. Thanks! https://codereview.chromium.org/1525023003/diff/80001/components/crash/conten... components/crash/content/browser/crash_micro_dump_manager_android.cc:133: if (rph->FastShutdownStarted()) { On 2015/12/17 16:47:09, Peter Wen wrote: > Does webview use fast shutdown? If so this may result in termination status > being MAX_ENUM when it's actually normal and expected. Good point -- we forgot to update the code in our ContentBrowserClient to support it in multi-process mode. Fixed.
torne@, wnwen@: Are you happy with this change, or do you have any concerns?
lgtm %question. https://codereview.chromium.org/1525023003/diff/100001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:749: return false; If context size is wrong, then should this be treated differently than OOM kill, or is a clean exit fine? If nothing is written then the rest of the code assumes OOM kill.
https://codereview.chromium.org/1525023003/diff/100001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/100001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:749: return false; On 2015/12/21 14:49:24, Peter Wen wrote: > If context size is wrong, then should this be treated differently than OOM kill, > or is a clean exit fine? If nothing is written then the rest of the code assumes > OOM kill. If the context size is of an unexpected value, that means something is wrong going on here. Normally, I would just DCHECK this, but since this is a crash handler, I think it's better to write into the pipe some value that is out of the signal numbers range. Thanks for pointing this out!
LGTM in general, just one minor question https://codereview.chromium.org/1525023003/diff/120001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/120001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:749: int signo = 66; // The code that doesn't correspond to any real signal. Why 66 in particular? maximum int or something seems more reasonable as a magic value..
rsesek@: Can you please look at components/crash/ ? https://codereview.chromium.org/1525023003/diff/120001/components/crash/conte... File components/crash/content/app/breakpad_linux.cc (right): https://codereview.chromium.org/1525023003/diff/120001/components/crash/conte... components/crash/content/app/breakpad_linux.cc:749: int signo = 66; // The code that doesn't correspond to any real signal. On 2016/01/04 11:28:51, Torne wrote: > Why 66 in particular? maximum int or something seems more reasonable as a magic > value.. Done.
The CQ bit was checked by mnaganov@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/1525023003/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525023003/140001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromium_presubmit on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
yfriedman@chromium.org changed reviewers: + yfriedman@chromium.org
https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.h (right): https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.h:26: class CrashMicroDumpManager : public content::NotificationObserver { should this be in aw/? It only uses content/public and it's not actually used in chrome/ right?
https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.h (right): https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.h:26: class CrashMicroDumpManager : public content::NotificationObserver { On 2016/01/04 22:03:56, Yaron wrote: > should this be in aw/? It only uses content/public and it's not actually used in > chrome/ right? Microdumps are implemented in components/crash, and the crash handler code will have to remain in breakpad_linux.cc. I'm not sure it's a good idea splitting the implementation between android_webview and components/crash.
https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.cc:66: if (!IPC::SocketPair(&local_pipe_fd, &child_pipe_fd)) { Can you use base::SyncSocket instead of IPC::SocketPair and base::File?
https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/140001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.cc:66: if (!IPC::SocketPair(&local_pipe_fd, &child_pipe_fd)) { On 2016/01/05 15:58:45, Robert Sesek wrote: > Can you use base::SyncSocket instead of IPC::SocketPair and base::File? I guess, as we only write / read a couple of bytes through the pipe, and don't need synchronization (as the reader is guaranteed to act after the writer), there is no real difference whether we use blocking or non-blocking IO here. With base::File, doing PostTasks is more involved than with fds. I don't really see any benefits of using base::File here, as this code is platform-specific.
lgtm
rsesek@: Robert, are you insisting on implementing your proposal for using IPC::SocketPair and base::File, or do you think it's OK to use a non-blocking pipe and fds?
On 2016/01/06 21:08:44, mnaganov wrote: > rsesek@: Robert, are you insisting on implementing your proposal for using > IPC::SocketPair and base::File, or do you think it's OK to use a non-blocking > pipe and fds? The code doesn't really seem to make use of async-IO, so I'm not sure how useful O_NONBLOCK actually is. But the motivation behind my comment was around the types used, rather than the IO operations: - I worry about DEPSing in //ipc since this component is used by iOS, and even though it's annotated with a comment, it won't trip any automated checks. - base::File feels like the wrong type to use. Why not just base::ScopedFD everywhere?
On 2016/01/06 21:31:10, Robert Sesek wrote: > On 2016/01/06 21:08:44, mnaganov wrote: > > rsesek@: Robert, are you insisting on implementing your proposal for using > > IPC::SocketPair and base::File, or do you think it's OK to use a non-blocking > > pipe and fds? > > The code doesn't really seem to make use of async-IO, so I'm not sure how useful > O_NONBLOCK actually is. But the motivation behind my comment was around the > types used, rather than the IO operations: > > - I worry about DEPSing in //ipc since this component is used by iOS, and even > though it's annotated with a comment, it won't trip any automated checks. > - base::File feels like the wrong type to use. Why not just base::ScopedFD > everywhere? OK, I agree that dragging in a dependency on IPC can be avoided. I've changed the code according to your proposal, PTAL!
Thanks for removing the //ipc dependency! https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.cc:26: CrashMicroDumpManager* CrashMicroDumpManager::GetInstance() { This object is both constructible and has singleton-like behavior. I'd either make this a real base::Singleton that can then be warmed up in aw_browser_main_parts.cc; or remove the singleton-like behavior and hang it off a global object (like AwBrowserContext) that can then be accessed from aw_content_browser_client.cc. https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.cc:49: STLDeleteValues(&child_process_id_to_pipe_); If you switch to scoped_ptr<> in the map, you'll still want to hold the lock here, though, and call child_process_id_to_pipe_.clear(). https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.h (right): https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.h:59: std::map<int, base::SyncSocket*> child_process_id_to_pipe_; Now that we've got C++11, you can put the scoped_ptr<> directly into the map.
https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.cc (right): https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.cc:26: CrashMicroDumpManager* CrashMicroDumpManager::GetInstance() { On 2016/01/07 20:02:22, Robert Sesek wrote: > This object is both constructible and has singleton-like behavior. I'd either > make this a real base::Singleton that can then be warmed up in > aw_browser_main_parts.cc; or remove the singleton-like behavior and hang it off > a global object (like AwBrowserContext) that can then be accessed from > aw_content_browser_client.cc. AwBrowserContext isn't global enough -- we used to have only one instance of it because it is bound to an instance of RenderProcessHost. While the crash manager is a truly singleton, so I've converted it use base::Singleton. https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.cc:49: STLDeleteValues(&child_process_id_to_pipe_); On 2016/01/07 20:02:22, Robert Sesek wrote: > If you switch to scoped_ptr<> in the map, you'll still want to hold the lock > here, though, and call child_process_id_to_pipe_.clear(). Done. https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... File components/crash/content/browser/crash_micro_dump_manager_android.h (right): https://codereview.chromium.org/1525023003/diff/160001/components/crash/conte... components/crash/content/browser/crash_micro_dump_manager_android.h:59: std::map<int, base::SyncSocket*> child_process_id_to_pipe_; On 2016/01/07 20:02:22, Robert Sesek wrote: > Now that we've got C++11, you can put the scoped_ptr<> directly into the map. Right, thanks! For some reason I was thinking it's still not possible. Fixed.
LGTM
The CQ bit was checked by mnaganov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from wnwen@chromium.org, torne@chromium.org, yfriedman@chromium.org Link to the patchset: https://codereview.chromium.org/1525023003/#ps180001 (title: "Comments addressed")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525023003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525023003/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by mnaganov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org, wnwen@chromium.org, yfriedman@chromium.org, torne@chromium.org Link to the patchset: https://codereview.chromium.org/1525023003/#ps200001 (title: "Remove std::move for a value in return")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1525023003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1525023003/200001
Message was sent while issue was closed.
Description was changed from ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing gets written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ========== to ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing gets written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ==========
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Description was changed from ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing gets written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 ========== to ========== Distinguish in the browser between renderer crashes and kills Use a pipe between a browser and a renderer, where the renderer writes something in case of a termination from a catchable signal. In case when the renderer is killed by the low memory killer, nothing gets written into the pipe. The browser then terminates itself in a fashion similar to the renderer. BUG=546009 Committed: https://crrev.com/f322c8fea0852d346caba09574a6b0774721f7ca Cr-Commit-Position: refs/heads/master@{#368251} ==========
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/f322c8fea0852d346caba09574a6b0774721f7ca Cr-Commit-Position: refs/heads/master@{#368251} |
