Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2304733002:
crash: Fixes following r413431 (Closed)
Descriptioncrash: Fixes following r413431
Add back dropped call, and also uncomment commented out code.
BUG=633979, 642364
Patch Set 1 #
Total comments: 8
Patch Set 2 : remove #
Total comments: 2
Messages
Total messages: 22 (8 generated)
The CQ bit was checked by boliu@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...
boliu@chromium.org changed reviewers: + thestig@chromium.org
ptal (switching owner since rsesek is ooo)
:\ https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... File components/crash/content/browser/crash_dump_manager_android.cc (left): https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_manager_android.cc:42: //if (!breakpad::IsCrashReporterEnabled()) Whoa, what happened here? Do we need to merge this back to M54 so that we respect the user preferences re: crash reporting? https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_observer_android.cc:25: return base::Singleton<CrashDumpObserver>::get(); For later, I guess. Speaking of Singletons, is it really necessary? (See top of singleton.h) https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_observer_android.cc:35: BrowserChildProcessObserver::Add(this); Does the dtor need a BrowserChildProcessObserver::Remove(this) call to balance this? Or does it not matter because this class is a Singleton?
https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... File components/crash/content/browser/crash_dump_manager_android.cc (left): https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_manager_android.cc:42: //if (!breakpad::IsCrashReporterEnabled()) On 2016/09/01 23:17:58, Lei Zhang wrote: > Whoa, what happened here? Do we need to merge this back to M54 so that we > respect the user preferences re: crash reporting? yes, definitely needs to be merged https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_observer_android.cc:25: return base::Singleton<CrashDumpObserver>::get(); On 2016/09/01 23:17:58, Lei Zhang wrote: > For later, I guess. Speaking of Singletons, is it really necessary? (See top of > singleton.h) Err, I'll leave that one to original CL author. Probably possible to place inside some other global object (eg ContentBrowserClient?), but changing lifetime of this object could imply not catching crashes on shutdown? But otoh, this is android and there is no such thing as shutdown. https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_observer_android.cc:35: BrowserChildProcessObserver::Add(this); On 2016/09/01 23:17:58, Lei Zhang wrote: > Does the dtor need a BrowserChildProcessObserver::Remove(this) call to balance > this? Or does it not matter because this class is a Singleton? Ahh, good point. Done (Although.. this is android, globals are never going to be cleaned up..)
The CQ bit was checked by boliu@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...
thestig@chromium.org changed reviewers: + tobiasjs@chromium.org
Patch set 2 LGTM, but please get tobiasjs to look this over too. Not sure why the previous CL landed in that state. https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_observer_android.cc:25: return base::Singleton<CrashDumpObserver>::get(); On 2016/09/01 23:26:18, boliu wrote: > On 2016/09/01 23:17:58, Lei Zhang wrote: > > For later, I guess. Speaking of Singletons, is it really necessary? (See top > of > > singleton.h) > > Err, I'll leave that one to original CL author. Right, not directed at you. > Probably possible to place inside some other global object (eg > ContentBrowserClient?), but changing lifetime of this object could imply not > catching crashes on shutdown? > > But otoh, this is android and there is no such thing as shutdown. I was just thinking of using a LazyInstance instead. https://codereview.chromium.org/2304733002/diff/1/components/crash/content/br... components/crash/content/browser/crash_dump_observer_android.cc:35: BrowserChildProcessObserver::Add(this); On 2016/09/01 23:26:18, boliu wrote: > On 2016/09/01 23:17:58, Lei Zhang wrote: > > Does the dtor need a BrowserChildProcessObserver::Remove(this) call to balance > > this? Or does it not matter because this class is a Singleton? > > Ahh, good point. Done > > (Although.. this is android, globals are never going to be cleaned up..) Right, just wondering what the policy is on Android.
heh, all the android tests are crashing, and produced no useful debug information, yay
The CQ bit was unchecked by boliu@chromium.org
On 2016/09/02 00:42:32, boliu wrote: > heh, all the android tests are crashing, and produced no useful debug > information, yay Ahh, threading. Abort message: '[FATAL:browser_child_process_host_impl.cc(128)] Check failed: ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called on Chrome_UIThread; actually called on Chrome_ProcessLauncherThread.
On 2016/09/02 00:48:34, boliu wrote: > On 2016/09/02 00:42:32, boliu wrote: > > heh, all the android tests are crashing, and produced no useful debug > > information, yay > > Ahh, threading. > > Abort message: '[FATAL:browser_child_process_host_impl.cc(128)] Check failed: > ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called on > Chrome_UIThread; actually called on Chrome_ProcessLauncherThread. Indeed. There's no comments in the header to answer my question, and DCHECK_CURRENTLY_ON() isn't being used, so it is not obvious to me - what thread does CrashDumpObserver live on? Looks like it's trying to be runnable on any thread? Whoever calls CrashDumpObserver::GetInstance() first constructs the object on whatever thread they are on. Do we need an explicit call somewhere to warm up CrashDumpObserver on the right thread?
On 2016/09/02 00:48:34, boliu wrote: > On 2016/09/02 00:42:32, boliu wrote: > > heh, all the android tests are crashing, and produced no useful debug > > information, yay > > Ahh, threading. > > Abort message: '[FATAL:browser_child_process_host_impl.cc(128)] Check failed: > ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called on > Chrome_UIThread; actually called on Chrome_ProcessLauncherThread. This is due to where the breakpad enabled check is moved to. Before original CL, if breakpad is disabled, this registration would not happen; and if breakpad is enabled, the registration happens in PreCreateThreads. But now the refactor made it such that if breakpad is disabled, the global is created anyway, but on Chrome_ProcessLauncherThread. I was kinda surprised though that the content::NotificationRegistrar thing actually ok; it's thread local o_O
More nits for tobiasjs: https://codereview.chromium.org/2304733002/diff/20001/components/crash/conten... File components/crash/content/browser/crash_dump_observer_android.cc (right): https://codereview.chromium.org/2304733002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.cc:78: if (std::find(std::begin(registered_clients_), std::end(registered_clients_), BTW, isn't this base::ContainsValue() ? https://codereview.chromium.org/2304733002/diff/20001/components/crash/conten... components/crash/content/browser/crash_dump_observer_android.cc:94: for (auto& client : registered_clients_) { auto*
On 2016/09/02 01:04:12, Lei Zhang wrote: > On 2016/09/02 00:48:34, boliu wrote: > > On 2016/09/02 00:42:32, boliu wrote: > > > heh, all the android tests are crashing, and produced no useful debug > > > information, yay > > > > Ahh, threading. > > > > Abort message: '[FATAL:browser_child_process_host_impl.cc(128)] Check failed: > > ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called on > > Chrome_UIThread; actually called on Chrome_ProcessLauncherThread. > > Indeed. There's no comments in the header to answer my question, and > DCHECK_CURRENTLY_ON() isn't being used, so it is not obvious to me - what thread > does CrashDumpObserver live on? Looks like it's trying to be runnable on any > thread? > > Whoever calls CrashDumpObserver::GetInstance() first constructs the object on > whatever thread they are on. Do we need an explicit call somewhere to warm up > CrashDumpObserver on the right thread? I think observer itself is supposed to be thread safe since it's internally lock protected. But yeah, warm up needs to happen on UI thread. Actually that's another thing.. holding a lock and calling out to observers is a sketchy pattern too. Ok, there are enough issues here that I think I'll just revert the original and merge that to m54, assuming there are no conflcits..
On 2016/09/02 01:10:20, boliu wrote: > On 2016/09/02 01:04:12, Lei Zhang wrote: > > On 2016/09/02 00:48:34, boliu wrote: > > > On 2016/09/02 00:42:32, boliu wrote: > > > > heh, all the android tests are crashing, and produced no useful debug > > > > information, yay > > > > > > Ahh, threading. > > > > > > Abort message: '[FATAL:browser_child_process_host_impl.cc(128)] Check > failed: > > > ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called on > > > Chrome_UIThread; actually called on Chrome_ProcessLauncherThread. > > > > Indeed. There's no comments in the header to answer my question, and > > DCHECK_CURRENTLY_ON() isn't being used, so it is not obvious to me - what > thread > > does CrashDumpObserver live on? Looks like it's trying to be runnable on any > > thread? > > > > Whoever calls CrashDumpObserver::GetInstance() first constructs the object on > > whatever thread they are on. Do we need an explicit call somewhere to warm up > > CrashDumpObserver on the right thread? > > I think observer itself is supposed to be thread safe since it's internally lock > protected. But yeah, warm up needs to happen on UI thread. Actually that's > another thing.. holding a lock and calling out to observers is a sketchy pattern > too. > > Ok, there are enough issues here that I think I'll just revert the original and > merge that to m54, assuming there are no conflcits.. conflicts on trunk because a gypi file got deleted, but reverts fine on branch I'm gonna revert on branch then and assign bug back to toby for trunk
On 2016/09/02 01:18:27, boliu wrote: > On 2016/09/02 01:10:20, boliu wrote: > > On 2016/09/02 01:04:12, Lei Zhang wrote: > > > On 2016/09/02 00:48:34, boliu wrote: > > > > On 2016/09/02 00:42:32, boliu wrote: > > > > > heh, all the android tests are crashing, and produced no useful debug > > > > > information, yay > > > > > > > > Ahh, threading. > > > > > > > > Abort message: '[FATAL:browser_child_process_host_impl.cc(128)] Check > > failed: > > > > ::content::BrowserThread::CurrentlyOn(BrowserThread::UI). Must be called > on > > > > Chrome_UIThread; actually called on Chrome_ProcessLauncherThread. > > > > > > Indeed. There's no comments in the header to answer my question, and > > > DCHECK_CURRENTLY_ON() isn't being used, so it is not obvious to me - what > > thread > > > does CrashDumpObserver live on? Looks like it's trying to be runnable on any > > > thread? > > > > > > Whoever calls CrashDumpObserver::GetInstance() first constructs the object > on > > > whatever thread they are on. Do we need an explicit call somewhere to warm > up > > > CrashDumpObserver on the right thread? > > > > I think observer itself is supposed to be thread safe since it's internally > lock > > protected. But yeah, warm up needs to happen on UI thread. Actually that's > > another thing.. holding a lock and calling out to observers is a sketchy > pattern > > too. > > > > Ok, there are enough issues here that I think I'll just revert the original > and > > merge that to m54, assuming there are no conflcits.. > > conflicts on trunk because a gypi file got deleted, but reverts fine on branch > > I'm gonna revert on branch then and assign bug back to toby for trunk Or uhh, I'm gonna revert both. Just that revert on branch is an actual revert, not a merge from trunk due to that conflict.
Description was changed from ========== crash: Fixes following r413431 Add back dropped call, and also uncomment commented out code. BUG=633979, 642364 ========== to ========== crash: Fixes following r413431 Add back dropped call, and also uncomment commented out code. BUG=633979, 642364 ==========
Message was sent while issue was closed.
On 2016/09/02 01:19:50, boliu wrote: > Or uhh, I'm gonna revert both. Just that revert on branch is an actual revert, > not a merge from trunk due to that conflict. Ack. Sounds like we are not ready in M54. You'll have to work out the revert with your friendly neighborhood release manager.
Message was sent while issue was closed.
On 2016/09/02 01:23:43, Lei Zhang wrote: > On 2016/09/02 01:19:50, boliu wrote: > > Or uhh, I'm gonna revert both. Just that revert on branch is an actual revert, > > not a merge from trunk due to that conflict. > > Ack. Sounds like we are not ready in M54. You'll have to work out the revert > with your friendly neighborhood release manager. The friendly neighborhood release manager assigned that bug to me :p |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
