CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

Unable to pinpoint why the crash is happening. Add print statements to help with future debugging attempts.

VLOGS enabled here: https://chromium-review.googlesource.com/c/457099/

TEST=none
BUG=660962

[sammiequon, 2017-02-28 23:15:43]

Description was changed from

==========
CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

Unable to pinpoint why the crash is happening. Add print statements to help with
future debugging attempts.

TEST=none
BUG=660962
==========

to

==========
CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

Unable to pinpoint why the crash is happening. Add print statements to help with
future debugging attempts.

TEST=none
BUG=660962
==========

[sammiequon, 2017-02-28 23:15:44]

sammiequon@chromium.org changed reviewers:
+ dgn@chromium.org, xdai@chromium.org

[sammiequon, 2017-03-01 00:54:06]

xdai@, dgn@ - Please take a look. Thanks! Do you think this will help track down
the crash next time it occurs?

[dgn, 2017-03-01 11:49:53]

thanks for looking into this, it would be helpful to debug what's happening,
yes. The DVLOGs also show interesting stuff to report, but they are quite heavy
and depend on debug only code. But maybe you could make it build into release
too?

https://codereview.chromium.org/2719813005/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/2719813005/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:164:
registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
why registering this here too? AFAICT all the observers on these 2 notifications
will be notified, so it's more about verifying in which order they are received?
Why not doing it in the places where we do something when receiving them
already?

[sammiequon, 2017-03-10 19:42:12]

Patchset #3 (id:40001) has been deleted

[sammiequon, 2017-03-10 22:09:26]

Patchset #3 (id:60001) has been deleted

[sammiequon, 2017-03-11 00:32:04]

The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-11 00:32:55]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-11 01:38:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-11 01:38:19]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[sammiequon, 2017-03-12 01:08:19]

The CQ bit was checked by sammiequon@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-12 01:08:31]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-12 02:20:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-12 02:20:22]

Dry run: This issue passed the CQ dry run.

[sammiequon, 2017-03-17 23:45:58]

Description was changed from

==========
CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

Unable to pinpoint why the crash is happening. Add print statements to help with
future debugging attempts.

TEST=none
BUG=660962
==========

to

==========
CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

Unable to pinpoint why the crash is happening. Add print statements to help with
future debugging attempts.

DVLOGS enabled here: https://chromium-review.googlesource.com/c/457099/

TEST=none
BUG=660962
==========

[sammiequon, 2017-03-18 00:01:39]

Sorry about the delay, left this one on the backburnner.

https://codereview.chromium.org/2719813005/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/2719813005/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:164:
registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
On 2017/03/01 11:49:53, dgn wrote:
> why registering this here too? AFAICT all the observers on these 2
notifications
> will be notified, so it's more about verifying in which order they are
received?
> Why not doing it in the places where we do something when receiving them
> already?

These are here to see if xiyuan@ 's hypothesis (on the bug) about these not be
handled is the cause of the crash.

[dgn, 2017-03-20 11:07:22]

non owner lgtm

https://codereview.chromium.org/2719813005/diff/20001/chrome/browser/push_mes...
File chrome/browser/push_messaging/push_messaging_service_impl.cc (right):

