Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2393853002:
Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton. (Closed)
|
Created:
4 years, 2 months ago by Tobias Sargeant Modified:
3 years, 2 months ago Reviewers:
Bernhard Bauer, Torne, michaelbai, Simeon, boliu, alokp, Lei Zhang, halliwell, Robert Sesek, Mike West, mfarrar03 CC:
alokp+watch_chromium.org, android-webview-reviews_chromium.org, boliu, chromium-reviews, darin-cc_chromium.org, einbinder+watch-test-runner_chromium.org, halliwell+watch_chromium.org, jochen+watch_chromium.org, kalyank, lcwu+watch_chromium.org, mlamouri+watch-content_chromium.org, mlamouri+watch-test-runner_chromium.org, Peter Beverloo, sadrul Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionRefactor CrashDump*Manager to use a shared CrashDumpObserver singleton.
CrashDumpManager watches for generated microdumps when child processes
die, and CrashMicroDumpManager is responsible for aborting the WebView
browser process. Both share similarities that can be combined, and doing
so allows us to eventually support generating a minidump and then dying
in WebView.
BUG=633979
Review-Url: https://codereview.chromium.org/2393853002
Cr-Commit-Position: refs/heads/master@{#442539}
Committed: https://chromium.googlesource.com/chromium/src/+/5b457a448b5f4c2e4fb620f39a495e35fef486c5
Patch Set 1 #Patch Set 2 : Cleanups, pre re-review #
Total comments: 13
Patch Set 3 : use a LazyInstance, have BrowserMainParts own clients, not observer #Patch Set 4 : fix initialization in ShellBrowserMainParts #
Total comments: 39
Patch Set 5 : rebase; review changes #
Total comments: 2
Patch Set 6 : Review changes: remove refcounting, ability to unregister clients, move responsibility for posting to clients #
Total comments: 8
Patch Set 7 : Make posted task methods static. Fix nits in #44 #Patch Set 8 : minor diff cleanups, reinstate ChromeBrowserMainPartsAndroid dtor #
Total comments: 6
Patch Set 9 : Rebase #Messages
Total messages: 79 (51 generated)
tobiasjs@chromium.org changed reviewers: + thestig@chromium.org
Hi Lei, I'd like to revive this CL. The original CL that got reverted is discussed here: https://codereview.chromium.org/2200693002/, but I figure that it's been long enough, and enough things changed in order to address the issues that caused the revert that it should start with a clean state. Could you please take a look before I loop in the other reviewers?
The CQ bit was checked by tobiasjs@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_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The problems found in https://codereview.chromium.org/2304733002/ look like they got addressed, so I would recommend going back to the reviewers from https://codereview.chromium.org/2200693002/ . It would be nice to explain what changed since the previous attempt. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.cc:28: CrashDumpObserver* CrashDumpObserver::instance_ = NULL; nit: nullptr
On 2016/10/05 16:57:45, Lei Zhang wrote: > The problems found in https://codereview.chromium.org/2304733002/ look like they > got addressed, so I would recommend going back to the reviewers from > https://codereview.chromium.org/2200693002/ . That's my intention, although I thought maybe you could review the components/crash part too? > It would be nice to explain what changed since the previous attempt. In summary, I: * Removed the implicit creation of CrashDumpObserver and stopped using a singleton to hold the instance, in favour of BrowserMainParts subclasses managing its lifetime. * Converted CrashDumpObserver::Client to be a RefCountedThreadSafe, as OnChildStart and OnChildEnd need to occur on different pools (with OnChildStart being synchronous during GetAdditionalMappedFilesForChildProcess, and OnChildEnd being posted to the blocking thread pool. * Fixed the issues with respect to the checking of breakpad::IsCrashReporterEnabled(); > https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... > File components/crash/content/browser/crash_dump_observer_android.cc (right): > > https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... > components/crash/content/browser/crash_dump_observer_android.cc:28: > CrashDumpObserver* CrashDumpObserver::instance_ = NULL; > nit: nullptr Ack. Will change.
Looks ok. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_manager_android.cc (left): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:94: DCHECK_CURRENTLY_ON(BrowserThread::FILE); I'm curious what thread CrashDumpManager actually lives on. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.cc:77: void CrashDumpObserver::UnregisterClient(scoped_refptr<Client> client) { I don't see anyone ever calling this. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:23: // This class centralises the observation of child processes for the Can you say something about what thread this class lives on? https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:85: std::list<scoped_refptr<Client>> registered_clients_; As is, there's no advantage of having this as a list over a vector. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:87: static CrashDumpObserver* instance_; Can this just be declared in crash_dump_observer_android.cc instead? Can it be a LazyInstance?
https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_manager_android.cc (left): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_manager_android.cc:94: DCHECK_CURRENTLY_ON(BrowserThread::FILE); On 2016/10/05 18:00:00, Lei Zhang wrote: > I'm curious what thread CrashDumpManager actually lives on. It's created on the main thread, but callbacks occur on the process launcher thread, and the blocking thread pool. Destruction thread could be any of these, depending on who holds the last reference. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.cc:77: void CrashDumpObserver::UnregisterClient(scoped_refptr<Client> client) { On 2016/10/05 18:00:00, Lei Zhang wrote: > I don't see anyone ever calling this. True, but it seems like it's reasonable to have it there for symmetry, and we may end up using it in a follow-on change for WebView. I'm happy to add it later, when it's needed, if you feel that's best. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:23: // This class centralises the observation of child processes for the On 2016/10/05 18:00:00, Lei Zhang wrote: > Can you say something about what thread this class lives on? The observer lives on the browser main thread. (I'll add a comment for that). https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:87: static CrashDumpObserver* instance_; On 2016/10/05 18:00:00, Lei Zhang wrote: > Can this just be declared in crash_dump_observer_android.cc instead? Can it be a > LazyInstance? It could be done this way. Would it be preferable to do that, and and have the lifetime of the observer not be directly owned by BrowserMainParts instances? I guess that would mean that the individual clients then would be, and they would be explicitly unregistered.
The CQ bit was checked by tobiasjs@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: 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 tobiasjs@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_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
Patchset #3 (id:40001) has been deleted
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by tobiasjs@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
The CQ bit was checked by tobiasjs@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-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Adding back initial reviewers. This CL is largely the same as https://codereview.chromium.org/2200693002/, but with some changes to the observer and threading to address the issues that lead to it being reverted. Sorry for the extra review burden, and thanks in advance for taking another look. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:85: std::list<scoped_refptr<Client>> registered_clients_; On 2016/10/05 18:00:00, Lei Zhang wrote: > As is, there's no advantage of having this as a list over a vector. Have changed it. https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:87: static CrashDumpObserver* instance_; On 2016/10/06 17:42:22, Tobias Sargeant wrote: > On 2016/10/05 18:00:00, Lei Zhang wrote: > > Can this just be declared in crash_dump_observer_android.cc instead? Can it be > a > > LazyInstance? > > It could be done this way. Would it be preferable to do that, and and have the > lifetime of the observer not be directly owned by BrowserMainParts instances? I > guess that would mean that the individual clients then would be, and they would > be explicitly unregistered. PS4 does things this way.
https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.h:87: static CrashDumpObserver* instance_; On 2016/10/13 12:52:14, Tobias Sargeant wrote: > On 2016/10/06 17:42:22, Tobias Sargeant wrote: > > On 2016/10/05 18:00:00, Lei Zhang wrote: > > > Can this just be declared in crash_dump_observer_android.cc instead? Can it > be > > a > > > LazyInstance? > > > > It could be done this way. Would it be preferable to do that, and and have the > > lifetime of the observer not be directly owned by BrowserMainParts instances? > I > > guess that would mean that the individual clients then would be, and they > would > > be explicitly unregistered. > > PS4 does things this way. LazyInstance seems overkill and wrong, because this class needs to be constructed on the UI thread. https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... File android_webview/browser/aw_browser_main_parts.cc (right): https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... android_webview/browser/aw_browser_main_parts.cc:126: aw_browser_terminator_ = new AwBrowserTerminator()); that is some weird syntax.. write this on two lines please, because I dunno what the return type of this statement is https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:54: // We might get a NOTIFICATION_RENDERER_PROCESS_TERMINATED and a hmm, you probably want to document on the observer that STATUS_NORMAL_TERMINATION might happen twice? https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_android.cc:78: crash_dump_manager_ = new breakpad::CrashDumpManager( ditto https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2781: breakpad::CrashDumpObserver::GetInstance()->BrowserChildProcessStarted( so now we transfer the handles regardless of whether breakpad is enabled? intentional change? https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_main_parts.cc:395: crash_dump_manager_ = new breakpad::CrashDumpManager( ditto https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:39: return g_instance.Pointer(); If you have an explicit Create, you probably want to ensure it's called first before GetInstance is called? https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:63: base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool(); old implementation uses FILE thread? probably should avoid behavior changes for a refactor CL. you can always change this later? https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:92: client->OnChildStart(child_process_id, mappings); if a client decides to remove itself inside OnChildStart, that would deadlock.. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:59: void RegisterClient(scoped_refptr<Client> client); not sure if there's a new rule, but at least according to old rules, these should be const& https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:62: void BrowserChildProcessStarted(int child_process_id, this one seems odd because embedder is supposed to actually call this at the right time. also at this point, seems like uneeded complexity, each embedder can just call its platform-specific thing, be that CrashDumpManager or AwBrowserTerminator, without going through Observer here are you envisioning webview have both Terminator and CrashManager at some point? https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:87: std::vector<scoped_refptr<Client>> registered_clients_; there is only ever one client is there ever going to be more than one? https://codereview.chromium.org/2393853002/diff/100001/content/shell/browser/... File content/shell/browser/shell_browser_main_parts.cc (right): https://codereview.chromium.org/2393853002/diff/100001/content/shell/browser/... content/shell/browser/shell_browser_main_parts.cc:181: crash_dump_manager_ = new breakpad::CrashDumpManager( ditto
https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2781: breakpad::CrashDumpObserver::GetInstance()->BrowserChildProcessStarted( On 2016/10/13 21:07:18, boliu wrote: > so now we transfer the handles regardless of whether breakpad is enabled? > intentional change? Oops, read that one wrong, ignore..
https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:33: std::unique_ptr<base::SyncSocket> local_pipe(new base::SyncSocket()); auto local_pipe = base::MakeUnique<base::SyncSocket>()? :) https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_main_parts.h (right): https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_main_parts.h:20: #if defined(OS_ANDROID) Nit: in the interest of simplicity, you might want to leave out the #ifdef. A superfluous forward-declaration is not going to hurt anyone. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:39: return g_instance.Pointer(); On 2016/10/13 21:07:19, boliu wrote: > If you have an explicit Create, you probably want to ensure it's called first > before GetInstance is called? And vice-versa, would it make sense to verify that no instance exists when Create() is called? https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:69: base::Bind(&Client::OnChildExit, client, child_process_id, pid, Actually, why do you need to post this to a background thread? You could just make it the responsibility of the client to handle the callback in the background. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:97: const content::ChildProcessData& data) { Can you add some DCHECKs for the threads the various methods are called on? https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:32: class Client : public base::RefCountedThreadSafe<Client> { Hm, it would be nice not to make the refcountedness part of the interface (see also my comment in the .cc file). https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:56: CrashDumpObserver(); Can you make constructor and destructor private? And then add comments about the lifetime and ownership of this class? :) https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:59: void RegisterClient(scoped_refptr<Client> client); On 2016/10/13 21:07:19, boliu wrote: > not sure if there's a new rule, but at least according to old rules, these > should be const& If the method takes a reference, the rule is to pass a scoped_refptr. It can be std::move()d to avoid ref churn.
michaelbai@chromium.org changed reviewers: + michaelbai@chromium.org
https://codereview.chromium.org/2393853002/diff/120001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/120001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:67: pool->PostSequencedWorkerTask( WebView wants know this immediately, sets all WebView API noop and calls crash handle API. Code here might pass whether application should be crashed according the return value of crash handle API. This doesn't have to be done in this patch.
The CQ bit was checked by tobiasjs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
Patchset #6 (id:140001) has been deleted
Patchset #6 (id:160001) has been deleted
Patchset #6 (id:180001) has been deleted
In order to remove the refcounting of clients (which I agree is a better decision), some relatively extensive changes needed to be made. * The observer is created once on the UI thread, and lives until process exit. * Clients may be registered but not unregistered. The observer owns registered clients. * Client callbacks are no longer posted to another thread, but instead are called synchronously. Clients have to deal with posting to other threads as appropriate. https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... File android_webview/browser/aw_browser_main_parts.cc (right): https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... android_webview/browser/aw_browser_main_parts.cc:126: aw_browser_terminator_ = new AwBrowserTerminator()); On 2016/10/13 21:07:18, boliu wrote: > that is some weird syntax.. > > write this on two lines please, because I dunno what the return type of this > statement is Done. https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:33: std::unique_ptr<base::SyncSocket> local_pipe(new base::SyncSocket()); On 2016/10/14 10:41:01, Bernhard Bauer wrote: > auto local_pipe = base::MakeUnique<base::SyncSocket>()? :) This is moved code, but ok. https://codereview.chromium.org/2393853002/diff/100001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:54: // We might get a NOTIFICATION_RENDERER_PROCESS_TERMINATED and a On 2016/10/13 21:07:18, boliu wrote: > hmm, you probably want to document on the observer that > STATUS_NORMAL_TERMINATION might happen twice? Done. https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_browser_main_android.cc (right): https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_browser_main_android.cc:78: crash_dump_manager_ = new breakpad::CrashDumpManager( On 2016/10/13 21:07:18, boliu wrote: > ditto Done. https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... File chrome/browser/chrome_content_browser_client.cc (right): https://codereview.chromium.org/2393853002/diff/100001/chrome/browser/chrome_... chrome/browser/chrome_content_browser_client.cc:2781: breakpad::CrashDumpObserver::GetInstance()->BrowserChildProcessStarted( On 2016/10/13 21:08:50, boliu wrote: > On 2016/10/13 21:07:18, boliu wrote: > > so now we transfer the handles regardless of whether breakpad is enabled? > > intentional change? > > Oops, read that one wrong, ignore.. Acknowledged. https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_main_parts.cc (right): https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_main_parts.cc:395: crash_dump_manager_ = new breakpad::CrashDumpManager( On 2016/10/13 21:07:18, boliu wrote: > ditto Done. https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... File chromecast/browser/cast_browser_main_parts.h (right): https://codereview.chromium.org/2393853002/diff/100001/chromecast/browser/cas... chromecast/browser/cast_browser_main_parts.h:20: #if defined(OS_ANDROID) On 2016/10/14 10:41:01, Bernhard Bauer wrote: > Nit: in the interest of simplicity, you might want to leave out the #ifdef. A > superfluous forward-declaration is not going to hurt anyone. Done. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:39: return g_instance.Pointer(); On 2016/10/14 10:41:01, Bernhard Bauer wrote: > On 2016/10/13 21:07:19, boliu wrote: > > If you have an explicit Create, you probably want to ensure it's called first > > before GetInstance is called? > > And vice-versa, would it make sense to verify that no instance exists when > Create() is called? Done. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:63: base::SequencedWorkerPool* pool = BrowserThread::GetBlockingPool(); On 2016/10/13 21:07:19, boliu wrote: > old implementation uses FILE thread? > > probably should avoid behavior changes for a refactor CL. you can always change > this later? Done. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:69: base::Bind(&Client::OnChildExit, client, child_process_id, pid, On 2016/10/14 10:41:01, Bernhard Bauer wrote: > Actually, why do you need to post this to a background thread? You could just > make it the responsibility of the client to handle the callback in the > background. Per the discussion offline, the post has been removed from here. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:92: client->OnChildStart(child_process_id, mappings); On 2016/10/13 21:07:19, boliu wrote: > if a client decides to remove itself inside OnChildStart, that would deadlock.. Clients can't be removed now, but registering new clients could cause the same problem, and that's fixed. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:97: const content::ChildProcessData& data) { On 2016/10/14 10:41:01, Bernhard Bauer wrote: > Can you add some DCHECKs for the threads the various methods are called on? Done. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:32: class Client : public base::RefCountedThreadSafe<Client> { On 2016/10/14 10:41:01, Bernhard Bauer wrote: > Hm, it would be nice not to make the refcountedness part of the interface (see > also my comment in the .cc file). Done, as we discussed. Described above. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:56: CrashDumpObserver(); On 2016/10/14 10:41:02, Bernhard Bauer wrote: > Can you make constructor and destructor private? And then add comments about the > lifetime and ownership of this class? :) Done. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:59: void RegisterClient(scoped_refptr<Client> client); On 2016/10/14 10:41:01, Bernhard Bauer wrote: > On 2016/10/13 21:07:19, boliu wrote: > > not sure if there's a new rule, but at least according to old rules, these > > should be const& > > If the method takes a reference, the rule is to pass a scoped_refptr. It can be > std::move()d to avoid ref churn. Acknowledged. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:62: void BrowserChildProcessStarted(int child_process_id, On 2016/10/13 21:07:19, boliu wrote: > this one seems odd because embedder is supposed to actually call this at the > right time. > > also at this point, seems like uneeded complexity, each embedder can just call > its platform-specific thing, be that CrashDumpManager or AwBrowserTerminator, > without going through Observer here > > are you envisioning webview have both Terminator and CrashManager at some point? Yes, I am. So while you're correct that it's a bit odd, it makes informing multiple clients easier. https://codereview.chromium.org/2393853002/diff/100001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:87: std::vector<scoped_refptr<Client>> registered_clients_; On 2016/10/13 21:07:19, boliu wrote: > there is only ever one client > > is there ever going to be more than one? Yes. WebView will eventually have both CrashDumpManager and AwBrowserTerminator, and potentially a third Client that processes the renderer crash handling API. https://codereview.chromium.org/2393853002/diff/100001/content/shell/browser/... File content/shell/browser/shell_browser_main_parts.cc (right): https://codereview.chromium.org/2393853002/diff/100001/content/shell/browser/... content/shell/browser/shell_browser_main_parts.cc:181: crash_dump_manager_ = new breakpad::CrashDumpManager( On 2016/10/13 21:07:19, boliu wrote: > ditto Done. https://codereview.chromium.org/2393853002/diff/120001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/120001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:67: pool->PostSequencedWorkerTask( On 2016/11/29 23:31:32, michaelbai wrote: > WebView wants know this immediately, sets all WebView API noop and calls crash > handle API. Code here might pass whether application should be crashed accordinghttps://codereview.chromium.org/static/zippyplus.gif > the return value of crash handle API. > > This doesn't have to be done in this patch. The notifications this code relies upon are already delivered on the UI thread asynchronously, and the aim of this is to do I/O operations to process minidumps or to read from the exit code pipe, so must be posted to the FILE thread anyway. We also need to preserve the ordering of processing steps, once these steps are chained (we can't terminate the browser process before minidump management has happened or we lose crashes).
Thanks! LGTM with nits: https://codereview.chromium.org/2393853002/diff/200001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/200001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:83: base::Bind(&AwBrowserTerminator::ProcessTerminationStatus, Nit: Things have slightly changed in base::Bind land since your last patch set :) You could use base::BindOnce here now (which (a) will be The Future for this kind of stuff and (b) has the slight advantage of not requiring the base::Passed wrapper anymore, I think). https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:30: DCHECK(g_instance == nullptr); Maybe add a comment that if this check fails in a unit test, someone who was previously creating the object forgot to use ShadowingAtExitManager? That might not be totally obvious. https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:52: base::AutoLock auto_lock(registered_clients_lock_); I'm not sure you really need the lock here -- if the object is being destroyed, no one should be using it anymore anyway. https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:56: // Fetch a pointer to the global CrashDumpObserver instance. The Nit: empty line before the comment?
https://codereview.chromium.org/2393853002/diff/200001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/200001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:83: base::Bind(&AwBrowserTerminator::ProcessTerminationStatus, On 2016/12/08 17:41:25, Bernhard Bauer wrote: > Nit: Things have slightly changed in base::Bind land since your last patch set > :) > > You could use base::BindOnce here now (which (a) will be The Future for this > kind of stuff and (b) has the slight advantage of not requiring the base::Passed > wrapper anymore, I think). Thanks for the tip. I've left this as it is, because for the moment PostTask won't accept a OnceCallback. https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:30: DCHECK(g_instance == nullptr); On 2016/12/08 17:41:25, Bernhard Bauer wrote: > Maybe add a comment that if this check fails in a unit test, someone who was > previously creating the object forgot to use ShadowingAtExitManager? That might > not be totally obvious. Done. https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.cc:52: base::AutoLock auto_lock(registered_clients_lock_); On 2016/12/08 17:41:25, Bernhard Bauer wrote: > I'm not sure you really need the lock here -- if the object is being destroyed, > no one should be using it anymore anyway. Removed it (and the explicit clear of registered_clients_). I guess with or without this there's an implicit requirement that the process launcher thread needs to be shut down before this destructor runs. https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/200001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:56: // Fetch a pointer to the global CrashDumpObserver instance. The On 2016/12/08 17:41:25, Bernhard Bauer wrote: > Nit: empty line before the comment? Done.
The CQ bit was checked by tobiasjs@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by tobiasjs@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.
lgtm
Description was changed from ========== Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton. CrashDumpManager watches for generated microdumps when child processes die, and CrashMicroDumpManager is responsible for aborting the WebView browser process. Both share similarities that can be combined, and doing so allows us to eventually support generating a minidump and then dying in WebView. BUG=633979 ========== to ========== Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton. CrashDumpManager watches for generated microdumps when child processes die, and CrashMicroDumpManager is responsible for aborting the WebView browser process. Both share similarities that can be combined, and doing so allows us to eventually support generating a minidump and then dying in WebView. BUG=633979 ==========
Robert, could you please take a look at components/crash?
https://codereview.chromium.org/2393853002/diff/240001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/240001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:53: // Create (on the UI thread), and lives until process exit. Tests Rather than making this a LazyInstance, would it be possible to just have this live as a member in the various ContentBrowserClients?
https://codereview.chromium.org/2393853002/diff/240001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/240001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:53: // Create (on the UI thread), and lives until process exit. Tests On 2016/12/14 22:44:14, Robert Sesek wrote: > Rather than making this a LazyInstance, would it be possible to just have this > live as a member in the various ContentBrowserClients? I tried doing this for AwContentBrowserClient, and it was definitely possible. In general though there's no consistent connection between subclasses of ContentBrowserClient and BrowserMainParts, so there's no one pattern that could be used here. Currently, for example, cast and shell do it backwards, with the BrowserMainParts holding the instance. And all of them rely on the CrashDumpManagerAndroid constructor stashing this in a global so that it can be fetched from other locations. I think just being a LazyInstance is preferable, because it makes it possible for the current situation to become much more consistent.
LGTM https://codereview.chromium.org/2393853002/diff/240001/components/crash/conte... File components/crash/content/browser/crash_dump_observer_android.h (right): https://codereview.chromium.org/2393853002/diff/240001/components/crash/conte... components/crash/content/browser/crash_dump_observer_android.h:53: // Create (on the UI thread), and lives until process exit. Tests On 2016/12/15 14:59:48, Tobias Sargeant wrote: > On 2016/12/14 22:44:14, Robert Sesek wrote: > > Rather than making this a LazyInstance, would it be possible to just have this > > live as a member in the various ContentBrowserClients? > > I tried doing this for AwContentBrowserClient, and it was definitely possible. > In general though there's no consistent connection between subclasses of > ContentBrowserClient and BrowserMainParts, so there's no one pattern that could > be used here. Currently, for example, cast and shell do it backwards, with the > BrowserMainParts holding the instance. And all of them rely on the > CrashDumpManagerAndroid constructor stashing this in a global so that it can be > fetched from other locations. I think just being a LazyInstance is preferable, > because it makes it possible for the current situation to become much more > consistent. OK, I thought that may be the case. Thanks for checking, though.
halliwell@, could you please take a look at chromecast/ mkwst@, could you please take a look at content/shell The changes are small, and you've LGTMed a previous iteration of this CL in https://codereview.chromium.org/2200693002/ Thanks.
https://codereview.chromium.org/2393853002/diff/240001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/240001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:44: void AwBrowserTerminator::ProcessTerminationStatus( Is it better to declare this method as static function in .cc file?
//content/shell LGTM (happy holidays! sorry for the delayed response)
https://codereview.chromium.org/2393853002/diff/240001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/240001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:44: void AwBrowserTerminator::ProcessTerminationStatus( On 2017/01/05 22:42:48, michaelbai wrote: > Is it better to declare this method as static function in .cc file? I'm equivocal about this. Do you have a strong reason to care one way or the other? I can't see any particular advice one way or the other in the style guide.
tobiasjs@chromium.org changed reviewers: + alokp@chromium.org
halliwell@ or alokp@, would you mind looking at //chromecast please? That's the last piece that needs approval.
The CQ bit was checked by tobiasjs@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/2393853002/diff/240001/android_webview/browse... File android_webview/browser/aw_browser_terminator.cc (right): https://codereview.chromium.org/2393853002/diff/240001/android_webview/browse... android_webview/browser/aw_browser_terminator.cc:44: void AwBrowserTerminator::ProcessTerminationStatus( On 2017/01/09 18:19:24, Tobias Sargeant wrote: > On 2017/01/05 22:42:48, michaelbai wrote: > > Is it better to declare this method as static function in .cc file? > > I'm equivocal about this. Do you have a strong reason to care one way or the > other? I can't see any particular advice one way or the other in the style > guide. I don't have the strong reason, basically it added unnecessary code in header file, if it is cc's static method, we don't need to modify 2 files when we change the signature, also, if more and more static methods are added, we don't ends up with a list of private static methods in header file. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
chromecast/ lgtm
The CQ bit was checked by tobiasjs@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from bauerb@chromium.org, torne@chromium.org, rsesek@chromium.org, mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2393853002/#ps260001 (title: "Rebase")
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": 260001, "attempt_start_ts": 1484040415980780,
"parent_rev": "dc0c3729d77398a6a36eb717f51b5a56bcad38dc", "commit_rev":
"5b457a448b5f4c2e4fb620f39a495e35fef486c5"}
Message was sent while issue was closed.
Description was changed from ========== Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton. CrashDumpManager watches for generated microdumps when child processes die, and CrashMicroDumpManager is responsible for aborting the WebView browser process. Both share similarities that can be combined, and doing so allows us to eventually support generating a minidump and then dying in WebView. BUG=633979 ========== to ========== Refactor CrashDump*Manager to use a shared CrashDumpObserver singleton. CrashDumpManager watches for generated microdumps when child processes die, and CrashMicroDumpManager is responsible for aborting the WebView browser process. Both share similarities that can be combined, and doing so allows us to eventually support generating a minidump and then dying in WebView. BUG=633979 Review-Url: https://codereview.chromium.org/2393853002 Cr-Commit-Position: refs/heads/master@{#442539} Committed: https://chromium.googlesource.com/chromium/src/+/5b457a448b5f4c2e4fb620f39a49... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:260001) as https://chromium.googlesource.com/chromium/src/+/5b457a448b5f4c2e4fb620f39a49...
Message was sent while issue was closed.
mfarrar03@gmail.com changed reviewers: + mfarrar03@gmail.com
Message was sent while issue was closed.
lgtm |
