1410634 - service worker updates don't correctly determine if result is from http cache or not when updating last update time

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

The spec says we should only update the last service worker update time if we retrieve a value from the network.  If we get it from the http cache then we should not update the last update time.

Currently we try to do this in OnStreamComplete() here:

https://searchfox.org/mozilla-central/source/dom/workers/ServiceWorkerScriptCache.cpp#970

However, IsFromCache() always fails if called after OnStopRequest():

https://searchfox.org/mozilla-central/source/netwerk/base/nsICacheInfoChannel.idl#37

Do we always think the result comes from the network.  We need to call IsFromCache in the OnStartRequest() instead.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

This reduces the effectiveness of the 24 hour check which is an important escape hatch for content using service workers.  We should get this fixed in 57.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Call channel IsFromCache() during OnStartRequest() to determine if update result came from http cache or network. r=ttung

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9bffc01ba07e19a53054269acb00d6af9050db01

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8921193 [details] [diff] [review]
Call channel IsFromCache() during OnStartRequest() to determine if update result came from http cache or network. r=ttung

Tom, do you mind reviewing the service worker update patch?  We need to update our "last update time" only when we actually receive a new result from the network.  We don't want to update if we get an http cache result.

The current code, however, fails to do this because its call IsFromCache() too late.  According to the documentation its only valid to call between OnStartRequest() and OnStopRequest().  When we get our OnStreamCompleted() its too late because OnStopRequest() has already been called.

I'll see if I can add a test for this in a P2 patch.  I verified it works locally, though.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Verify ServiceWorkerInfo.lastUpdateTime is not changed when reading from http cache. r=tt

Tom, this adds a test check to verify we detect when the http cache is actually used.  It fails on current m-c and passes with the P1 patch.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4f43f6c67e354b55d3cdef2edcafe31ea82a64ea

Comment 5

[Tom Tung [:tt, :ttung]]

Comment on attachment 8921193 [details] [diff] [review]
Call channel IsFromCache() during OnStartRequest() to determine if update result came from http cache or network. r=ttung

Review of attachment 8921193 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM, thanks!

Comment 6

[Tom Tung [:tt, :ttung]]

Comment on attachment 8921283 [details] [diff] [review]
P2 Verify ServiceWorkerInfo.lastUpdateTime is not changed when reading from http cache. r=tt

Review of attachment 8921283 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for having a test to cover it!

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/087e11c956e3
P1 Call channel IsFromCache() during OnStartRequest() to determine if update result came from http cache or network. r=tt
https://hg.mozilla.org/integration/mozilla-inbound/rev/1abb70007b4c
P2 Verify ServiceWorkerInfo.lastUpdateTime is not changed when reading from http cache. r=tt

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/087e11c956e3
https://hg.mozilla.org/mozilla-central/rev/1abb70007b4c

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8921193 [details] [diff] [review]
Call channel IsFromCache() during OnStartRequest() to determine if update result came from http cache or network. r=ttung

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: Service workers have a safety mechanism that makes the update bypass http caching if we haven't checked the network result in 24 hours.  This prevents sites from becoming broken by a bad service worker for longer than 24 hours.  Without this fix, however, we may never trigger the safety mechanism because we think every update checks the network.  This creates a risk for sites using service workers in firefox that they may not be able to fix a broken deployment.
[Is this code covered by automated tests?]: Yes, I added a test in this bug.
[Has the fix been verified in Nightly?]: It just landed on nightly.  I verified the behavior locally as well.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Minimal
[Why is the change risky/not risky?]: This code just moves an existing IsFromCache() call earlier in the update process.  The call was always failing in its old call location, but now succeeds.  Now that it succeeds we simply avoid updating a timestamp.  There should be minimal regression risk from these changes.
[String changes made/needed]: None

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8921283 [details] [diff] [review]
P2 Verify ServiceWorkerInfo.lastUpdateTime is not changed when reading from http cache. r=tt

See comment 9.

Comment 11

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8921193 [details] [diff] [review]
Call channel IsFromCache() during OnStartRequest() to determine if update result came from http cache or network. r=ttung

Fixes our safety mechanism that helps sites fix broken deployments, Beta57+

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f16311e84f0d
https://hg.mozilla.org/releases/mozilla-beta/rev/f9071831e7d8

Comment 13

[Andrei Vaida [:avaida]]

(In reply to Ben Kelly [:bkelly] from comment #9)
> [Is this code covered by automated tests?]: Yes, I added a test in this bug.
> [Has the fix been verified in Nightly?]: It just landed on nightly.  I
> verified the behavior locally as well.
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Ben's assessment on manual testing needs and the fact that this fix has automated coverage.