After 24 hours, bust the browser cache when checking for ServiceWorker updates

After 24 hours, bust the browser cache when checking for ServiceWorker updates and fetching it's imports. Also bypass the browser cache for the initial registration.

BUG=371671
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=289891

[michaeln, 2014-08-09 02:16:11]

ptal, tests are still [wip]

[dominicc (has gone to gerrit), 2014-08-11 04:01:16]

nit

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:234: //
TODO(michaeln): If the last update check was less then 24 hours
then -> than

[falken, 2014-08-11 04:13:15]

This looks good to me.

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler.cc:64: //
checks after 24 hours have past.
grammar nit: s/past/passed

[michaeln, 2014-08-13 03:42:55]

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler.cc:64: //
checks after 24 hours have past.
On 2014/08/11 04:13:15, falken wrote:
> grammar nit: s/past/passed

Done.

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/457903002/diff/40001/content/browser/service_...
content/browser/service_worker/service_worker_register_job.cc:234: //
TODO(michaeln): If the last update check was less then 24 hours
On 2014/08/11 04:01:15, dominicc wrote:
> then -> than

Done.

[falken, 2014-08-13 04:43:05]

Thanks. Is this patch still WIP for tests?

[michaeln, 2014-08-14 02:39:27]

I still haven't put together tests for this but i'd like get it in prior to the
branch date, possibly w/o tests, because the behavior in here is important for
the thing.

[falken, 2014-08-14 02:53:30]

On 2014/08/14 02:39:27, michaeln wrote:
> I still haven't put together tests for this but i'd like get it in prior to
the
> branch date, possibly w/o tests, because the behavior in here is important for
> the thing.

OK LGTM

So to summarize our update functionality, including this patch:
- every page load with a ServiceWorker schedules an update job
- update jobs bypass the browser cache when fetching a SW if at least 24 hours
have elapsed since the last fetch
- initial register jobs always bypass the browser cache

is that right?

[dominicc (has gone to gerrit), 2014-08-14 07:03:37]

On 2014/08/14 02:39:27, michaeln wrote:
> I still haven't put together tests for this but i'd like get it in prior to
the
> branch date, possibly w/o tests, because the behavior in here is important for
> the thing.

The thing?

In everything I have observed, skipping writing tests to make a deadline is a
false economy.

[michaeln, 2014-08-15 01:15:42]

> In everything I have observed, skipping writing tests to make a deadline is a
> false economy.

Me2. Not skipping but deferring some. There are unittest now, but still no
system tests.

[michaeln, 2014-08-15 01:39:59]

> So to summarize our update functionality, including this patch:
> - every page load with a ServiceWorker schedules an update job
> - update jobs bypass the browser cache when fetching a SW if at least 24 hours
> have elapsed since the last fetch
> - initial register jobs always bypass the browser cache
> 
> is that right?

yes

https://codereview.chromium.org/457903002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/457903002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:64: //
checks after 24 hours have passed.
There's a problem with this right here. I think we should bust the cache for
imports too, but last_update_check has the new value when we're loading imports.

I'll add a TODO for that.

https://codereview.chromium.org/457903002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/457903002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:453: // Only bump
the last check time when we've bypassed the browser cache.
i changed this part since you last looked

[falken, 2014-08-15 02:13:56]

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
File
content/browser/service_worker/service_worker_context_request_handler_unittest.cc
(right):

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:54:
context()->AddProviderHost(host.Pass());
Can you clean it up in service_worker_controllee_request_handler_unittest.cc
also?

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:80:
TEST_F(ServiceWorkerContextRequestHandlerTest, SoftUpdateBefore24Hours) {
nit: I'm not sure about the naming of soft vs hard update. The spec's "soft
update" is just an update the browser triggers spontaneously, and also busts the
cache after 24 hours. Maybe the tests should just be "UpdateBefore24Hours" and
"UpdateAfter24Hours"?

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:81:
// Give the registration an very recent last update time and pretend
grammar nit: s/an/a

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:140:

nit: extra line

https://codereview.chromium.org/457903002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/457903002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:453: // Only bump
the last check time when we've bypassed the browser cache.
Nice catch! But this seems flawed also. Doesn't it mean if you cache a script,
let 24 hours pass, and the script never changes, you'll bypass the cache every
time?

Would it be better for ContextRequestHandler to update the check time whenever
it sets LOAD_BYPASS_CACHE?

[michaeln, 2014-08-15 02:53:24]

thnx for looking

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
File
content/browser/service_worker/service_worker_context_request_handler_unittest.cc
(right):

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:80:
TEST_F(ServiceWorkerContextRequestHandlerTest, SoftUpdateBefore24Hours) {
On 2014/08/15 02:13:55, falken wrote:
> nit: I'm not sure about the naming of soft vs hard update. The spec's "soft
> update" is just an update the browser triggers spontaneously, and also busts
the
> cache after 24 hours. Maybe the tests should just be "UpdateBefore24Hours" and
> "UpdateAfter24Hours"?

Done.

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:81:
// Give the registration an very recent last update time and pretend
On 2014/08/15 02:13:56, falken wrote:
> grammar nit: s/an/a

Done.

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:140:

On 2014/08/15 02:13:56, falken wrote:
> nit: extra line

Done.

https://codereview.chromium.org/457903002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_context_request_handler_unittest.cc:140:

On 2014/08/15 02:13:56, falken wrote:
> nit: extra line

Done.

https://codereview.chromium.org/457903002/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_context_request_handler.cc
(right):

https://codereview.chromium.org/457903002/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_context_request_handler.cc:64: //
checks after 24 hours have passed.
On 2014/08/15 01:39:59, michaeln wrote:
> There's a problem with this right here. I think we should bust the cache for
> imports too, but last_update_check has the new value when we're loading
imports.

I take that back, when the main script is found to be the same, the job stops at
hat point and no imports are loaded, the worker never starts in that case.
Removing the comment and TODO.

https://codereview.chromium.org/457903002/diff/120001/content/browser/service...
File content/browser/service_worker/service_worker_register_job.cc (right):

https://codereview.chromium.org/457903002/diff/120001/content/browser/service...
content/browser/service_worker/service_worker_register_job.cc:453: // Only bump
the last check time when we've bypassed the browser cache.
On 2014/08/15 02:13:56, falken wrote:
> Nice catch! But this seems flawed also. Doesn't it mean if you cache a script,
> let 24 hours pass, and the script never changes, you'll bypass the cache every
> time?

I don't think so?
- cache script
- wait a day
- update check will bypass the cache
- script is the same, so we set a new last_update_check time
- update check 10 minutes later, won't bypass cache and won't update the last
time
- update check a day later, will bypass cache and if equal, will update the last
time and if not equal will install a new version

If you wait 24 hours between each check, you'll bypass the cache every time.


> Would it be better for ContextRequestHandler to update the check time whenever
> it sets LOAD_BYPASS_CACHE?

The request could fail and that class doesn't have enough context to know if the
resources match.

[michaeln, 2014-08-15 03:04:37]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-08-15 03:08:42]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/457903002/140001

[commit-bot: I haz the power, 2014-08-15 16:05:36]

Committed patchset #8 (140001) as 289891

[mlamouri (slow - plz ping), 2014-08-27 10:17:48]

A revert of this CL (patchset #8) has been created in
https://codereview.chromium.org/511813002/ by mlamouri@chromium.org.

The reason for reverting is: Some SW tests are starting to randomly crash
following that CL.

See bug 404607..