Make metrics state machine more lenient about OLD vs. CURRENT

Make metrics state machine more lenient about OLD vs. CURRENT

Instead of DCHECKing (and then later crashing) if the state machine thinks it should be uploading old logs, but there aren't any, just advance the state and return.

This should prevent crashing in edge cases where something else has gone wrong and the state is out of sync.

Also change StageNextLogForUpload to CHECK instead of DCHECK that there is a next log before doing a swap(), since swap()ing in garbage pointers leads to hard-to-understand crashes later.

BUG=135285
TEST=None

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=146064

[stuartmorgan, 2012-07-02 13:25:25]

This is slightly speculative, but swapping in bad pointers in the staging is the
only explanation I can see for those crashes, and after staring at the code I
was able to convince myself that there are ways to get into a mismatched state
(convoluted ones, but still possible).

We've talked about removing OLD vs. CURRENT entirely; the fact that it's getting
so hard to reason seems like good motivation to do it, but I wanted to do a more
targeted fix for the crash and the follow-up with a simplification if you still
think we should do that.

[Ilya Sherman, 2012-07-09 16:12:54]

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (left):

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:759: state_ = SENDING_OLD_LOGS;
Hmm, why is this no longer needed?

[jar (doing other things), 2012-07-09 16:38:04]

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (left):

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:759: state_ = SENDING_OLD_LOGS;
Can you motivate this removal?

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:1161: state_ = SENDING_CURRENT_LOGS;
This seems nice, conservative, and defenive.

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:1162: scheduler_->UploadFinished(true
/* healthy */, false /* no unsent logs */);
Why do we need to do this now?  Won't this (eventually) get called after the
upload attempt we instigate below completes?

[stuartmorgan, 2012-07-10 07:26:25]

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (left):

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:759: state_ = SENDING_OLD_LOGS;
On 2012/07/09 16:38:04, jar wrote:
> Can you motivate this removal?

It was cruft almost the moment it was added; it landed almost simultaneously
with my change to move the state_-changing logic into
PushPendingLogsToPersistentStorage.

And it's wrong, because while the PPLTPS() version checks that there are
actually stored logs after storing (there might not be, if the logs were huge),
this doesn't and could thus be a way of incorrectly entering SENDING_OLD_LOGS.

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
File chrome/browser/metrics/metrics_service.cc (right):

https://chromiumcodereview.appspot.com/10692059/diff/1/chrome/browser/metrics...
chrome/browser/metrics/metrics_service.cc:1162: scheduler_->UploadFinished(true
/* healthy */, false /* no unsent logs */);
On 2012/07/09 16:38:04, jar wrote:
> Why do we need to do this now?  Won't this (eventually) get called after the
> upload attempt we instigate below completes?

Whoops, it's because I'm missing the |return| I intended to add. Fixed. (We want
to return rather than continue because if the callback was for sending an old
log, it was much shorter that we want for sending a new log.)

[jar (doing other things), 2012-07-10 16:06:52]

lgtm

[commit-bot: I haz the power, 2012-07-11 04:54:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/stuartmorgan@chromium.org/10692059/6001

[commit-bot: I haz the power, 2012-07-11 06:01:36]

Change committed as 146064