1156847 - We always go to the network even if we respond with a custom response on a fetch event handler

Status: RESOLVED FIXED

(Core :: DOM: Workers, defect)

Description

[Fernando Jiménez Moreno [:ferjm]]

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

Comment 1

[Josh Matthews [:jdm]]

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]].

Comment 2

[Andrea Marchesini [:baku]]

Attachment: Testcase

Comment 3

[(no longer active)]

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

Comment 4

[(no longer active)]

Attachment: Part 1: Expose the dom.serviceWorkers.testing.enabled pref to workers

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

Comment 5

[(no longer active)]

Attachment: Part 2: Add the Response::GetSecurityInfo() helper method

Comment 6

[(no longer active)]

Attachment: Part 3: Store the security info for a service worker on its WorkerPrivate

Comment 7

[(no longer active)]

Attachment: Part 4: When loading a service worker from the network, remember the security info of the channel on the WorkerPrivate

Comment 8

[(no longer active)]

Attachment: Part 5: When storing a service worker script to the cache, store its security info in the cache as well

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.

Comment 9

[(no longer active)]

Attachment: Part 6: When loading a service worker from the cache, set its security info on the WorkerPrivate

Comment 10

[(no longer active)]

Attachment: Part 7: Assert that service workers have their security info set by the time that SetPrincipal is called

Comment 11

[(no longer active)]

Attachment: 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

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.

Comment 12

[(no longer active)]

Attachment: Part 9: Add unit tests for responding to a FetchEvent with a synthesized Response both in the case where the service worker is loaded from the network and from the cache

Comment 13

[(no longer active)]

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 14

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 15

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 16

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 17

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 18

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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 19

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

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.

Comment 20

[Nikhil Marathe [:nsm] (No longer reading bugmail, please needinfo?)]

(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?

Comment 21

[(no longer active)]

(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.  ;-)

Comment 22

[(no longer active)]

(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 23

[Ben Turner (not reading bugmail, use the needinfo flag!)]

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 24

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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 25

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

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

Comment 26

[(no longer active)]

(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...

Comment 27

[(no longer active)]

Attachment: Part 1: Add the Response::GetSecurityInfo() helper method

Comment 28

[(no longer active)]

Attachment: Part 2: Store the security info for a service worker on its WorkerPrivate

Comment 29

[(no longer active)]

Attachment: Part 3: When loading a service worker from the network, remember the security info of the channel on the WorkerPrivate

Comment 30

[(no longer active)]

Attachment: Part 4: When storing a service worker script to the cache, store its security info in the cache as well

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.

Comment 31

[(no longer active)]

Attachment: Part 5: When loading a service worker from the cache, set its security info on the WorkerPrivate

Comment 32

[(no longer active)]

Attachment: Part 6: 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

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.

Comment 33

[(no longer active)]

Attachment: Part 7: Add unit tests for responding to a FetchEvent with a synthesized Response both in the case where the service worker is loaded from the network and from the cache

Comment 34

[GitHub Autolander Bot]

Attachment: [gaia] arcturus:bug-1158093 > mozilla-b2g:master

Comment 35

[(no longer active)]

(In reply to Autolander from comment #34)
> Created attachment 8599474 [details] [review]
> [gaia] arcturus:bug-1158093 > mozilla-b2g:master

What's this?  Wrong bug #?

Comment 36

[Pulsebot]

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

Comment 37

[Francisco Jordano [:arcturus] [:francisco]]

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

Totally, sorry my fault.

Comment 38

[GitHub Autolander Bot]

Attachment: [gaia] arcturus:bug-1158093 > mozilla-b2g:master

Comment 39

[Ryan VanderMeulen [:RyanVM]]

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 40

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/883ca6986e84

Comment 41

[Daniel Holbert [:dholbert]]

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.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/883ca6986e84