service workers should always re-intercept on navigation redirects

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox45 affected, firefox46 fixed)

Details

Attachments

(1 attachment)

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

Updated

3 years ago
Assignee: nobody → ehsan
(Assignee)

Comment 2

3 years ago
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().
Attachment #8704297 - Flags: review?(josh)
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?
Attachment #8704297 - Flags: review?(josh) → review+
(Assignee)

Comment 4

3 years ago
(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.
Flags: needinfo?(josh)
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.
Flags: needinfo?(josh)

Comment 7

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c31168f94ebf
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
(Assignee)

Comment 8

3 years ago
(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.
You need to log in before you can comment on or make changes to this bug.