Closed Bug 1233245 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s - ---
firefox46 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

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.
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.
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+
(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?
(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+
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?
Josh, did you get a chance to look at this bug?
Flags: needinfo?(ehsan) → needinfo?(josh)
I already have a fix...
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 - Attachment is obsolete: true
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)
Of course.  This is green together with bug 1219469. <https://treeherder.mozilla.org/#/jobs?repo=try&revision=a49f73edb6df>
https://hg.mozilla.org/mozilla-central/rev/655aa0601a8a
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.