While working on a test case for bug 1152899 I realized that even if we response with a custom response while handling a fetch event, we always hit the network. You can easily test this by modifying this line [1] to "event.respondWith('whatever');". The test should fail cause it is expecting a message with "status" "done" at [2], but it isn't failing. That means that we are serving the content of [3] instead of the synthesized response. [1] https://mxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/fetch/https/https_test.js#9 [2] https://mxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/test_https_fetch.html?force=1#33 [3] https://mxr.mozilla.org/mozilla-central/source/dom/workers/test/serviceworkers/fetch/https/index.html?force=1

It looks like we need to tighten up the implementation of FetchEvent::RespondWith and ServiceWorkerManager::DispatchFetchEvent to correspond to steps 18.9, 20, and 21 in [[HandleFetch]].

Note to self: bug 1154494 is adding one more broken code path to be fixed here.

This is used in the assertions added in the later parts of this series.

Before this patch, we would call Cache.put() before opening the channel, which means that we have no way of knowing the security info for the channel in order to set it on the Response object that we synthesize. This patch adds an nsIRequestObserver to the tee created for piping the body of the response to the cache, and delays calling Cache.put() until we receive the nsIRequestObserver::OnStartRequest() notification, at which point we set the obtained security info on the Response object to be stored in the cache.

This case will only happen if we're responding with a synthesized Response object. Response objects obtained through fetch() already have their correct security info.

Nikhil, bug 1003991 added yet another way of enabling the service workers testing pref, based on a boolean stored on the outer window. That means that all of the assertions that I have added in these patches unfortunately need to change in order to take that into account as well. Should I do that, or give up and remove the assertions?

Comment on attachment 8596893 [details] [diff] [review] Part 1: Expose the dom.serviceWorkers.testing.enabled pref to workers Review of attachment 8596893 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/RuntimeService.cpp @@ +2677,5 @@ > } > #endif > > if (key == WORKERPREF_DOM_CACHES) { > key = WORKERPREF_DOM_CACHES; Nit: please remove this too while you are here. @@ +2681,5 @@ > key = WORKERPREF_DOM_CACHES; > sDefaultPreferences[WORKERPREF_DOM_CACHES] = > Preferences::GetBool(PREF_DOM_CACHES_ENABLED, false); > + } else if (key == WORKERPREF_DOM_SERVICEWORKERS_TESTING_ENABLED) { > + key = WORKERPREF_DOM_SERVICEWORKERS_TESTING_ENABLED; this isn't really required since key is already set to it.

Comment on attachment 8596895 [details] [diff] [review] Part 3: Store the security info for a service worker on its WorkerPrivate Review of attachment 8596895 [details] [diff] [review]: ----------------------------------------------------------------- r=me. Needs worker peer signoff.

