Closed
Bug 1233245
Opened 7 years ago
Closed 7 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)
Core
DOM: Service Workers
Tracking
()
RESOLVED
FIXED
mozilla46
People
(Reporter: ehsan.akhgari, Assigned: ehsan.akhgari)
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.
Assignee | ||
Comment 1•7 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)
Comment 2•7 years ago
|
||
The spec is changing here. The test is probably correct.
Comment 3•7 years ago
|
||
I'd like to look at this before it lands, but I'm away from my computer ATM.
Assignee | ||
Updated•7 years ago
|
Attachment #8699229 -
Flags: review?(bkelly)
Comment 4•7 years ago
|
||
Please see spec discussion linked in bug 1229369.
Updated•7 years ago
|
tracking-e10s:
--- → -
Comment 5•7 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 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•7 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.
Comment 7•7 years ago
|
||
(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•7 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.
Updated•7 years ago
|
Attachment #8699229 -
Flags: review?(josh) → review+
Assignee | ||
Comment 9•7 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?
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
Flags: needinfo?(ehsan)
Looks like some mochitests got busted, too: https://treeherder.mozilla.org/logviewer.html#?job_id=18738822&repo=mozilla-inbound
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•7 years ago
|
||
Josh, did you get a chance to look at this bug?
Flags: needinfo?(ehsan) → needinfo?(josh)
Assignee | ||
Comment 17•7 years ago
|
||
I already have a fix...
Assignee | ||
Comment 18•7 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•7 years ago
|
Attachment #8699229 -
Attachment is obsolete: true
Assignee | ||
Updated•7 years ago
|
Attachment #8703020 -
Flags: review?(josh)
Assignee | ||
Comment 19•7 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=430aec1a737d
Comment 20•7 years ago
|
||
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+
Updated•7 years ago
|
Flags: needinfo?(josh)
Assignee | ||
Comment 21•7 years ago
|
||
Of course. This is green together with bug 1219469. <https://treeherder.mozilla.org/#/jobs?repo=try&revision=a49f73edb6df>
Comment 23•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/655aa0601a8a
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•