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

RESOLVED FIXED in Firefox 57

Status

()

defect
RESOLVED FIXED
2 years ago
a year ago

People

(Reporter: bkelly, Assigned: bkelly)

Tracking

(Blocks 1 bug)

unspecified
mozilla58
Points:
---
Bug Flags:
in-testsuite +
qe-verify -

Firefox Tracking Flags

(firefox56 wontfix, firefox57 fixed, firefox58 fixed)

Details

Attachments

(2 attachments)

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.
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 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.
Attachment #8921193 - Flags: review?(ttung)
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
Attachment #8921283 - Flags: review?(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!
Attachment #8921193 - Flags: review?(ttung) → review+
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!
Attachment #8921283 - Flags: review?(ttung) → review+

Comment 7

2 years ago
Pushed by bkelly@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/087e11c956e3
https://hg.mozilla.org/mozilla-central/rev/1abb70007b4c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
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
Attachment #8921193 - Flags: approval-mozilla-beta?
Comment on attachment 8921283 [details] [diff] [review]
P2 Verify ServiceWorkerInfo.lastUpdateTime is not changed when reading from http cache. r=tt

See comment 9.
Attachment #8921283 - Flags: approval-mozilla-beta?
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+
Attachment #8921193 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8921283 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.