Closed
Bug 1353636
Opened 7 years ago
Closed 7 years ago
Change ServiceWorker useCache to an enum and default to "imports"
Categories
(Core :: DOM: Service Workers, enhancement)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: bkelly, Assigned: bhsu)
References
(Blocks 1 open bug)
Details
Attachments
(4 files, 7 obsolete files)
2.37 KB,
patch
|
bkelly
:
review+
|
Details | Diff | Splinter Review |
24.54 KB,
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
3.98 KB,
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
25.32 KB,
patch
|
bhsu
:
review+
|
Details | Diff | Splinter Review |
In bug 1290944 we changed the service worker update check to use no-cache by default and provided a `useCache` boolean script could use to opt-in to http cache behavior. At the recent spec meeting, however, we decided that this is not a good idea when combined with checking all importScripts() during update. It seems that some 3rd party scripts being imported via importScripts() could be melted when this is turned on. The new solution is to use an enumeration instead of a boolean: https://github.com/w3c/ServiceWorker/issues/1104 The default will be no-cache for the top level script, but will use caching for importScripts(). This bug is to implement this new behavior. I will write a separate bug to prevent us from shipping the boolean `useCache` in FF53.
Assignee | ||
Comment 1•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bhsu
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Attachment #8854785 -
Attachment is obsolete: true
Reporter | ||
Comment 2•7 years ago
|
||
Note, Jake has written a wpt test for the updated spec: https://github.com/w3c/web-platform-tests/pull/5515 Ho-Pang, can you review that pull request and give any feedback to Jake? Thanks!
Flags: needinfo?(bhsu)
Assignee | ||
Comment 3•7 years ago
|
||
Sorry for the late response, I've updated it just now :)
Flags: needinfo?(bhsu)
Reporter | ||
Comment 4•7 years ago
|
||
Looks like the spec changes landed: https://github.com/w3c/ServiceWorker/pull/1107
Assignee | ||
Comment 5•7 years ago
|
||
Hi Ben, After working on it for a while, I'd like to separate this bug into sub-bugs, where, 1). Pass the testcase provided by Jake (only include the ones testing via ServiceWorkerContainer.register() api) 2). Expose ServiceWorkerRegistration::updateViaCache (WebAPI). 3). Update SERVICEWORKERREGISTRAR_VERSION and corresponding implementations. 4). Other stuff in the testcase provided by Jake. Since ServiceWorkerRegistration doesn't have a reference to its corresponding ServiceWorkerRegistrationInfo, we should acquire it via ServiceWorkerManager, and thus I'd like to do it in a separated bug. Do you think this plan acceptable?
Reporter | ||
Comment 6•7 years ago
|
||
> Do you think this plan acceptable?
Sounds good to me.
Assignee | ||
Comment 7•7 years ago
|
||
Just a simple update here. At this moment, I think I've what to pass phase I. However, there are something needed to be updated in the testcase provided by Jake, which I've commented on the github thread, so I am moving to the following phases.
Assignee | ||
Comment 8•7 years ago
|
||
In this patch, I use ServivceWorkerUpdateViaCache to replace the original LoadFlags which used for setting whether HttpCache should be bypassed when updating ServiceWorkers. One interesting thing about this patch is that we still save LoadFlags in ServiceWorkerInfo, since the LoadFlags only affect imported scripts. Thus, we can simply translate ServiceWorkerUpdateViaCache into the desired LoadFlags via the following table, and are able keep the implementation after byte-check remaining the same. ServiceWorkerUpdateViaCache | LoadFlags ----------------------------+---------------- Imports | LOAD_NORMAL All | LOAD_NORMAL None | VALIDATE_ALWAYS
Assignee | ||
Comment 9•7 years ago
|
||
This is another update, I've finished implementing phase 3. Now, I am waiting for the landing of new wpt-testcacse.
Reporter | ||
Comment 10•7 years ago
|
||
The WPT test cases have landed in upstream and James Graham tell me they are in inbound now. Hopefully they will be merged to m-c by the time you wake up tomorrow.
Assignee | ||
Comment 11•7 years ago
|
||
Assignee | ||
Comment 12•7 years ago
|
||
Assignee | ||
Comment 13•7 years ago
|
||
Assignee | ||
Comment 14•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Attachment #8881456 -
Attachment is obsolete: true
Assignee | ||
Comment 15•7 years ago
|
||
Comment on attachment 8894420 [details] [diff] [review] P1: Move from nsLoadFlags to UpdateViaCache Hi Ben, and thanks for your help This part is the phase 1 in Comment 5, where its details are written in comment 8. May I ask for your review?
Attachment #8894420 -
Flags: review?(bkelly)
Assignee | ||
Comment 16•7 years ago
|
||
Comment on attachment 8894421 [details] [diff] [review] P2: Expose ServieworkerRegistration.updateViaCache This part is for the phase 2 of comment 5.
Attachment #8894421 -
Flags: review?(bkelly)
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8894422 [details] [diff] [review] P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest This part is for phase 3 in comment 5.
Attachment #8894422 -
Flags: review?(bkelly)
Assignee | ||
Comment 18•7 years ago
|
||
Comment on attachment 8894426 [details] [diff] [review] P4: Enable a related web-platform-test. r=bkelly This is the test case for both phase 1 and phase 2 in comment 5.
Attachment #8894426 -
Flags: review?(bkelly)
Reporter | ||
Comment 19•7 years ago
|
||
Comment on attachment 8894420 [details] [diff] [review] P1: Move from nsLoadFlags to UpdateViaCache Review of attachment 8894420 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. ::: dom/webidl/ServiceWorkerContainer.webidl @@ +42,4 @@ > > dictionary RegistrationOptions { > USVString scope; > + ServiceWorkerUpdateViaCache updateViaCache; Please use the default `= "imports"` like the spec does. Boris tells me this should be supported. ::: dom/workers/ServiceWorkerContainer.cpp @@ +200,5 @@ > } > > + ServiceWorkerUpdateViaCache updateViaCache = > + aOptions.mUpdateViaCache.WasPassed() ? aOptions.mUpdateViaCache.Value() > + : ServiceWorkerUpdateViaCache::Imports; If we use the default value in the webidl, this should not be necessary. ::: dom/workers/ServiceWorkerInfo.cpp @@ +235,5 @@ > + > + // Scripts of a service worker should always be loaded bypass service workers. > + // Otherwise, we might not be able to update a service worker correctly, if > + // there is a service worker generating the script. > + MOZ_ASSERT(mLoadFlags & nsIChannel::LOAD_BYPASS_SERVICE_WORKER); MOZ_DIAGNOSTIC_ASSERT() for something cheap like this. ::: dom/workers/ServiceWorkerInfo.h @@ +34,4 @@ > ServiceWorkerState mState; > OriginAttributes mOriginAttributes; > > + // This LoadFlags is only applied to imorted scripts, since the main script s/imorted/imported/ @@ +34,5 @@ > ServiceWorkerState mState; > OriginAttributes mOriginAttributes; > > + // This LoadFlags is only applied to imorted scripts, since the main script > + // has alreday been downloaded when practicing bytecheck. This LoadFlag is s/alreday/already/ s/practicing bytecheck/performing the bytecheck/ @@ +41,5 @@ > + // 2. (Optional) nsIRequest::VALIDATE_ALWAYS > + // depends on ServiceWorkerUpdateViaCache of its registration. > + // 3. (optional) nsIRequest::LOAD_BYPASS_CACHE > + // depends on whether the update timer is expired. > + const nsLoadFlags mLoadFlags; Maybe rename this to "mImportsLoadFlags" or something in a follow-on patch? ::: dom/workers/ServiceWorkerManager.cpp @@ +210,5 @@ > } > > + // This is a workaround before we update SERVICEWORKERREGISTRAR_VERSION and > + // the related implementation. > + ServiceWorkerUpdateViaCache UVC = aRegistration->GetUpdateViaCache(); nit: Lower case chars for stack variables. @@ +2055,5 @@ > > + // This is a workaround before we update SERVICEWORKERREGISTRAR_VERSION and > + // the related implementation. > + nsLoadFlags flags = aRegistration.loadFlags(); > + ServiceWorkerUpdateViaCache UVC = nit: uvc ::: dom/workers/ServiceWorkerUpdateJob.cpp @@ +430,5 @@ > > // Begin step 7 of the Update algorithm to evaluate the new script. > + nsLoadFlags flags = aLoadFlags; > + if (GetUpdateViaCache() == ServiceWorkerUpdateViaCache::None) { > + aLoadFlags |= nsIRequest::VALIDATE_ALWAYS; This needs to |= flags, not aLoadFlags, right? It seems this value is being lost. Also, can we rename this to importFlags or something else to help explain why we are only skipping cache for "none", but not "imports"?
Attachment #8894420 -
Flags: review?(bkelly) → review+
Reporter | ||
Updated•7 years ago
|
Attachment #8894421 -
Flags: review?(bkelly) → review+
Reporter | ||
Comment 20•7 years ago
|
||
I'll try to pick this up again later tonight after the kids are in bed.
Reporter | ||
Comment 21•7 years ago
|
||
Comment on attachment 8894422 [details] [diff] [review] P3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest Review of attachment 8894422 [details] [diff] [review]: ----------------------------------------------------------------- r=me with comments addressed. I only skimmed the test. ::: dom/workers/ServiceWorkerManager.cpp @@ +2075,5 @@ > > + nsLoadFlags flags = > + aRegistration.updateViaCache() == static_cast<uint16_t>(ServiceWorkerUpdateViaCache::All) > + ? nsIRequest::LOAD_NORMAL > + : nsIRequest::VALIDATE_ALWAYS; Maybe clarify this is for the top level script only? ::: dom/workers/ServiceWorkerRegistrar.cpp @@ +378,5 @@ > + entry->updateViaCache() = updateViaCache.ToInteger(&rv, 16); > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return rv; > + } else if (entry->updateViaCache() < nsIServiceWorkerRegistrationInfo::UPDATE_VIA_CACHE_IMPORTS && > + entry->updateViaCache() > nsIServiceWorkerRegistrationInfo::UPDATE_VIA_CACHE_NONE) { I think this will always evaluate to false. Since its an unsigned int you probably just want the check against the max value. ::: dom/workers/ServiceWorkerRegistrarTypes.ipdlh @@ +19,4 @@ > > PrincipalInfo principal; > > + uint16_t updateViaCache; Our packing sucks here, but moving the fields around would require fixing any constructors as well. Maybe file a follow-up bug? Ideally the 2-byte int and the 1-byte bool would be at the end of the list.
Attachment #8894422 -
Flags: review?(bkelly) → review+
Reporter | ||
Comment 22•7 years ago
|
||
Comment on attachment 8894426 [details] [diff] [review] P4: Enable a related web-platform-test. r=bkelly Review of attachment 8894426 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!
Attachment #8894426 -
Flags: review?(bkelly) → review+
Assignee | ||
Comment 23•7 years ago
|
||
Comment addressed.
Assignee | ||
Comment 24•7 years ago
|
||
Fixed an unused variable in an assertion.
Assignee | ||
Comment 25•7 years ago
|
||
Comment addressed.
Assignee | ||
Updated•7 years ago
|
Attachment #8894420 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8894421 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8894422 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8894426 -
Attachment description: P4: Enable a related web-platform-test → P4: Enable a related web-platform-test. r=bkelly
Assignee | ||
Comment 26•7 years ago
|
||
Comment addressed
Assignee | ||
Updated•7 years ago
|
Attachment #8897739 -
Attachment is obsolete: true
Assignee | ||
Comment 27•7 years ago
|
||
Fix a typo!
Assignee | ||
Updated•7 years ago
|
Attachment #8897764 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8897736 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8897738 -
Flags: review+
Assignee | ||
Updated•7 years ago
|
Attachment #8898165 -
Flags: review+
Assignee | ||
Comment 28•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=699af3dced7b61be257cae076579fea65e338d65
Keywords: checkin-needed
Comment 29•7 years ago
|
||
Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/9865180c7e59 Part 1: Move from nsLoadFlags to UpdateViaCache. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/128194809ab3 Part 2: Expose ServieworkerRegistration.updateViaCache. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/cbba741c3061 Part 3: Update SERVICEWORKERREGISTRAR_VERSION and a related gtest. r=bkelly https://hg.mozilla.org/integration/mozilla-inbound/rev/8cda11dd3e3f Part 4: Enable a related web-platform-test. r=bkelly
Keywords: checkin-needed
Comment 30•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/9865180c7e59 https://hg.mozilla.org/mozilla-central/rev/128194809ab3 https://hg.mozilla.org/mozilla-central/rev/cbba741c3061 https://hg.mozilla.org/mozilla-central/rev/8cda11dd3e3f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 31•7 years ago
|
||
We currently have 9 different intermittents on the WPT registration-updateviacache.https.html[1] updated in part 4, :HoPang, can you take a look at them? (I hit them a few different times on an unrelated try run; I'm not volunteering for ServiceWorker intermittent triage duty ;) 1: https://bugzilla.mozilla.org/buglist.cgi?quicksearch=updateviacache
Flags: needinfo?(bhsu)
Reporter | ||
Comment 32•7 years ago
|
||
I believe we landed a fix to the test in the last 24 hours or so.
Comment 33•7 years ago
|
||
(In reply to Ben Kelly [:bkelly] from comment #32) > I believe we landed a fix to the test in the last 24 hours or so. To mozilla repos, or the github repo? This is the tip of mozilla-inbound and mozilla-central for the file which still shows only 1 revision: https://hg.mozilla.org/integration/mozilla-inbound/log/tip/testing/web-platform/tests/service-workers/service-worker/registration-updateviacache.https.html https://hg.mozilla.org/mozilla-central/log/tip/testing/web-platform/tests/service-workers/service-worker/registration-updateviacache.https.html
Reporter | ||
Comment 34•7 years ago
|
||
It was in a resources file. See bug 1391532.
Comment 35•7 years ago
|
||
Awesome! Sorry about the noise! (Also, clearly I need to improve how I do my bugzilla quicksearches, because if mine had included FIXED with a recent transition, I would have seen that more obviously. Also, I probably should also just abandon thunderbird in favor of gmail; if I weren't afraid of the 15-second lockups on its quicksearch/quickfilter, I would have found the recent activity as well.)
Flags: needinfo?(bhsu)
You need to log in
before you can comment on or make changes to this bug.
Description
•