1912607 - SW script installation fails on scripts served with "Vary: *", potential spec violation. Manifests as: Assertion failure: !result.IsErrorWithMessage(), at /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerScriptCache.cpp:58

Status: NEW

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

Description

[Tyson Smith [:tsmith]]

Found with m-c 20240807-70728a2fa4a2 (--enable-debug --enable-fuzzing)

This was found by visiting a live website with a debug build.

STR:

• Launch browser and visit site

This issue was triggered by visiting http://www.tenethealth.com/.

A Pernosco session is available here: https://pernos.co/debug/aXUUTdodkObRiN19XzsjRg/index.html

Assertion failure: !result.IsErrorWithMessage(), at /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerScriptCache.cpp:58

#0 0x778398adfe96 in WriteToCache /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerScriptCache.cpp:580:7
#1 0x778398adfe96 in ManageNewCache /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerScriptCache.cpp:497:12
#2 0x778398adfe96 in mozilla::dom::serviceWorkerScriptCache::(anonymous namespace)::CompareManager::ResolvedCallback(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/serviceworkers/ServiceWorkerScriptCache.cpp:1380:7
#3 0x778398790eb1 in mozilla::dom::(anonymous namespace)::PromiseNativeHandlerShim::ResolvedCallback(JSContext*, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/checkouts/gecko/dom/promise/Promise.cpp:416:12
#4 0x7783987914d6 in mozilla::dom::NativeHandlerCallback(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/obj-build/dist/include/js/CallArgs.h
#5 0x778399d08514 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:489:13
#6 0x778399d07cff in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:583:12
#7 0x778399d092ff in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:682:8
#8 0x778399d4d201 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.h:121:10
#9 0x778399fa0771 in PromiseReactionJob(JSContext*, unsigned int, JS::Value*) /builds/worker/checkouts/gecko/js/src/builtin/Promise.cpp:2242:10
#10 0x778399d08514 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:489:13
#11 0x778399d07cff in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:583:12
#12 0x778399d092ff in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason) /builds/worker/checkouts/gecko/js/src/vm/Interpreter.cpp:682:8
#13 0x778399e0d2d7 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/checkouts/gecko/js/src/vm/CallAndConstruct.cpp:119:10
#14 0x778395b91f10 in mozilla::dom::VoidFunction::Call(mozilla::dom::BindingCallContext&, JS::Handle<JS::Value>, mozilla::ErrorResult&) /builds/worker/workspace/obj-build/dom/bindings/./JSActorBinding.cpp:35:8
#15 0x7783937d79f5 in mozilla::dom::PromiseJobCallback::Call(mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*) /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:198:12
#16 0x7783937d7110 in Call /builds/worker/workspace/obj-build/dist/include/mozilla/dom/PromiseBinding.h:211:12
#17 0x7783937d7110 in mozilla::PromiseJobRunnable::Run(mozilla::AutoSlowOperation&) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:209:18
#18 0x7783937c3b08 in mozilla::CycleCollectedJSContext::PerformMicroTaskCheckPoint(bool) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:711:17
#19 0x7783937c4b29 in mozilla::CycleCollectedJSContext::AfterProcessTask(unsigned int) /builds/worker/checkouts/gecko/xpcom/base/CycleCollectedJSContext.cpp:498:3
#20 0x7783945bb5b6 in XPCJSContext::AfterProcessTask(unsigned int) /builds/worker/checkouts/gecko/js/xpconnect/src/XPCJSContext.cpp:1451:28
#21 0x7783938e4d4d in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/checkouts/gecko/xpcom/threads/nsThread.cpp:1242:24
#22 0x7783938eb5af in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/checkouts/gecko/xpcom/threads/nsThreadUtils.cpp:480:10
#23 0x77839444b765 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/checkouts/gecko/ipc/glue/MessagePump.cpp:85:21
#24 0x7783943a3831 in RunHandler /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:363:3
#25 0x7783943a3831 in MessageLoop::Run() /builds/worker/checkouts/gecko/ipc/chromium/src/base/message_loop.cc:345:3
#26 0x778398c00ef8 in nsBaseAppShell::Run() /builds/worker/checkouts/gecko/widget/nsBaseAppShell.cpp:148:27
#27 0x778398cb9eb4 in nsAppShell::Run() /builds/worker/checkouts/gecko/widget/gtk/nsAppShell.cpp:469:33
#28 0x778399a40d04 in nsAppStartup::Run() /builds/worker/checkouts/gecko/toolkit/components/startup/nsAppStartup.cpp:296:30
#29 0x778399b5417c in XREMain::XRE_mainRun() /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:5805:22
#30 0x778399b55978 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:6030:8
#31 0x778399b56796 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/checkouts/gecko/toolkit/xre/nsAppRunner.cpp:6105:21
#32 0x5e95ad6a9095 in do_main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:230:22
#33 0x5e95ad6a9095 in main /builds/worker/checkouts/gecko/browser/app/nsBrowserApp.cpp:448:16
#34 0x7783a6c84d8f in __libc_start_call_main csu/../sysdeps/nptl/libc_start_call_main.h:58:16
#35 0x7783a6c84e3f in __libc_start_main csu/../csu/libc-start.c:392:3
#36 0x5e95ad67ebb8 in _start (/home/worker/build/firefox-bin+0x58bb8) (BuildId: 0e7124ae991724e052c197dbbdf6ee22e640d889)

