1233245 - We shouldn't intercept the redirected channel if the original channel has hit the network in non-e10s

Status: RESOLVED FIXED

Description

[:Ehsan Akhgari]

Currently we do this properly for internal redirects but not HTTP redirects.  navigation-redirect.https.html's expected value in the first two tests is incorrect.  I have a fix.

Comment 1

[:Ehsan Akhgari]

Attachment: Propagate the interception information in the non-e10s case for all HTTP redirects, not just the internal ones

Doing this work in StartRedirectChannelToURI() was causing us to not set
the bypass service worker flag properly in the case of real HTTP redirects.
This patch also fixes the incorrect test expectation value.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

The spec is changing here.  The test is probably correct.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

I'd like to look at this before it lands, but I'm away from my computer ATM.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Please see spec discussion linked in bug 1229369.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8699229 [details] [diff] [review]
Propagate the interception information in the non-e10s case for all HTTP redirects, not just the internal ones

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

This is consistent with our previous understanding of the spec, but the test changes will be reversed by bug 1229369.

r=me I guess, although I would prefer us just implement the behavior intended by the spec editors and described in bug 1229369.

Comment 6

[:Ehsan Akhgari]

(In reply to Ben Kelly [:bkelly] from comment #5)
> r=me I guess, although I would prefer us just implement the behavior
> intended by the spec editors and described in bug 1229369.

We can do that later, in both e10s and non-e10s mode.  The bigger issue here is our behavior is different in the two cases, which will be problematic in 45.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

(In reply to :Ehsan Akhgari from comment #6)
> We can do that later, in both e10s and non-e10s mode.  The bigger issue here
> is our behavior is different in the two cases, which will be problematic in
> 45.

Ok.  I guess this was masked by the crash on e10s so we never noticed the differences?

Comment 8

[:Ehsan Akhgari]

(In reply to Ben Kelly [:bkelly] from comment #7)
> (In reply to :Ehsan Akhgari from comment #6)
> > We can do that later, in both e10s and non-e10s mode.  The bigger issue here
> > is our behavior is different in the two cases, which will be problematic in
> > 45.
> 
> Ok.  I guess this was masked by the crash on e10s so we never noticed the
> differences?

Yes.

Comment 9

[:Ehsan Akhgari]

Comment on attachment 8699229 [details] [diff] [review]
Propagate the interception information in the non-e10s case for all HTTP redirects, not just the internal ones

This is needed for service workers and doesn't affect anything else, and doesn't add new strings or uuids.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/82cd6a486698

Comment 11

[Wes Kocher (:KWierso)]

Had to back this out for wpt failures like https://treeherder.mozilla.org/logviewer.html#?job_id=18739486&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/1c8ba5322a17

Comment 12

[Wes Kocher (:KWierso)]

Looks like some mochitests got busted, too: https://treeherder.mozilla.org/logviewer.html#?job_id=18738822&repo=mozilla-inbound

Comment 13

[Ritu Kothari (:ritu)]

Ehsan, I cannot uplift this to Beta44 (today) until the test failures are investigated/fixed. I will gtb 44.0b2 on Monday noon PST so if this is ready by then, I will take it.

Comment 14

[Ritu Kothari (:ritu)]

Comment on attachment 8699229 [details] [diff] [review]
Propagate the interception information in the non-e10s case for all HTTP redirects, not just the internal ones

Please re-nominate once the test issues are addressed (see comment 13).

Comment 15

[:Ehsan Akhgari]

Josh, did you get a chance to look at this bug?

Comment 16

[Josh Matthews [:jdm]]

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

Comment 17

[:Ehsan Akhgari]

I already have a fix...

Comment 18

[:Ehsan Akhgari]

Attachment: Propagate the interception information in the non-e10s case for all HTTP redirects, not just the internal ones

Doing this work in StartRedirectChannelToURI() was causing us to not set
the bypass service worker flag properly in the case of real HTTP redirects.
This patch also fixes the incorrect test expectation value.

Comment 19

[:Ehsan Akhgari]

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

Comment 20

[Josh Matthews [:jdm]]

Comment on attachment 8703020 [details] [diff] [review]
Propagate the interception information in the non-e10s case for all HTTP redirects, not just the internal ones

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

Just lacking the removal of the ini file.

Comment 21

[:Ehsan Akhgari]

Of course.  This is green together with bug 1219469. <https://treeherder.mozilla.org/#/jobs?repo=try&revision=a49f73edb6df>

Comment 22

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/655aa0601a8a

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/655aa0601a8a