Comment on attachment 8596896 [details] [diff] [review] Part 4: When loading a service worker from the network, remember the security info of the channel on the WorkerPrivate Review of attachment 8596896 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ScriptLoader.cpp @@ +926,5 @@ > + nsCOMPtr<nsISerializable> serializable = do_QueryInterface(infoObj); > + if (serializable) { > + mWorkerPrivate->SetSecurityInfo(serializable); > + } else { > + NS_WARNING("A non-serializable object was obtained from nsIChannel::GetSecurityInfo()!"); Would it make sense to fail the load or are non-serialize-able security infos common?

Comment on attachment 8596898 [details] [diff] [review] Part 6: When loading a service worker from the cache, set its security info on the WorkerPrivate Review of attachment 8596898 [details] [diff] [review]: ----------------------------------------------------------------- I'd like to take a look again. ::: dom/workers/ScriptLoader.cpp @@ +415,5 @@ > AssertIsOnMainThread(); > } > > ScriptLoadInfo& mLoadInfo; > + WorkerPrivate* mWorkerPrivate; I'd avoid making CacheScriptLoader aware of the worker private. ScriptLoaderRunnable properly handles the possibility of the worker going away by adding a feature and such. It is plausible that ResolvedCallback may be triggered even after the worker has gone away, since ScriptLoaderRunnable does not notify the cache code about this. I'm not saying it will happen, particularly, our code paths right now have checks, but there are already several code paths, so let's keep WorkerPrivate out of this :) Instead... (see comment below). @@ +1053,3 @@ > void > DataReceivedFromCache(uint32_t aIndex, const uint8_t* aString, > uint32_t aStringLen) ... modify this to accept the security info as another param. @@ +1057,5 @@ > AssertIsOnMainThread(); > MOZ_ASSERT(aIndex < mLoadInfos.Length()); > ScriptLoadInfo& loadInfo = mLoadInfos[aIndex]; > MOZ_ASSERT(loadInfo.mCacheStatus == ScriptLoadInfo::Cached); > + MOZ_ASSERT_IF(!Preferences::GetBool("dom.serviceWorkers.testing.enabled", false), Please add a comment about why this is gated on the pref when it was unconditional in the network case. @@ +1058,5 @@ > MOZ_ASSERT(aIndex < mLoadInfos.Length()); > ScriptLoadInfo& loadInfo = mLoadInfos[aIndex]; > MOZ_ASSERT(loadInfo.mCacheStatus == ScriptLoadInfo::Cached); > + MOZ_ASSERT_IF(!Preferences::GetBool("dom.serviceWorkers.testing.enabled", false), > + !mWorkerPrivate->GetSecurityInfo().IsEmpty()); A few lines below this there is a call to SetPrincipal() inside a IsMainWorkerScript() block. Set the security info there. @@ +1437,5 @@ > response->GetBody(getter_AddRefs(inputStream)); > > + MOZ_ASSERT_IF(!Preferences::GetBool("dom.serviceWorkers.testing.enabled", false), > + !response->GetSecurityInfo().IsEmpty()); > + mWorkerPrivate->SetSecurityInfo(response->GetSecurityInfo()); set this in a string/nsISerializable member and pass it to DataReceivedFromCache below.

Comment on attachment 8596899 [details] [diff] [review] Part 7: Assert that service workers have their security info set by the time that SetPrincipal is called Review of attachment 8596899 [details] [diff] [review]: ----------------------------------------------------------------- r=me pending the pref comment on the previous patch.

Comment on attachment 8596900 [details] [diff] [review] Part 8: When calling FetchEvent.respondWith(), fall back to the security info of the service worker if the Response object that we are responding with doesn't have its own security info Review of attachment 8596900 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the one change. ::: dom/workers/ServiceWorkerEvents.cpp @@ +180,5 @@ > { > + WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + worker->AssertIsOnWorkerThread(); > + MOZ_ASSERT(worker->IsServiceWorker()); Nit: This is a little overkill considering FetchEvent is only exposed on and dispatched to ServiceWorkerGlobalScope :) @@ +181,5 @@ > + WorkerPrivate* worker = GetCurrentThreadWorkerPrivate(); > + MOZ_ASSERT(worker); > + worker->AssertIsOnWorkerThread(); > + MOZ_ASSERT(worker->IsServiceWorker()); > + mWorkerSecurityInfo = worker->GetSecurityInfo(); I'd prefer getting the security info from the worker in RespondWithHandler::ResolvedCallback and passing it to the ctor.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #13) > Nikhil, bug 1003991 added yet another way of enabling the service workers > testing pref, based on a boolean stored on the outer window. That means > that all of the assertions that I have added in these patches unfortunately > need to change in order to take that into account as well. Should I do > that, or give up and remove the assertions? Could you explain why the assertion exists?

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #20) > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from > comment #13) > > Nikhil, bug 1003991 added yet another way of enabling the service workers > > testing pref, based on a boolean stored on the outer window. That means > > that all of the assertions that I have added in these patches unfortunately > > need to change in order to take that into account as well. Should I do > > that, or give up and remove the assertions? > > Could you explain why the assertion exists? There is no great reason now, I used it more when developing this. I'd say we should leave them in if it's not too painful, but it is crossing that boundary right now. ;-)

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #16) > Comment on attachment 8596896 [details] [diff] [review] > Part 4: When loading a service worker from the network, remember the > security info of the channel on the WorkerPrivate > > Review of attachment 8596896 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/workers/ScriptLoader.cpp > @@ +926,5 @@ > > + nsCOMPtr<nsISerializable> serializable = do_QueryInterface(infoObj); > > + if (serializable) { > > + mWorkerPrivate->SetSecurityInfo(serializable); > > + } else { > > + NS_WARNING("A non-serializable object was obtained from nsIChannel::GetSecurityInfo()!"); > > Would it make sense to fail the load or are non-serialize-able security > infos common? That's a hard question to answer. :-) From what I can tell reading the code involved, all security info objects must be serializable, and there's already Necko stuff that depends on this. However I don't trust this enough to want to fail the load if this happens, which is why I stuck with only adding a warning.

Comment on attachment 8596895 [details] [diff] [review] Part 3: Store the security info for a service worker on its WorkerPrivate Review of attachment 8596895 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/WorkerPrivate.cpp @@ +4086,5 @@ > +WorkerPrivateParent<Derived>::SetSecurityInfo(nsISerializable* aSerializable) > +{ > + MOZ_ASSERT(IsServiceWorker()); > + AssertIsOnMainThread(); > + NS_SerializeToString(aSerializable, mLoadInfo.mSecurityInfo); Wouldn't it be better to do: nsCString securityInfo; NS_SerializeToString(aSerializable, securityInfo); SetSecurityInfo(securityInfo); That way you have a common code path. ::: dom/workers/WorkerPrivate.h @@ +503,5 @@ > + SetSecurityInfo(const nsCString& aSecurityInfo) > + { > + MOZ_ASSERT(IsServiceWorker()); > + AssertIsOnMainThread(); > + MOZ_ASSERT(mLoadInfo.mSecurityInfo.IsEmpty()); Also |MOZ_ASSERT(!aSecurityInfo.IsEmpty())| here?

Comment on attachment 8596895 [details] [diff] [review] Part 3: Store the security info for a service worker on its WorkerPrivate Review of attachment 8596895 [details] [diff] [review]: ----------------------------------------------------------------- r=me with bent's comments addressed

Comment on attachment 8596897 [details] [diff] [review] Part 5: When storing a service worker script to the cache, store its security info in the cache as well Review of attachment 8596897 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/workers/ScriptLoader.cpp @@ +568,5 @@ > + { > + AssertIsOnMainThread(); > + > + nsCOMPtr<nsISupportsPRUint32> indexSupports(do_QueryInterface(aContext)); > + NS_ASSERTION(indexSupports, "This should never fail!"); MOZ_ASSERT @@ +573,5 @@ > + > + uint32_t index = UINT32_MAX; > + if (NS_FAILED(indexSupports->GetData(&index)) || > + index >= mLoadInfos.Length()) { > + NS_ERROR("Bad index!"); MOZ_CRASH

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #23) > ::: dom/workers/WorkerPrivate.h > @@ +503,5 @@ > > + SetSecurityInfo(const nsCString& aSecurityInfo) > > + { > > + MOZ_ASSERT(IsServiceWorker()); > > + AssertIsOnMainThread(); > > + MOZ_ASSERT(mLoadInfo.mSecurityInfo.IsEmpty()); > > Also |MOZ_ASSERT(!aSecurityInfo.IsEmpty())| here? Unfortunately I'll have to remove these assertions because of comment 21...

Before this patch, we would call Cache.put() before opening the channel, which means that we have no way of knowing the security info for the channel in order to set it on the Response object that we synthesize. This patch adds an nsIRequestObserver to the tee created for piping the body of the response to the cache, and delays calling Cache.put() until we receive the nsIRequestObserver::OnStartRequest() notification, at which point we set the obtained security info on the Response object to be stored in the cache.

This case will only happen if we're responding with a synthesized Response object. Response objects obtained through fetch() already have their correct security info.

(In reply to Autolander from comment #34) > Created attachment 8599474 [details] > [gaia] arcturus:bug-1158093 > mozilla-b2g:master What's this? Wrong bug #?

https://hg.mozilla.org/integration/mozilla-inbound/rev/02d49d9e3492 https://hg.mozilla.org/integration/mozilla-inbound/rev/b85b9b590db0 https://hg.mozilla.org/integration/mozilla-inbound/rev/e30dcb5569be https://hg.mozilla.org/integration/mozilla-inbound/rev/343858d83d73 https://hg.mozilla.org/integration/mozilla-inbound/rev/970c30fdca49 https://hg.mozilla.org/integration/mozilla-inbound/rev/e3a415d282b0 https://hg.mozilla.org/integration/mozilla-inbound/rev/a59eaeaeb8b4

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #35) > (In reply to Autolander from comment #34) > > Created attachment 8599474 [details] > > [gaia] arcturus:bug-1158093 > mozilla-b2g:master > > What's this? Wrong bug #? Totally, sorry my fault.

https://hg.mozilla.org/mozilla-central/rev/02d49d9e3492 https://hg.mozilla.org/mozilla-central/rev/b85b9b590db0 https://hg.mozilla.org/mozilla-central/rev/e30dcb5569be https://hg.mozilla.org/mozilla-central/rev/343858d83d73 https://hg.mozilla.org/mozilla-central/rev/970c30fdca49 https://hg.mozilla.org/mozilla-central/rev/e3a415d282b0 https://hg.mozilla.org/mozilla-central/rev/a59eaeaeb8b4

Comment on attachment 8598197 [details] [diff] [review] Part 4: When storing a service worker script to the cache, store its security info in the cache as well >diff --git a/dom/workers/ScriptLoader.cpp b/dom/workers/ScriptLoader.cpp > class ScriptLoaderRunnable final : public WorkerFeature, [...] >+ NS_IMETHOD >+ OnStartRequest(nsIRequest* aRequest, nsISupports* aContext) >+ { [...] >+ } >+ >+ NS_IMETHOD >+ OnStopRequest(nsIRequest* aRequest, nsISupports* aContext, >+ nsresult aStatusCode) >+ { These were missing "override" annotations (& spam warnings in clang 3.6/3.7 as a result). Pushed a followup ^ to add these annotations.