[ServiceWorkerCache] Make the cache operations run serially.

[ServiceWorkerCache] Make the cache operations run serially.

There are correctness issues in the ServiceWorkerCache when multiple
operations occur on the same key in parallel. The various steps of the
operations can interleave. This CL serializes all cache operations by
placing all operations in a queue and calling the next operation from
the callback of the previous operation.

While this provides correctness, it is sub-optimal. We really only
need to serialize operations that affect the same entry. A smarter
scheduler can be created in the future to implement that (see
https://crbug.com/451174).

BUG=421226
Committed: https://crrev.com/52dafffb509c6ca4ef59b62f08a7ecdc2e179017
Cr-Commit-Position: refs/heads/master@{#313117}

[jkarlin, 2015-01-22 21:53:15]

jkarlin@chromium.org changed reviewers:
+ michaeln@chromium.org

[jkarlin, 2015-01-22 21:53:15]

Thanks!

[michaeln, 2015-01-23 00:55:25]

lgtm

https://codereview.chromium.org/863263003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/863263003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:1201: void
ServiceWorkerCache::InitBackend(const base::Closure& callback) {
The contract is a little odd, the 'callback' provided by the first caller to
InitBackend runs, others are ignored. Now that you have an explicit Q of pending
ops, maybe you don't need the callback param anymore. Would it make sense to
have InitDone run the front most op?

https://codereview.chromium.org/863263003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:1234: void
ServiceWorkerCache::RunOperationIfIdle() {
nit: this method is a little dangerous, if called twice in a row it will have
two operations in the running, maybe consider using a flag know when an op is
running, you could call RunOperationIfIdle() from within
CompleteOperationAndRunNext() and InitDone().

[jkarlin, 2015-01-23 18:12:28]

Patchset #6 (id:100001) has been deleted

[jkarlin, 2015-01-23 18:13:42]

Thanks michaeln. I've also updated Close a smidge, it now waits to change the
backend state once it is its turn to run. PTAL.

https://codereview.chromium.org/863263003/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_cache.cc (right):

https://codereview.chromium.org/863263003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:1201: void
ServiceWorkerCache::InitBackend(const base::Closure& callback) {
On 2015/01/23 00:55:25, michaeln wrote:
> The contract is a little odd, the 'callback' provided by the first caller to
> InitBackend runs, others are ignored. Now that you have an explicit Q of
pending
> ops, maybe you don't need the callback param anymore. Would it make sense to
> have InitDone run the front most op?

Done.

https://codereview.chromium.org/863263003/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_cache.cc:1234: void
ServiceWorkerCache::RunOperationIfIdle() {
On 2015/01/23 00:55:25, michaeln wrote:
> nit: this method is a little dangerous, if called twice in a row it will have
> two operations in the running, maybe consider using a flag know when an op is
> running, you could call RunOperationIfIdle() from within
> CompleteOperationAndRunNext() and InitDone().

Done.

[jkarlin, 2015-01-26 13:50:07]

PTAL if you get a chance michaeln, thanks!

[jkarlin, 2015-01-26 13:50:09]

PTAL if you get a chance michaeln, thanks!

[michaeln, 2015-01-26 19:13:23]

lgtm!

[jkarlin, 2015-01-26 19:14:05]

The CQ bit was checked by jkarlin@chromium.org

[commit-bot: I haz the power, 2015-01-26 19:14:56]

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

[commit-bot: I haz the power, 2015-01-26 20:26:04]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-01-26 20:27:56]

Patchset 6 (id:??) landed as
https://crrev.com/52dafffb509c6ca4ef59b62f08a7ecdc2e179017
Cr-Commit-Position: refs/heads/master@{#313117}