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

Issue 2380003002: GCM Store: Fix invalid argument errors (Closed)

Created:
4 years, 2 months ago by johnme
Modified:
4 years, 2 months ago
Reviewers:
Nicolas Zea
CC:
chromium-reviews, Peter Beverloo, johnme+watch_chromium.org, zea+watch_chromium.org
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

GCM Store: Fix invalid argument errors Opening the GCM leveldb database resulted in a high rate of "does not exist (create_if_missing is false)" invalid argument errors, since the GCM Store was incorrectly considering the database to already exist if its directory exists, when actually the directory sometimes continues to exist after the database is destroyed. This is expected to significantly reduce the following UMA buckets: - GCM.Database.Open "Invalid Argument" percent (from ~20% to < 1%) - GCM.LoadStatus "Store open failed" percent (from ~10% to < 1%) - GCM.ResetStore "Infinite store reset" count (from ~10% of GCM.Database.Open count to < 1%) BUG=650254 Committed: https://crrev.com/6d6c7809c0fff72b92a8038b7d65909b7449d3da Cr-Commit-Position: refs/heads/master@{#421957}

Patch Set 1 #

Total comments: 4

Patch Set 2 : Address review comments #

Patch Set 3 : Fix Win compile with FILE_PATH_LITERAL #

Unified diffs Side-by-side diffs Delta from patch set Stats (+86 lines, -13 lines) Patch
M components/gcm_driver/gcm_client_impl_unittest.cc View 1 2 2 chunks +45 lines, -0 lines 0 comments Download
M google_apis/gcm/engine/gcm_store_impl.cc View 1 2 2 chunks +8 lines, -1 line 0 comments Download
M google_apis/gcm/engine/gcm_store_impl_unittest.cc View 1 2 4 chunks +33 lines, -12 lines 0 comments Download

Messages

Total messages: 18 (9 generated)
johnme
There's more context on https://crbug.com/650254 about the analysis that led to this patch.
4 years, 2 months ago (2016-09-29 12:09:55 UTC) #2
Nicolas Zea
Nice find. LGTM with some nits. Also, is this a correctness fix? Or does it ...
4 years, 2 months ago (2016-09-29 16:49:57 UTC) #3
johnme
Addressed review comments - thanks! https://codereview.chromium.org/2380003002/diff/1/google_apis/gcm/engine/gcm_store_impl.cc File google_apis/gcm/engine/gcm_store_impl.cc (right): https://codereview.chromium.org/2380003002/diff/1/google_apis/gcm/engine/gcm_store_impl.cc#newcode186 google_apis/gcm/engine/gcm_store_impl.cc:186: base::PathExists(path.Append("CURRENT")); On 2016/09/29 16:49:57, ...
4 years, 2 months ago (2016-09-29 17:08:29 UTC) #4
johnme
> Also, is this a correctness fix? Or does it only affect UMA stats? Partially ...
4 years, 2 months ago (2016-09-29 17:13:30 UTC) #5
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2380003002/20001
4 years, 2 months ago (2016-09-29 17:13:54 UTC) #8
commit-bot: I haz the power
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_ng/builds/302146)
4 years, 2 months ago (2016-09-29 17:55:13 UTC) #10
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2380003002/40001
4 years, 2 months ago (2016-09-29 21:37:04 UTC) #15
commit-bot: I haz the power
Committed patchset #3 (id:40001)
4 years, 2 months ago (2016-09-29 22:28:35 UTC) #16
commit-bot: I haz the power
4 years, 2 months ago (2016-09-29 22:32:23 UTC) #18
Message was sent while issue was closed.
Patchset 3 (id:??) landed as
https://crrev.com/6d6c7809c0fff72b92a8038b7d65909b7449d3da
Cr-Commit-Position: refs/heads/master@{#421957}

Powered by Google App Engine
This is Rietveld 408576698