https://codereview.chromium.org/2719813005/diff/20001/chrome/browser/push_mes...
chrome/browser/push_messaging/push_messaging_service_impl.cc:164:
registrar_.Add(this, chrome::NOTIFICATION_APP_TERMINATING,
On 2017/03/18 00:01:39, sammiequon wrote:
> On 2017/03/01 11:49:53, dgn wrote:
> > why registering this here too? AFAICT all the observers on these 2
> notifications
> > will be notified, so it's more about verifying in which order they are
> received?
> > Why not doing it in the places where we do something when receiving them
> > already?
> 
> These are here to see if xiyuan@ 's hypothesis (on the bug) about these not be
> handled is the cause of the crash.

Acknowledged.

https://codereview.chromium.org/2719813005/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.cc (right):

https://codereview.chromium.org/2719813005/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.cc:58: DVLOG(1) <<
"ProfileNotification Terminating";
Shouldn't that be a VLOG rather than DVLOG? Otherwise the statement is compiled
out in release build and the vmodule flags won't do anything anyway?

(e.g. here they use vlogs:
https://cs.chromium.org/chromium/src/components/arc/arc_session.cc?type=cs&q=...)

[sammiequon, 2017-03-20 16:55:23]

sammiequon@chromium.org changed reviewers:
+ peter@chromium.org

[sammiequon, 2017-03-20 17:18:49]

sammiequon@chromium.org changed reviewers:
+ sky@chromium.org
- peter@chromium.org

[sammiequon, 2017-03-20 17:19:00]

Description was changed from

==========
CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

Unable to pinpoint why the crash is happening. Add print statements to help with
future debugging attempts.

DVLOGS enabled here: https://chromium-review.googlesource.com/c/457099/

TEST=none
BUG=660962
==========

to

==========
CrOS: Add print statements to help debug BrowserProcessImpl::Unpin crash.

Unable to pinpoint why the crash is happening. Add print statements to help with
future debugging attempts.

VLOGS enabled here: https://chromium-review.googlesource.com/c/457099/

TEST=none
BUG=660962
==========

[sammiequon, 2017-03-20 17:20:06]

Thanks! sky@ - Please take look.

https://codereview.chromium.org/2719813005/diff/80001/chrome/browser/notifica...
File chrome/browser/notifications/profile_notification.cc (right):

https://codereview.chromium.org/2719813005/diff/80001/chrome/browser/notifica...
chrome/browser/notifications/profile_notification.cc:58: DVLOG(1) <<
"ProfileNotification Terminating";
On 2017/03/20 11:07:22, dgn wrote:
> Shouldn't that be a VLOG rather than DVLOG? Otherwise the statement is
compiled
> out in release build and the vmodule flags won't do anything anyway?
> 
> (e.g. here they use vlogs:
>
https://cs.chromium.org/chromium/src/components/arc/arc_session.cc?type=cs&q=...)

Done.

[sky, 2017-03-20 20:00:00]

How is the logging going to help? By that I mean how are you going to get the
logs from actual customers?

[sammiequon, 2017-03-20 20:54:47]

On 2017/03/20 20:00:00, sky wrote:
> How is the logging going to help? By that I mean how are you going to get the
> logs from actual customers?

We would have to wait until somebody gets this crash again with the change, and
then download the file from the crash summary.

[sky, 2017-03-20 21:58:26]

Do we actually upload log files for chrome on chromeos? Also, if we
crash how do you know how much of the log you're going to get?

On Mon, Mar 20, 2017 at 1:54 PM,  <sammiequon@chromium.org> wrote:
> On 2017/03/20 20:00:00, sky wrote:
>> How is the logging going to help? By that I mean how are you going to get
>> the
>> logs from actual customers?
>
> We would have to wait until somebody gets this crash again with the change,
> and
> then download the file from the crash summary.
>
> https://codereview.chromium.org/2719813005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sammiequon, 2017-03-20 22:40:29]

On 2017/03/20 21:58:26, sky wrote:
> Do we actually upload log files for chrome on chromeos? Also, if we
> crash how do you know how much of the log you're going to get?
> 
> On Mon, Mar 20, 2017 at 1:54 PM,  <mailto:sammiequon@chromium.org> wrote:
> > On 2017/03/20 20:00:00, sky wrote:
> >> How is the logging going to help? By that I mean how are you going to get
> >> the
> >> logs from actual customers?
> >
> > We would have to wait until somebody gets this crash again with the change,
> > and
> > then download the file from the crash summary.
> >
> > https://codereview.chromium.org/2719813005/
> 
> -- 
> You received this message because you are subscribed to the Google Groups
> "Chromium-reviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email
> to mailto:chromium-reviews+unsubscribe@chromium.org.
> 

Yes we do have log files available from the crash website. I am not sure how
much of the log we are going to get :(. I am not very familiar with the logging
but any portion of the new logs will help with debugging this crash.

[sky, 2017-03-21 02:59:11]

Why not track more state so that when the crash happens you can have
it on the stack so you know it'll be available? By that I mean I
assume at some point you have a CHECK. In the function containing the
check you use base::debug::Alias(&data) that should then be available
in the crash.

  -Scott

On Mon, Mar 20, 2017 at 3:40 PM,  <sammiequon@chromium.org> wrote:
> On 2017/03/20 21:58:26, sky wrote:
>> Do we actually upload log files for chrome on chromeos? Also, if we
>> crash how do you know how much of the log you're going to get?
>>
>> On Mon, Mar 20, 2017 at 1:54 PM, <mailto:sammiequon@chromium.org> wrote:
>> > On 2017/03/20 20:00:00, sky wrote:
>> >> How is the logging going to help? By that I mean how are you going to
>> >> get
>> >> the
>> >> logs from actual customers?
>> >
>> > We would have to wait until somebody gets this crash again with the
>> > change,
>> > and
>> > then download the file from the crash summary.
>> >
>> > https://codereview.chromium.org/2719813005/
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "Chromium-reviews" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
> email
>> to mailto:chromium-reviews+unsubscribe@chromium.org.
>>
>
> Yes we do have log files available from the crash website. I am not sure how
> much of the log we are going to get :(. I am not very familiar with the
> logging
> but any portion of the new logs will help with debugging this crash.
>
> https://codereview.chromium.org/2719813005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sammiequon, 2017-03-22 01:18:07]

Hi sky@, thanks for the tips. I added a dump, but do you know how I can check
that the dump contains what it should?

[sky, 2017-03-22 03:49:45]

That I don't know. You could try someone on the crash team.

  -Scott

On Tue, Mar 21, 2017 at 6:18 PM,  <sammiequon@chromium.org> wrote:
> Hi sky@, thanks for the tips. I added a dump, but do you know how I can
> check
> that the dump contains what it should?
>
>
>
> https://codereview.chromium.org/2719813005/

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[sky, 2017-03-22 03:56:16]

Please don't add all the VLOGs. I don't think they'll help you locate the crash
and they have a way of staying around when we don't want them.

https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.cc (right):

https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:74: stream << *this;
Are you sure this returns something helpful?

https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.cc:87: VLOG_IF(1, registered_count_
> 0 || registered_keep_alives_.size() > 0)
!empty()

https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
File chrome/browser/lifetime/keep_alive_registry.h (right):

https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
chrome/browser/lifetime/keep_alive_registry.h:46: std::string ToString() const;
How about naming this more specifically, possibly ToDebugString()?

https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/notific...
File chrome/browser/notifications/profile_notification.cc (right):

https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/notific...
chrome/browser/notifications/profile_notification.cc:58: VLOG(1) <<
"ProfileNotification Terminating";
If you really need to log this information, add it to the call sites, not here.

[sammiequon, 2017-04-04 00:21:04]

On 2017/03/22 03:56:16, sky wrote:
> Please don't add all the VLOGs. I don't think they'll help you locate the
crash
> and they have a way of staying around when we don't want them.
> 
>
https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
> File chrome/browser/lifetime/keep_alive_registry.cc (right):
> 
>
https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
> chrome/browser/lifetime/keep_alive_registry.cc:74: stream << *this;
> Are you sure this returns something helpful?
> 
>
https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
> chrome/browser/lifetime/keep_alive_registry.cc:87: VLOG_IF(1,
registered_count_
> > 0 || registered_keep_alives_.size() > 0)
> !empty()
> 
>
https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
> File chrome/browser/lifetime/keep_alive_registry.h (right):
> 
>
https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/lifetim...
> chrome/browser/lifetime/keep_alive_registry.h:46: std::string ToString()
const;
> How about naming this more specifically, possibly ToDebugString()?
> 
>
https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/notific...
> File chrome/browser/notifications/profile_notification.cc (right):
> 
>
https://codereview.chromium.org/2719813005/diff/120001/chrome/browser/notific...
> chrome/browser/notifications/profile_notification.cc:58: VLOG(1) <<
> "ProfileNotification Terminating";
> If you really need to log this information, add it to the call sites, not
here.

Closing this in favour of https://codereview.chromium.org/2776933006/.