Reset the GCM store to recover from it when it is busted

Reset the GCM store to recover from it when it is busted

Also update the UMA to include details

BUG=435387
TEST=new test added

Committed: https://crrev.com/00b4600fb94befdc77ceb76ff2913b14fcc0cbeb
Cr-Commit-Position: refs/heads/master@{#315673}

[jianli, 2015-02-10 00:51:48]

jianli@chromium.org changed reviewers:
+ fgorski@chromium.org, zea@chromium.org

[jianli, 2015-02-10 00:52:54]

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

[jianli, 2015-02-10 00:52:54]

Adding asvitkine for histograms.xml

[Alexei Svitkine (slow), 2015-02-10 13:47:06]

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl.cc:12: #include "base/metrics/histogram.h"
Nit: Change this to histogram_macros.h instead.

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl.cc:447: "GCM.ResetStore",
INFINITE_STORE_RESET, RESET_STORE_ERROR_COUNT);
Nit: Make a function in the anon namespace for this and pass in the enum. (Each
macro expands to a big boilerplate, so ensuring there's only one per histogram
saves bloat).

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.cc:14: #include "base/metrics/histogram.h"
Nit: Change to histogram_macros.h

[fgorski, 2015-02-10 18:31:31]

lgtm with minor nits

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl_unittest.cc:624: // Restart GCM client.
The store should be resetted and the loading should
reset

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.cc:40: LOADING_DEVICE_CREDENTIAL_FAILED,
credentials

[Nicolas Zea, 2015-02-10 18:59:46]

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl.cc:656: "GCM.ResetStore",
DESTROYING_STORE_FAILED, RESET_STORE_ERROR_COUNT);
Why is this saying we failed? Shouldn't that be dependent on success?

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.cc:36: enum LoadStatus {
Do we really care to differentiate all these different failures? They all imply
directory corruption. I'm not sure what value we get out of this.

[jianli, 2015-02-10 19:44:32]

New patchsets have been uploaded after l-g-t-m from fgorski@chromium.org

[jianli, 2015-02-10 19:45:14]

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl.cc:12: #include "base/metrics/histogram.h"
On 2015/02/10 13:47:06, Alexei Svitkine wrote:
> Nit: Change this to histogram_macros.h instead.

Done.

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl.cc:447: "GCM.ResetStore",
INFINITE_STORE_RESET, RESET_STORE_ERROR_COUNT);
On 2015/02/10 13:47:06, Alexei Svitkine wrote:
> Nit: Make a function in the anon namespace for this and pass in the enum.
(Each
> macro expands to a big boilerplate, so ensuring there's only one per histogram
> saves bloat).

Done.

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl.cc:656: "GCM.ResetStore",
DESTROYING_STORE_FAILED, RESET_STORE_ERROR_COUNT);
On 2015/02/10 18:59:46, Nicolas Zea wrote:
> Why is this saying we failed? Shouldn't that be dependent on success?

My bad. It should be placed within if block.

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
File components/gcm_driver/gcm_client_impl_unittest.cc (right):

https://codereview.chromium.org/910093003/diff/1/components/gcm_driver/gcm_cl...
components/gcm_driver/gcm_client_impl_unittest.cc:624: // Restart GCM client.
The store should be resetted and the loading should
On 2015/02/10 18:31:31, fgorski wrote:
> reset

Done.

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.cc:14: #include "base/metrics/histogram.h"
On 2015/02/10 13:47:06, Alexei Svitkine wrote:
> Nit: Change to histogram_macros.h

Done.

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.cc:36: enum LoadStatus {
On 2015/02/10 18:59:46, Nicolas Zea wrote:
> Do we really care to differentiate all these different failures? They all
imply
> directory corruption. I'm not sure what value we get out of this.

I think we might need more info on the underlying failures. OPENING_STORE_FAILED
means that the leveldb store fails to open due to corruption or IO error. All
LOADING_***_FAILED are mostly related to parsing error. Since now we
automatically reset the store on error, we might not catch the parsing error
immediately when the client change is made. So I think it will be good to record
the UMA.

https://codereview.chromium.org/910093003/diff/1/google_apis/gcm/engine/gcm_s...
google_apis/gcm/engine/gcm_store_impl.cc:40: LOADING_DEVICE_CREDENTIAL_FAILED,
On 2015/02/10 18:31:31, fgorski wrote:
> credentials

Done.

[Alexei Svitkine (slow), 2015-02-10 19:51:25]

lgtm

https://codereview.chromium.org/910093003/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/910093003/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:192:
UMA_HISTOGRAM_ENUMERATION("GCM.ResetStore", error, RESET_STORE_ERROR_COUNT);
Nit: Indent 2 less.

[Nicolas Zea, 2015-02-10 21:07:37]

https://codereview.chromium.org/910093003/diff/20001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/910093003/diff/20001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:282:
UMA_HISTOGRAM_ENUMERATION("GCM.LoadStatus", load_status, LOAD_STATUS_COUNT);
Did you not want to record success as well? If so, perhaps name this and
describe it differently to reflect that this is just a breakdown of the errors.

[jianli, 2015-02-10 21:11:49]

New patchsets have been uploaded after l-g-t-m from asvitkine@chromium.org

[jianli, 2015-02-10 21:11:53]

https://codereview.chromium.org/910093003/diff/20001/components/gcm_driver/gc...
File components/gcm_driver/gcm_client_impl.cc (right):

https://codereview.chromium.org/910093003/diff/20001/components/gcm_driver/gc...
components/gcm_driver/gcm_client_impl.cc:192:
UMA_HISTOGRAM_ENUMERATION("GCM.ResetStore", error, RESET_STORE_ERROR_COUNT);
On 2015/02/10 19:51:24, Alexei Svitkine wrote:
> Nit: Indent 2 less.

Done.

https://codereview.chromium.org/910093003/diff/20001/google_apis/gcm/engine/g...
File google_apis/gcm/engine/gcm_store_impl.cc (right):

https://codereview.chromium.org/910093003/diff/20001/google_apis/gcm/engine/g...
google_apis/gcm/engine/gcm_store_impl.cc:282:
UMA_HISTOGRAM_ENUMERATION("GCM.LoadStatus", load_status, LOAD_STATUS_COUNT);
On 2015/02/10 21:07:37, Nicolas Zea wrote:
> Did you not want to record success as well? If so, perhaps name this and
> describe it differently to reflect that this is just a breakdown of the
errors.

Good catch. I do want to record the success. Move the UMA code outside if. I
think LoadStatus mean both success and error.

[Nicolas Zea, 2015-02-10 21:33:49]

lgtm

[jianli, 2015-02-10 21:50:52]

The CQ bit was checked by jianli@chromium.org

[commit-bot: I haz the power, 2015-02-10 21:51:22]

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

[commit-bot: I haz the power, 2015-02-10 23:33:00]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-02-10 23:33:39]

Patchset 3 (id:??) landed as
https://crrev.com/00b4600fb94befdc77ceb76ff2913b14fcc0cbeb
Cr-Commit-Position: refs/heads/master@{#315673}