Comment 1

[Jens Stutte [:jstutte]]

It seems TypeUtils::ToCacheResponseWithoutBody fails with "Invalid Response object with a 'Vary: *' header."

We handle errors in terms of aborting, such that I wonder if we really should assert anything there? Is this assert just meant to support our tests but not meant to trigger for general usage of this code path in DEBUG and should better be removed ?

I believe this has no impact on users in release, can you confirm?

Comment 2

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

(In reply to Jens Stutte [:jstutte] from comment #1)

I believe this has no impact on users in release, can you confirm?

The assertions aren't crashing anyone because they're just MOZ_ASSERT, but the assertions indicate we are deviating from spec behavior here (more below). I believe, per spec, we are failing to install a ServiceWorker that a spec-compliant browser would install. That said, a search on vary under the SW wpt dir (excluding cache API) finds no use of vary so it doesn't look like there's WPT coverage for this.

If I go to https://www.tenethealth.com/ in Chrome Beta and Firefox Nightly, we both end up installing the ServiceWorker successfully. And using Chrome's devtools I see no indications that the site is serving any Vary headers other than "Accept-Encoding" and "Accept, Accept-Encoding". (Due to outstanding bugs it's hard to get the SW script loader fetches unless using the Firefox Profiler, but those don't include the headers, and this seems to be the case even if turning on the fancier SW debugging.)

Looking at the pernosco trace (thank you, :tsmith!), here is where we are throwing the error and we can see that the URL is https://www.tenethealth.com/sw.js?v=www.tenethealth.com which is the top-level script and the vary header payload was *, Accept-Encoding which is definitely an interesting choice. It seems conceivable this was a transient misconfiguration on their part and maybe they noticed because it made other browsers angry?

Assertion Rationale

I understand these assertions to primarily be about ensuring that we don't lose any fidelity in our error propagation. We assert whenever there would be a loss of fidelity, and comments attempt to explain why they should hold. Some specific history:

• MOZ_ASSERT(!result.IsErrorWithMessage()) was initially introduced at the request of :wanderview in https://bugzilla.mozilla.org/show_bug.cgi?id=1154494#c3 saying "This isn't going to work quite right if you have a TypeError or something. So assert that we don't when getting ErrorCode(). Here and other places you use ErrorCode()."
• MOZ_ASSERT(!result.IsJSException()); - the tl;dr here is a bunch of assertions were added in cases where we ReadableStreams cannot be involved. Specifically, ServiceWorker script loading will always go directly to the network because they are not subject to ServiceWorker interception. (Whereas fetches made from windows or controlled workers can be intercepted and therefore backed by a readable stream.)
  • This seems to have been introduced at the request of :bzbarsky in https://bugzilla.mozilla.org/show_bug.cgi?id=1128959#c141 saying: "OK. In that case this needs some comments explaining what's going on, and possibly assertions about the state of the ErrorResult or JSContext. So if we go the NoteJSContextException route, we could assert that there is no exception on the JSContext here, for example, since with no streams that should always be true, right? If we keep our current stealing of the exception onto ErrorResult, assert that the thing on ErrorResult is not a JS exception..."
  • :baku reported introducing the exception in https://bugzilla.mozilla.org/show_bug.cgi?id=1128959#c152
  • :bzbarsky indicated concerns about it in https://bugzilla.mozilla.org/show_bug.cgi?id=1128959#c160
  • :baku then responded in https://bugzilla.mozilla.org/show_bug.cgi?id=1128959#c163 that he wrote comments explaining what's going on with the error reporting, with https://bugzilla.mozilla.org/attachment.cgi?id=8889973&action=diff being that interdiff attached in https://bugzilla.mozilla.org/show_bug.cgi?id=1128959#c165.
  • In terms of related error-handling changes, note that bug 1357463 which gave us mozilla::CopyableErrorResult was proposed in 2017-04 but landed in 2018-04 and the bug 1128959 work happened substantially in 2017-07.

Spec behavior: Throwing on Vary *

The underlying throw is part of the spec for Cache.put in step 7.2.1 and similarly in Cache.addAll step 5.7.2.2 (which has to be distinct because of its Promise.all()-style all-or-nothing semantics.)

There's some interesting context particularly at https://github.com/w3c/ServiceWorker/issues/656#issuecomment-87874393 where :mnot said:

Vary: * means "varies on things that I can't describe" -- i.e., it's not cacheable.

That seems to have led to the behavior and our bug 1149987 implementing forbidding that.

Spec behavior: ServiceWorkers do not store their scripts using the Cache API

An interesting complication is that that ServiceWorkers are not defined to use the Cache API for storage. Instead a script resource map is defined. We chose to use Cache API for storage.

Next Steps

I think it could make sense to:

• Create a WPT under https://searchfox.org/mozilla-central/source/testing/web-platform/tests/service-workers/service-worker that uses the .headers mechanism to serve the top-level script with "Vary: *" and see what we and other browsers do.
• In the event any browsers besides Firefox fail, we should probably file a spec issue to discuss the situation.
• In the event only Firefox fails, it could make sense to land the test as part of a change to make us not throw in the case of "Vary: *" for when we are caching a script, although I think it might be appropriate for us to explicitly strip the vary header because our Cache API implementation really does not want it in there, so it seems like it would be reasonable for us to pretend it does not exist.