1412015 - intercepted channels should enforce redirection limits on synthetic redirects

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Description

[Ben Kelly [:bkelly, not reviewing]]

Currently we are failing a test in the WPT redirected-response.https.html file because we don't enforce the 20 redirect limit on synthetic redirects.  We should fix this.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

I introduced a new problem in bug 1405739, so I'm going to fix this now.  When I split the mRedirectCount into a separate mInternalRedirectCount I failed to propagate both values across all kinds of redirects.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P1 Propagate mRedirectCount and mInternalRedirectCount across all redirects. r=valentin

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P3 Check ResetInterception result and CancelInterception if a failure occurs. r=asuth

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update redirected-response.https.html WPT expectation for redirect limit test. r=asuth

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

I still need to fix e10s case.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

Updated to check redirect limit in HttpChannelChild as well.

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P4 Update redirected-response.https.html WPT expectation for redirect limit test. r=asuth

https://treeherder.mozilla.org/#/jobs?repo=try&revision=becb448a5f9f502c1557070000d68e16100505c1

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8922386 [details] [diff] [review]
P1 Propagate mRedirectCount and mInternalRedirectCount across all redirects. r=valentin

Valentin, in bug 1405739 I separated out mInternalRedirectCount from mRedirectCount.  Unfortunately, though, I made a logic error.

The previous code increments the appropriate counter for the type of redirect and propagates that particular counter to the next channel.

The bug, however, is that we don't propagate the *other* counter to the next channel.  So if you have some thing does:

  "normal redirect"->"internal redirect"->"normal redirect"

You end up with an mRedirectCount of 1 and an mInternalRedirectCount of 0.  These should be mRedirect=2 and mInternalRedirectCount=1.

The solution in this patch is to always propagate both counters to the next channel regardless of the kind of redirect.  We still only update one or the other of course.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

I need to investigate some orange before flagging the rest of the patches.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

I need to fix P2 patch a bit more.  It doesn't look like nsHttpChannel enforces the redirect limit on internal redirects at all right now.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

This gets a consistent check, but it has issues in e10s still.  We're not getting canceled correctly there in this test case if the redirect fails.

Comment 14

[Ben Kelly [:bkelly, not reviewing]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca467f935aa9a6f8d342fdf274cc63ccbb8c9767

Comment 15

[Ben Kelly [:bkelly, not reviewing]]

Attachment: P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

https://treeherder.mozilla.org/#/jobs?repo=try&revision=403f02e89530d9144f7197a72e2d066c768c76f7

Comment 16

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8923031 [details] [diff] [review]
P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

Valentin, this patch unifies the redirection limit check by moving it into a helper routine on HttpBaseChannel.  I think ensure that its called for all parent-side redirects by calling it in both nsHttpChannel and InterceptedHttpChannel SetupReplacementChannel() methods.

I also had to add an extra check on the client-side for synthetic redirects coming from a service worker.  If a service worker provides a synthetic redirect it can then re-intercept and do it again.  In these cases the redirection count is not propagated in the parent since we are triggering the redirect in the child.  Therefore this patch also checks the limit when triggering a synthetic redirect.

Note, for service workers to function at the limits (20 synthetic redirects, etc) we need to allow slightly more internal redirects than real redirects.  For now I created a constant for the extra amount.  It seemed like overkill to create a pref for it.

Please note, this patch will make nsHttpChannel check the redirect limit on internal redirects.  I found many internal redirects were not being checked against the limit and could in theory infinitely recurse.

Comment 17

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8922403 [details] [diff] [review]
P3 Check ResetInterception result and CancelInterception if a failure occurs. r=asuth

Andrew, the additional redirect checking means we can now fail ResetInterception() due to hitting the limit on internal redirects.  In these cases we should cancel the channel so that OnStopRequest fires.

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8922504 [details] [diff] [review]
P4 Update redirected-response.https.html WPT expectation for redirect limit test. r=asuth

Update redirects-response.https.html WPT expectations.

Comment 19

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8923031 [details] [diff] [review]
P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin

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

Looks good! Thanks, Ben!

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +5565,5 @@
>      if (NS_FAILED(rv))
>          return rv;
>  
> +    rv = CheckRedirectLimit(redirectFlags);
> +    NS_ENSURE_SUCCESS(rv, rv);

nit: Is there a reason why we can't add the check to SetupReplacementChannel?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Valentin Gosu [:valentin] from comment #19)
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +5565,5 @@
> >      if (NS_FAILED(rv))
> >          return rv;
> >  
> > +    rv = CheckRedirectLimit(redirectFlags);
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> nit: Is there a reason why we can't add the check to SetupReplacementChannel?

Do you mean HttpBaseChannel::SetupReplacementChannel()?  I did try that, but enforcing the limit in the child broke some of the machinery around redirects in e10s mode.  I don't remember the exact problems, but this patch ended up being less intrusive.  We continue to do redirection limit enforcement in the parent process like before with the one exception for service worker interception.  (And hopefully we will remove service worker interception in the child soonish so that exception will be temporary.)

Comment 21

[Valentin Gosu [:valentin] (he/him)]

(In reply to Ben Kelly [:bkelly] from comment #20)
> (In reply to Valentin Gosu [:valentin] from comment #19)
> > 
> > nit: Is there a reason why we can't add the check to SetupReplacementChannel?
> 
> Do you mean HttpBaseChannel::SetupReplacementChannel()?  I did try that, but
> enforcing the limit in the child broke some of the machinery around
> redirects in e10s mode.  I don't remember the exact problems, but this patch
> ended up being less intrusive.  We continue to do redirection limit
> enforcement in the parent process like before with the one exception for
> service worker interception.  (And hopefully we will remove service worker
> interception in the child soonish so that exception will be temporary.)

That sounds good. Thanks!

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/39cb0f9cde37
P1 Propagate mRedirectCount and mInternalRedirectCount across all redirects. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/07285260f735
P2 Create a shared method in HttpBaseChannel to check the redirection limit. r=valentin
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d552d6d1581
P3 Check ResetInterception result and CancelInterception if a failure occurs. r=asuth
https://hg.mozilla.org/integration/mozilla-inbound/rev/b594a19da9dc
P4 Update redirected-response.https.html WPT expectation for redirect limit test. r=asuth

Comment 23

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/39cb0f9cde37
https://hg.mozilla.org/mozilla-central/rev/07285260f735
https://hg.mozilla.org/mozilla-central/rev/6d552d6d1581
https://hg.mozilla.org/mozilla-central/rev/b594a19da9dc

Comment 24

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/39cb0f9cde37
https://hg.mozilla.org/mozilla-central/rev/07285260f735
https://hg.mozilla.org/mozilla-central/rev/6d552d6d1581
https://hg.mozilla.org/mozilla-central/rev/b594a19da9dc