Change the meaning of MobileSessionShutdownType's FIRST_LAUNCH_AFTER_UPGRADE bucket.

Change the meaning of MobileSessionShutdownType's FIRST_LAUNCH_AFTER_UPGRADE bucket.

This CL changes the last bucket to restrict to foreground crashes because of
upgrade instead of all terminations because of upgrade.

BUG=none
R=olivierrobin@chromium.org,stuartmorgan@chromium.org

[lpromero, 2015-07-21 15:43:48]

lpromero@chromium.org changed reviewers:
+ asvitkine@chromium.org

[lpromero, 2015-07-21 15:43:49]

asvitkine@chromium.org: Please review changes in histograms.xml.

[Alexei Svitkine (slow), 2015-07-21 15:48:45]

lgtm

[stuartmorgan, 2015-07-21 20:16:24]

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.h (right):

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.h:25:
SHUTDOWN_IN_FOREGROUND_UNKNOWN_LOG_STATE,
Seems like we should keep the fact that it's post-upgrade; it's a very specific
reason we have an unknown log state.

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right):

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:51: if
(IsFirstLaunchAfterUpgrade()) {
I realized after suggesting just flipping these that I'm not 100% certain
WasLashShutdownClean is still correct after an upgrade. Have you checked that
the first-new-lauch logic doesn't disturb this state?

[lpromero, 2015-07-21 22:12:07]

Olivier, can you explain here your idea?

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right):

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:51: if
(IsFirstLaunchAfterUpgrade()) {
On 2015/07/21 20:16:24, stuartmorgan wrote:
> I realized after suggesting just flipping these that I'm not 100% certain
> WasLashShutdownClean is still correct after an upgrade. Have you checked that
> the first-new-lauch logic doesn't disturb this state?

That's indeed to be checked. I will check that tomorrow.
IIRC, WasLastShutdownClean only reads a pref and I don't recall any special
treatment of this pref after an upgrade. We need to be sure.

BTW, Olivier raised the issue that this bucket is actually not necessary
anymore. It was there when logging the MobileSessionShutdownType was performed
in the main_controller, at the end of the startup sequence.
Now that MobileSessionShutdownType is correctly attributed to the correct
version (being in the initial stability log and not an ongoing log), we may not
need to account for this case.

[Olivier, 2015-07-22 16:54:19]

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right):

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:51: if
(IsFirstLaunchAfterUpgrade()) {
On 2015/07/21 22:12:07, lpromero wrote:
> On 2015/07/21 20:16:24, stuartmorgan wrote:
> > I realized after suggesting just flipping these that I'm not 100% certain
> > WasLashShutdownClean is still correct after an upgrade. Have you checked
that
> > the first-new-lauch logic doesn't disturb this state?
> 
> That's indeed to be checked. I will check that tomorrow.
> IIRC, WasLastShutdownClean only reads a pref and I don't recall any special
> treatment of this pref after an upgrade. We need to be sure.
> 
> BTW, Olivier raised the issue that this bucket is actually not necessary
> anymore. It was there when logging the MobileSessionShutdownType was performed
> in the main_controller, at the end of the startup sequence.
> Now that MobileSessionShutdownType is correctly attributed to the correct
> version (being in the initial stability log and not an ongoing log), we may
not
> need to account for this case.

If the WasLastShutdownClean is not reliable after an upgrade, we need to keep
the bucket like it was (include both foreground and background).

If it is reliable, this bucket was introduced to avoid attributing crashes from
previous version to the new version.
We just ignored the previous version shutdown, in the stats and that's it.
Now that the version of the histogram is correct, we could just consider the
shutdown as a normal one.

[stuartmorgan, 2015-07-28 22:27:43]

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.h (right):

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.h:25:
SHUTDOWN_IN_FOREGROUND_UNKNOWN_LOG_STATE,
On 2015/07/21 20:16:24, stuartmorgan wrote:
> Seems like we should keep the fact that it's post-upgrade; it's a very
specific
> reason we have an unknown log state.

I still think this is true.

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right):

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:51: if
(IsFirstLaunchAfterUpgrade()) {
On 2015/07/22 16:54:19, Olivier Robin wrote:
> On 2015/07/21 22:12:07, lpromero wrote:
> > On 2015/07/21 20:16:24, stuartmorgan wrote:
> > > I realized after suggesting just flipping these that I'm not 100% certain
> > > WasLashShutdownClean is still correct after an upgrade. Have you checked
> that
> > > the first-new-lauch logic doesn't disturb this state?
> > 
> > That's indeed to be checked. I will check that tomorrow.
> > IIRC, WasLastShutdownClean only reads a pref and I don't recall any special
> > treatment of this pref after an upgrade. We need to be sure.
> > 
> > BTW, Olivier raised the issue that this bucket is actually not necessary
> > anymore. It was there when logging the MobileSessionShutdownType was
performed
> > in the main_controller, at the end of the startup sequence.
> > Now that MobileSessionShutdownType is correctly attributed to the correct
> > version (being in the initial stability log and not an ongoing log), we may
> not
> > need to account for this case.
> 
> If the WasLastShutdownClean is not reliable after an upgrade, we need to keep
> the bucket like it was (include both foreground and background).
> 
> If it is reliable, this bucket was introduced to avoid attributing crashes
from
> previous version to the new version.
> We just ignored the previous version shutdown, in the stats and that's it.
> Now that the version of the histogram is correct, we could just consider the
> shutdown as a normal one.
> 

The comment for this block is still true AFAICT. MainController has:
  // Cleanup crash reports if this is the first run after an update.
  if (isFirstLaunchAfterUpgrade_) {
    [self scheduleCrashReportCleanup];
  }
So it seems like it's still the case that on first launch, the information we
have is different, and if we remove this state entirely then we'll incorrectly
put some crashes with logs into the no-log buckets. This is orthogonal to
WasLastShutdownClean being reliable.

[Olivier, 2015-07-29 08:01:27]

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right):

