1229369 - service workers should always re-intercept on navigation redirects

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

In bug 1200677 we had some discussion about whether a navigation that is not intercepted should ever intercept again on redirect.  The spec is a bit vague and suggests not.

As a result I wrote this chrome issue and spoke with Anne on IRC:

  https://code.google.com/p/chromium/issues/detail?id=559447

Anne indicated that the intention was that navigation redirects should always check for interception again.  He agreed the spec was vague and suggested a fix:

  https://github.com/slightlyoff/ServiceWorker/issues/793

So we would only set the skip-service-worker flag for requests that are configured to follow redirects.  Since navigations use the manual redirect mode, they would not get the skip-service-worker flag.

My impression is that this might be a bit tricky to implement correctly and probably hits the crasher on e10s in bug 1219469.

Comment 1

[Josh Matthews [:jdm]]

Ah, this intersects with bug 1233245.

Comment 2

[(no longer active)]

Attachment: Intercept redirected network fetches that have their request mode set to manual

In the non-e10s case, this is done by simply avoiding to set the
LOAD_BYPASS_SERVICE_WORKER flag when we detect a non-internal redirect
in the manual redirect mode.

The e10s solution is a bit more complicated.  Only the child process
knows whether a URI needs to be intercepted, so we piggy back on the
code written to support the |event.respondWith(Response.redirect())|
case where we know in the child that the target of the redirect needs to
be intercepted.

This means that we need to check the same condition as in the non-e10s
case, but we also need to check whether the target of the redirection
needs to be intercepted, which means we need to properly take secure
upgrades into account as well.  This is done by computing both whether
we should intercept and whether we should do a secure upgrade in
HttpChannelChild::SetupRedirect() and saving the information on the
HttpChannelChild for later usage in HttpChannelChild::AsyncOpen().

Comment 3

[Josh Matthews [:jdm]]

Comment on attachment 8704297 [details] [diff] [review]
Intercept redirected network fetches that have their request mode set to manual

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

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +2653,5 @@
> +  rv = NS_ShouldSecureUpgrade(aURI,
> +                              mLoadInfo,
> +                              resultPrincipal,
> +                              mPrivateBrowsing,
> +                              mAllowSTS,

Any chance these members have already been propagated to aChannel in the case where this is called from SetupRedirect? If so, could this method avoid the extra argument?

Comment 4

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #3)
> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +2653,5 @@
> > +  rv = NS_ShouldSecureUpgrade(aURI,
> > +                              mLoadInfo,
> > +                              resultPrincipal,
> > +                              mPrivateBrowsing,
> > +                              mAllowSTS,
> 
> Any chance these members have already been propagated to aChannel in the
> case where this is called from SetupRedirect? If so, could this method avoid
> the extra argument?

If you mean the extra aChannel argument, I thought about calling ShouldInterceptURI() on httpChannelChild, but httpChannelChild is an nsIHttpChannelChild*, and it seemed to me that doing the static_cast to HttpChannelChild is more gross than passing in the extra argument.  The data bits are the same since they get copied in SetupReplacementChannel so the end result is the same either way, but if you prefer the cast, let me know.

Comment 5

[Josh Matthews [:jdm]]

I found it very subtle that most of the data being used were members of the this pointer, but the result principal was coming from a different channel argument. I think the explicit cast is a better tradeoff than the current patch.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c31168f94ebf

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c31168f94ebf

Comment 8

[(no longer active)]

(In reply to Josh Matthews [:jdm] from comment #5)
> I found it very subtle that most of the data being used were members of the
> this pointer, but the result principal was coming from a different channel
> argument. I think the explicit cast is a better tradeoff than the current
> patch.

Oops, sorry I somehow missed this comment.  Pushing a patch to correct this right now.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b193412ac803

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b193412ac803