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

RESOLVED FIXED in Firefox 46

Status

()

defect
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 1 bug)

Trunk
mozilla46
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s-, firefox46 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

3 years ago
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.
Attachment #8699229 - Flags: review?(josh)
The spec is changing here.  The test is probably correct.
I'd like to look at this before it lands, but I'm away from my computer ATM.
(Assignee)

Updated

3 years ago
Attachment #8699229 - Flags: review?(bkelly)
Please see spec discussion linked in bug 1229369.
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.
Attachment #8699229 - Flags: review?(bkelly) → review+
(Assignee)

Comment 6

3 years ago
(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.
(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?
(Assignee)

Comment 8

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

Comment 9

3 years ago
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.
Attachment #8699229 - Flags: approval-mozilla-beta?
Attachment #8699229 - Flags: approval-mozilla-aurora?
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 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).
Attachment #8699229 - Flags: approval-mozilla-beta?
Attachment #8699229 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 15

3 years ago
Josh, did you get a chance to look at this bug?
Flags: needinfo?(ehsan) → needinfo?(josh)
(Assignee)

Comment 17

3 years ago
I already have a fix...
(Assignee)

Comment 18

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

Updated

3 years ago
Attachment #8699229 - Attachment is obsolete: true
(Assignee)

Updated

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

Comment 21

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

Comment 23

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/655aa0601a8a
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.