1407245 - service worker 24-hour update cache bypass triggers too early

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect, P1)

Description

[Ben Kelly [:bkelly, not reviewing]]

The code for the 24 hour time check is using the wrong constant to convert from microseconds to seconds.  So we are bypassing the cache after only 86 seconds instead of the 86,400 seconds in a day.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fix service worker update 24-hour time check conversion from microseconds. r=asuth

We are converting from microseconds, not milliseconds.  We use a pref to test updates as its difficult to actually measure a 24-hour period in automation.

Comment 2

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 8917032 [details] [diff] [review]
Fix service worker update 24-hour time check conversion from microseconds. r=asuth

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

Restating:
* We store the following time-related values:
  * mCreationTimesStamp, a TimeStamp capturing the non-wallclock time that the SWRI instance was created.
  * mCreationTime, the (usec) PRTime capturing the wallclock time that the SWRI instance was created.
  * mLastUpdateTime, the (usec) PRTime capturing the wallclock time of the last update check time, set on the SWRI by SWM::LoadRegistration.
* The pair of creation times lets us create our own wallclock-ish time unit that can't jump around.  We use TimeStamp::Now()[1] and mCreationTimeStamp to derive the time delta and add it to mCreationTime.  Some context/a survey of our type gap was provided by :tt in/around https://bugzilla.mozilla.org/show_bug.cgi?id=1251238#c28.
* This is a standard units mismatch problem.

1: Docs suggest NowLowRes() may be more appropriate for performance reasons on Windows given our granularity.

::: dom/workers/ServiceWorkerRegistrationInfo.cpp
@@ +348,2 @@
>    const int64_t kSecondsPerDay = 86400;
>    const int64_t now =

I suggest renaming `now` to `nowUsec`.

@@ +351,5 @@
>                                           mCreationTimeStamp).ToMicroseconds());
>  
>    // now < mLastUpdateTime if the system time is reset between storing
>    // and loading mLastUpdateTime from ServiceWorkerRegistrar.
>    if (now < mLastUpdateTime ||

This is somewhat orthogonal, but it seems more logically sound to me to do the "did we time-warp" check in SWRI::SetLastUpdateTime().  Specifically, the setting would look more like:

  // Ignore update times in our future that suggest there was a clock problem.
  if (aTime <= PR_Now()) {
    mLastUpdateTime = aTime;
  }

The current implementation makes it possible to miss realizing that a time-warp occurred, adding some extra delay to the update check.  It doesn't seem like this is likely to be a problem, however.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: Fix service worker update 24-hour time check conversion from microseconds. r=asuth

I updated the variable name as requested.  I left the bits as-is for now.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/412da89cc8e7
Fix service worker update 24-hour time check conversion from microseconds. r=asuth

Comment 5

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

https://hg.mozilla.org/mozilla-central/rev/412da89cc8e7

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8917143 [details] [diff] [review]
Fix service worker update 24-hour time check conversion from microseconds. r=asuth

Approval Request Comment
[Feature/Bug causing the regression]: Service workers
[User impact if declined]: We trigger network service worker updates that bypass the http cache more frequently than we should.  This uses extra network and CPU.
[Is this code covered by automated tests?]: Its difficult to automate that we measure 24 hours correctly since tests don't run that long.  Updates are tested, but we bypass this calculation using a pref in the tests.  We verified the calculation code through inspection and manual testing.
[Has the fix been verified in Nightly?]: Its been in nightly for over a week now and I manually verified it.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: No
[Is the change risky?]: Minimal risk
[Why is the change risky/not risky?]: Its essentially replacing one constant for another and renaming a variable.  Its very low risk.
[String changes made/needed]: None

Comment 7

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

Comment on attachment 8917143 [details] [diff] [review]
Fix service worker update 24-hour time check conversion from microseconds. r=asuth

less CPU and network usage is good, Beta57+

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f3db43c2871f