[CacheStorage] Updating cache size within the scheduled operation.

[CacheStorage] Updating cache size within the scheduled operation.

Previously scheduled cache mutation operations would begin the
size update calculation and then return to potentially begin
another mutation operation in parallel with the running size
update calculation. This could result in size held by CacheStorage
becoming mismatched with the actual cache size if Chrome were to
crash before the index could be written.

This change completes the size calculation before calling the
closure to begin the next scheduled operation.

BUG=678455
Review-Url: https://codereview.chromium.org/2613183003
Cr-Commit-Position: refs/heads/master@{#443059}
Committed: https://chromium.googlesource.com/chromium/src/+/3418f524d88d8851f26ea6a9ea0ac04cf6aea332

[cmumford, 2017-01-06 20:58:01]

The CQ bit was checked by cmumford@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-06 20:58:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cmumford, 2017-01-06 21:07:11]

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

[commit-bot: I haz the power, 2017-01-06 21:48:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-06 21:48:36]

Dry run: This issue passed the CQ dry run.

[Wez, 2017-01-07 02:21:44]

Is the cache-size stored atomically with each update to the cache?

If not then surely it can still get out-of-sync with the real cache size if we
crash between those writes?

[jkarlin, 2017-01-09 12:33:36]

lgtm

[jkarlin, 2017-01-09 12:34:41]

On 2017/01/07 02:21:44, Wez wrote:
> Is the cache-size stored atomically with each update to the cache?
> 
> If not then surely it can still get out-of-sync with the real cache size if we
> crash between those writes?

It's not atomic, but if the browser crashes between writes we'll detect that on
next start-up (by differences in last-modified values) and recalculate.

[cmumford, 2017-01-09 18:15:56]

On 2017/01/09 12:34:41, jkarlin wrote:
> On 2017/01/07 02:21:44, Wez wrote:
> > Is the cache-size stored atomically with each update to the cache?
> > 
> > If not then surely it can still get out-of-sync with the real cache size if
we
> > crash between those writes?
> 
> It's not atomic, but if the browser crashes between writes we'll detect that
on
> next start-up (by differences in last-modified values) and recalculate.

correct: not atomic, the cache size is written to the index store in a a second
write. However, the index out-of-date check will throw away any sizes from  the
index if the cache modification date is >= index modification date.

I'm theorizing that something like this is happening:

1. Write to Cache A.
2. Start async size calculation for A.
3. Write to Cache B.
4. Start async size calculation for B.
5. Cache A size was calculated.
6. Index (in memory) is updated and write is scheduled.
7. Scheduled index is written (with new A and old B sizes).

<- Crash here!

8. Cache B size was calculated.
9. Index (in memory) is updated and write is scheduled.

In the above scenario the index will be newer than both A and B, but the index
will have the old B size.

With this CL, if I'm not mistaken, there is no opportunity for the index to be
written to disk with an out-of-date cache size *and with* the cach on disk being
modified.

1. Write to Cache A.
2. Start async size calculation for A.
3. Cache A size was calculated.
4. Index (in memory) is updated and write is scheduled.
5. Scheduled index is written (with new A and old B sizes).
6. Write to Cache B.
7. Start async size calculation for B.
8. Cache B size was calculated.
9. Index (in memory) is updated and write is scheduled.

The program should be able to crash anywhere in this timeline and still be
consistent. Do you see any flaws in my understanding?

[cmumford, 2017-01-11 18:59:50]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:00:46]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 19:58:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-11 19:58:53]

Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[cmumford, 2017-01-11 23:13:39]

The CQ bit was checked by cmumford@chromium.org

[commit-bot: I haz the power, 2017-01-11 23:14:34]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-01-11 23:50:18]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1484176419516370, "parent_rev":
"a908d62ac9640cc8e9e54521c8ab8133919ef24a", "commit_rev":
"3418f524d88d8851f26ea6a9ea0ac04cf6aea332"}

[commit-bot: I haz the power, 2017-01-11 23:51:38]

Description was changed from

==========
[CacheStorage] Updating cache size within the scheduled operation.

Previously scheduled cache mutation operations would begin the
size update calculation and then return to potentially begin
another mutation operation in parallel with the running size
update calculation. This could result in size held by CacheStorage
becoming mismatched with the actual cache size if Chrome were to
crash before the index could be written.

This change completes the size calculation before calling the
closure to begin the next scheduled operation.

BUG=678455
==========

to

==========
[CacheStorage] Updating cache size within the scheduled operation.

Previously scheduled cache mutation operations would begin the
size update calculation and then return to potentially begin
another mutation operation in parallel with the running size
update calculation. This could result in size held by CacheStorage
becoming mismatched with the actual cache size if Chrome were to
crash before the index could be written.

This change completes the size calculation before calling the
closure to begin the next scheduled operation.

BUG=678455

Review-Url: https://codereview.chromium.org/2613183003
Cr-Commit-Position: refs/heads/master@{#443059}
Committed:
https://chromium.googlesource.com/chromium/src/+/3418f524d88d8851f26ea6a9ea0a...
==========

[commit-bot: I haz the power, 2017-01-11 23:51:39]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/3418f524d88d8851f26ea6a9ea0a...