Closed Bug 1156847 Opened 9 years ago Closed 9 years ago

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

Categories

(Core :: DOM: Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: ferjm, Assigned: ehsan.akhgari)

References

Details

Attachments

(7 files, 12 obsolete files)

729 bytes, patch
Details | Diff | Splinter Review
3.79 KB, patch
Details | Diff | Splinter Review
2.06 KB, patch
Details | Diff | Splinter Review
7.62 KB, patch
Details | Diff | Splinter Review
3.27 KB, patch
nsm
: review+
Details | Diff | Splinter Review
4.72 KB, patch
Details | Diff | Splinter Review
5.98 KB, patch
Details | Diff | Splinter Review
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
Blocks: 1152899
Depends on: 1133763
Summary: We always go to the network even if we response with a custom response on a fetch event handler → We always go to the network even if we respond with a custom response on a fetch event handler
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]].
Attached patch Testcase (obsolete) — Splinter Review
Assignee: nobody → ehsan
Attachment #8596006 - Attachment description: patch.patch → Testcase
Status: NEW → ASSIGNED
Note to self: bug 1154494 is adding one more broken code path to be fixed here.
Status: ASSIGNED → NEW
Depends on: 1154494
Status: NEW → ASSIGNED
Attachment #8596006 - Attachment is obsolete: true
This is used in the assertions added in the later parts of this series.
Attachment #8596893 - Flags: review?(nsm.nikhil)
Attachment #8596894 - Flags: review?(nsm.nikhil)
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.
Attachment #8596897 - Flags: review?(khuey)
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.
Attachment #8596900 - Flags: review?(nsm.nikhil)
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?
Flags: needinfo?(nsm.nikhil)
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.
Attachment #8596893 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8596895 - Flags: review?(nsm.nikhil)
Attachment #8596895 - Flags: review?(khuey)
Attachment #8596895 - Flags: review?(bent.mozilla)
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?
Attachment #8596896 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8596898 - Flags: review?(nsm.nikhil)
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.
Attachment #8596899 - Flags: review?(nsm.nikhil) → review+
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.
Attachment #8596900 - Flags: review?(nsm.nikhil) → review+
(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?
Flags: needinfo?(nsm.nikhil)
(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?
Attachment #8596895 - Flags: review?(bent.mozilla) → review+
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
Attachment #8596895 - Flags: review?(khuey) → review+
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
Attachment #8596897 - Flags: review?(khuey) → review+
(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...
Attachment #8596893 - Attachment is obsolete: true
Attachment #8596894 - Attachment is obsolete: true
Attachment #8596895 - Attachment is obsolete: true
Attachment #8596896 - Attachment is obsolete: true
Attachment #8596897 - Attachment is obsolete: true
Attachment #8596898 - Attachment is obsolete: true
Attachment #8596899 - Attachment is obsolete: true
Attachment #8596900 - Attachment is obsolete: true
Attachment #8596901 - Attachment is obsolete: true
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.
Depends on: 1159378
(In reply to Autolander from comment #34)
> Created attachment 8599474 [details] [review]
> [gaia] arcturus:bug-1158093 > mozilla-b2g:master

What's this?  Wrong bug #?
Flags: needinfo?(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.
Flags: needinfo?(francisco)
Attachment #8599474 - Attachment is obsolete: true
Attachment #8599765 - Attachment is obsolete: true
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.
See Also: → 1157619
Depends on: 1249351
You need to log in before you can comment on or make changes to this bug.