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.

Ah, this intersects with bug 1233245.

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

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

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.

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