Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(241)

Issue 1246843002: Change the meaning of MobileSessionShutdownType's FIRST_LAUNCH_AFTER_UPGRADE bucket. (Closed)

Created:
5 years, 5 months ago by lpromero
Modified:
3 years, 11 months ago
CC:
chromium-reviews, asvitkine+watch_chromium.org, sdefresne+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : Keep deprecated enum value. Update histograms #

Total comments: 7
Unified diffs Side-by-side diffs Delta from patch set Stats (+34 lines, -31 lines) Patch
M ios/chrome/browser/metrics/ios_stability_metrics_provider.h View 1 1 chunk +2 lines, -1 line 2 comments Download
M ios/chrome/browser/metrics/ios_stability_metrics_provider.mm View 2 chunks +8 lines, -8 lines 5 comments Download
M ios/chrome/browser/metrics/ios_stability_metrics_provider_unittest.mm View 3 chunks +22 lines, -21 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 1 chunk +2 lines, -1 line 0 comments Download

Depends on Patchset:

Messages

Total messages: 10 (2 generated)
lpromero
5 years, 5 months ago (2015-07-20 22:37:42 UTC) #1
lpromero
asvitkine@chromium.org: Please review changes in histograms.xml.
5 years, 5 months ago (2015-07-21 15:43:49 UTC) #3
Alexei Svitkine (slow)
lgtm
5 years, 5 months ago (2015-07-21 15:48:45 UTC) #4
stuartmorgan
https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.h File ios/chrome/browser/metrics/ios_stability_metrics_provider.h (right): https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.h#newcode25 ios/chrome/browser/metrics/ios_stability_metrics_provider.h:25: SHUTDOWN_IN_FOREGROUND_UNKNOWN_LOG_STATE, Seems like we should keep the fact that ...
5 years, 5 months ago (2015-07-21 20:16:24 UTC) #5
lpromero
Olivier, can you explain here your idea? https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.mm File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right): https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.mm#newcode51 ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:51: if (IsFirstLaunchAfterUpgrade()) ...
5 years, 5 months ago (2015-07-21 22:12:07 UTC) #6
Olivier
https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.mm File ios/chrome/browser/metrics/ios_stability_metrics_provider.mm (right): https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.mm#newcode51 ios/chrome/browser/metrics/ios_stability_metrics_provider.mm:51: if (IsFirstLaunchAfterUpgrade()) { On 2015/07/21 22:12:07, lpromero wrote: > ...
5 years, 5 months ago (2015-07-22 16:54:19 UTC) #7
stuartmorgan
https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.h File ios/chrome/browser/metrics/ios_stability_metrics_provider.h (right): https://codereview.chromium.org/1246843002/diff/20001/ios/chrome/browser/metrics/ios_stability_metrics_provider.h#newcode25 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 ...
5 years, 4 months ago (2015-07-28 22:27:43 UTC) #8
Olivier
5 years, 4 months ago (2015-07-29 08:01:27 UTC) #9
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.

Powered by Google App Engine
This is Rietveld 408576698