Closed Bug 1163545 Opened 8 years ago Closed 7 years ago

Bypass AppCache completely when Service Workers supported & registered

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox41 --- affected
firefox43 --- fixed

People

(Reporter: flaki, Assigned: baku)

References

Details

Attachments

(1 file, 1 obsolete file)

Discussions on enhancing backward-compatibility of the Service Workers spec resulted in allowing AppCache & Service Workers used simultaneously, but a registered service worker script in a supporting browser should override and evict the AppCache completely.

Related spec bug: https://www.w3.org/Bugs/Public/show_bug.cgi?id=27482

Discussion: https://github.com/slightlyoff/ServiceWorker/issues/697

Chrome status (partially implemented): https://code.google.com/p/chromium/issues/detail?id=410665

Found a FixMe by :nsm, but didn't see this filed so I went ahead filing this bug: https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#
Flags: needinfo?(nsm.nikhil)
(In reply to Szmozsánszky István [:flaki] from comment #0)
> Found a FixMe by :nsm, but didn't see this filed so I went ahead filing this
> bug:

Correct link is https://dxr.mozilla.org/mozilla-central/source/dom/workers/ServiceWorkerManager.cpp#1191
I don't think I'll be working on this immediately. Clearing ni?
Flags: needinfo?(nsm.nikhil)
I think we need to consider this a bit more as well.  I believe there are sites that are built on appcache today, but also want to use a service worker for push (or to get android chrome's install banner).  They can't migrate off appcache, but might have a service worker for other reasons.
I think we might need to do something here for v1.  We need to at least make sure things don't completely blow up for people using SW and appcache simultaneously.

We can move this back to v2 or v3 if people don't think its a critical problem.
Blocks: ServiceWorkers-v1
No longer blocks: ServiceWorkers-B2G
We have telemetry on app cache usage <https://telemetry.mozilla.org/new-pipeline/dist.html#!cumulative=0&end_date=2015-08-26&keys=__none__!__none__!__none__&max_channel_version=release%252F40&measure=HTTP_OFFLINE_CACHE_DOCUMENT_LOAD&min_channel_version=release%252F39&product=Firefox&sanitize=1&sort_keys=submissions&start_date=2015-08-07&table=0&trim=1&use_submission_date=0> and on the release channel, if I'm doing the math correctly, the usage is 0.043% of page loads.

Based on that, I don't really think this should block v1, but if the fix is super simple, it's definitely nice to fix.
Assignee: nobody → amarchesini
Status: NEW → ASSIGNED
Attached patch appCache.patch (obsolete) — Splinter Review
Attachment #8658225 - Flags: feedback?(nsm.nikhil)
Attached patch appCache.patchSplinter Review
Attachment #8658225 - Attachment is obsolete: true
Attachment #8658225 - Flags: feedback?(nsm.nikhil)
Attachment #8658797 - Flags: review?(josh)
Comment on attachment 8658797 [details] [diff] [review]
appCache.patch

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

A test for these changes would be useful, I think.

::: dom/base/nsContentSink.cpp
@@ +1063,5 @@
>    if (!mDocShell) {
>      return;
>    }
>  
> +  // If this document has been interecepted, let's skip the processing of the

Don't bother processing offline manifest for documents with a ServiceWorker.
Attachment #8658797 - Flags: review?(josh) → review+
I'll file a follow up for the test. I'm going to land this patch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/982d9a863989
https://hg.mozilla.org/mozilla-central/rev/982d9a863989
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.