Instrument some of the exit paths likely to suffer hangs.

Instrument some of the exit paths likely to suffer hangs.

This is temporary instrumentation to try and narrow down the area
and mode where Chrome is (likely) hanging on logoff/restart.

TBR=mpearson@chromium.org,erikwright@chromium.org

BUG=412384
Committed: https://crrev.com/079c5a333e8a2dfd792b1cbf288151fcc42fe3f7
Cr-Commit-Position: refs/heads/master@{#308222}

[Sigurður Ásgeirsson, 2014-12-11 20:36:42]

siggi@chromium.org changed reviewers:
+ sky@chromium.org

[Sigurður Ásgeirsson, 2014-12-11 20:37:37]

Hey Scott,

PTAL - I'd try and land this tomorrow and let it ride over the weekend to get
some data for next week, if possible.

Siggi

[sky, 2014-12-11 23:36:23]

LGTM with the following change

https://codereview.chromium.org/796963002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/796963002/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:662: if (should_quit_if_last_browser &&
chrome::ShouldStartShutdown(this))
you need to add {} here.

[Sigurður Ásgeirsson, 2014-12-12 20:09:23]

Hey Scott,

so that CL wasn't my proudest moment. It was missing the final instrumentation
point, and didn't compile :/.
Care to take another quick look?

Siggi

https://codereview.chromium.org/796963002/diff/1/chrome/browser/ui/browser.cc
File chrome/browser/ui/browser.cc (right):

https://codereview.chromium.org/796963002/diff/1/chrome/browser/ui/browser.cc...
chrome/browser/ui/browser.cc:662: if (should_quit_if_last_browser &&
chrome::ShouldStartShutdown(this))
On 2014/12/11 23:36:22, sky wrote:
> you need to add {} here.

Phew - thanks. One thing I hadn't done yet is run any of this code :/

[sky, 2014-12-12 20:52:49]

sky@chromium.org changed reviewers:
+ cpu@chromium.org

[sky, 2014-12-12 20:52:49]

I'm LGTM ing this. You should get cpu to look at the chrome_watcher change.

+cpu

[Sigurður Ásgeirsson, 2014-12-12 22:43:47]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-12-12 22:45:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796963002/20001

[commit-bot: I haz the power, 2014-12-13 00:02:17]

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

[commit-bot: I haz the power, 2014-12-13 00:02:18]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[sky, 2014-12-13 00:29:49]

Don't commit until cpu approves this.

[Sigurður Ásgeirsson, 2014-12-13 01:12:23]

'K

On Fri Dec 12 2014 at 7:29:50 PM <sky@chromium.org> wrote:

> Don't commit until cpu approves this.
>
> https://codereview.chromium.org/796963002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[cpu_(ooo_6.6-7.5), 2014-12-13 01:17:34]

consider rvargas review as my review.

[rvargas (doing something else), 2014-12-13 01:32:52]

rvargas@chromium.org changed reviewers:
+ rvargas@chromium.org

[rvargas (doing something else), 2014-12-13 01:32:53]

lgtm

https://codereview.chromium.org/796963002/diff/20001/chrome/chrome_watcher/ch...
File chrome/chrome_watcher/chrome_watcher_main.cc (right):

https://codereview.chromium.org/796963002/diff/20001/chrome/chrome_watcher/ch...
chrome/chrome_watcher/chrome_watcher_main.cc:47: base::ProcessHandle dupe =
base::kNullProcessHandle;
It would be nice to use base::Process here... but that can be done afterwards.

[Sigurður Ásgeirsson, 2014-12-13 01:35:17]

The CQ bit was checked by siggi@chromium.org

[commit-bot: I haz the power, 2014-12-13 01:37:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/796963002/20001

[Sigurður Ásgeirsson, 2014-12-13 01:56:53]

siggi@chromium.org changed reviewers:
+ erikwright@chromium.org, mpearson@google.com

[Sigurður Ásgeirsson, 2014-12-13 01:56:53]

Hey Erik, Mark,

I'm TBRing you guys on this for histograms.xml and
components/browser_watcher.gypi respectively. I hope you don't mind, but I'm
really itching for some data over the weekend.

Siggi

[commit-bot: I haz the power, 2014-12-13 02:02:46]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2014-12-13 02:03:32]

Patchset 2 (id:??) landed as
https://crrev.com/079c5a333e8a2dfd792b1cbf288151fcc42fe3f7
Cr-Commit-Position: refs/heads/master@{#308222}

[Mark P, 2014-12-15 19:01:54]

mpearson@chromium.org changed reviewers:
+ mpearson@chromium.org

[Mark P, 2014-12-15 19:01:55]

https://codereview.chromium.org/796963002/diff/20001/tools/metrics/histograms...
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/796963002/diff/20001/tools/metrics/histograms...
tools/metrics/histograms/histograms.xml:35443: +  <summary>Temporary
instrumentation. See http://crbug.com/412384.</summary>
This is unsatisfactory.

Do none of this histograms have any future use, ever?  Will you remove them
_all_?  I am okay with bad histogram descriptions if they're going to be marked
as obsolete real soon now.  Please create a new bug assigned to you targeting
M-41 (is that where the crashes started appearing and that's where you have to
make any fixes?) to clean up the histogram recording and histograms
descriptions.

[Sigurður Ásgeirsson, 2014-12-15 23:16:08]

Hey Mark,

filed http://crbug.com/442526.

This is really intended as a very short term measure, I hope I can rip them
out within the week.
These "histograms" are not going to be useful in histogram view at all. To
reconstruct the exit path of a Chrome instance, I'll have to dremel out the
stability report from raw logs.

Siggi

On Mon Dec 15 2014 at 2:01:56 PM <mpearson@chromium.org> wrote:

>
> https://codereview.chromium.org/796963002/diff/20001/
> tools/metrics/histograms/histograms.xml
> File tools/metrics/histograms/histograms.xml (right):
>
> https://codereview.chromium.org/796963002/diff/20001/
> tools/metrics/histograms/histograms.xml#newcode35443
> tools/metrics/histograms/histograms.xml:35443
>
<https://codereview.chromium.org/796963002/diff/20001/tools/metrics/histograms...>:
> +  <summary>Temporary
> instrumentation. See http://crbug.com/412384.</summary>
> This is unsatisfactory.
>
> Do none of this histograms have any future use, ever?  Will you remove
> them _all_?  I am okay with bad histogram descriptions if they're going
> to be marked as obsolete real soon now.  Please create a new bug
> assigned to you targeting M-41 (is that where the crashes started
> appearing and that's where you have to make any fixes?) to clean up the
> histogram recording and histograms descriptions.
>
> https://codereview.chromium.org/796963002/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.