https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metr...
ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:51: if
(IsFirstLaunchAfterUpgrade()) {
On 2015/07/28 22:27:43, stuartmorgan wrote:
> On 2015/07/22 16:54:19, Olivier Robin wrote:
> > On 2015/07/21 22:12:07, lpromero wrote:
> > > On 2015/07/21 20:16:24, stuartmorgan wrote:
> > > > I realized after suggesting just flipping these that I'm not 100%
certain
> > > > WasLashShutdownClean is still correct after an upgrade. Have you checked
> > that
> > > > the first-new-lauch logic doesn't disturb this state?
> > > 
> > > That's indeed to be checked. I will check that tomorrow.
> > > IIRC, WasLastShutdownClean only reads a pref and I don't recall any
special
> > > treatment of this pref after an upgrade. We need to be sure.
> > > 
> > > BTW, Olivier raised the issue that this bucket is actually not necessary
> > > anymore. It was there when logging the MobileSessionShutdownType was
> performed
> > > in the main_controller, at the end of the startup sequence.
> > > Now that MobileSessionShutdownType is correctly attributed to the correct
> > > version (being in the initial stability log and not an ongoing log), we
may
> > not
> > > need to account for this case.
> > 
> > If the WasLastShutdownClean is not reliable after an upgrade, we need to
keep
> > the bucket like it was (include both foreground and background).
> > 
> > If it is reliable, this bucket was introduced to avoid attributing crashes
> from
> > previous version to the new version.
> > We just ignored the previous version shutdown, in the stats and that's it.
> > Now that the version of the histogram is correct, we could just consider the
> > shutdown as a normal one.
> > 
> 
> The comment for this block is still true AFAICT. MainController has:
>   // Cleanup crash reports if this is the first run after an update.
>   if (isFirstLaunchAfterUpgrade_) {
>     [self scheduleCrashReportCleanup];
>   }
> So it seems like it's still the case that on first launch, the information we
> have is different, and if we remove this state entirely then we'll incorrectly
> put some crashes with logs into the no-log buckets. This is orthogonal to
> WasLastShutdownClean being reliable.

 Even if we clean the reports, we can keep track that there were reports at
startup.

[lpromero, 2017-01-11 13:42:13]

Description was changed from

==========
Change the meaning of MobileSessionShutdownType's FIRST_LAUNCH_AFTER_UPGRADE
bucket.

This CL changes the last bucket to restrict to foreground crashes because of
upgrade instead of all terminations because of upgrade.

BUG=none
R=olivierrobin@chromium.org,stuartmorgan@chromium.org
==========

to

==========
Change the meaning of MobileSessionShutdownType's FIRST_LAUNCH_AFTER_UPGRADE
bucket.

This CL changes the last bucket to restrict to foreground crashes because of
upgrade instead of all terminations because of upgrade.

BUG=none
R=olivierrobin@chromium.org,stuartmorgan@chromium.org
==========