Synthesized 301 response does not redirect in e10s

RESOLVED FIXED in Firefox 43

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bkelly, Assigned: mayhemer)

Tracking

unspecified
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox43 fixed)

Details

(URL)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
The test case from bug 1172884 has an additional problem beyond garbling the output.  It just doesn't redirect properly when in e10s mode.  With the patch from bug 1172884 we can now see that we get a 404 response page in e10s mode instead of the proper redirected result.  It works in non-e10s.
(Reporter)

Comment 1

3 years ago
Honza, do you have any ideas on this one?  I'm not that familiar with the redirect process in e10s and Josh is on PTO for the rest of the month.  Thanks!
Flags: needinfo?(honzab.moz)

Comment 2

3 years ago
This may be a dupe of bug 1170795.
(Reporter)

Comment 3

3 years ago
Bug 1170795 seems to be about when there is no Location header.  In this case we do have a Location header.  Sometimes it works (non-e10s in the browser or mochitests) and sometimes it doesn't (browser e10s).

Comment 4

3 years ago
Oh, I see...  I thought that bug 1157283 has fixed this in the e10s case, but the tests in that file are using Response.redirect() in your test changes something?

At any rate, I don't know of any other open bugs about this issue.
(Reporter)

Comment 5

3 years ago
I'm investigating.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
(Reporter)

Comment 6

3 years ago
I think the issue is that the security_info is not getting set properly in this case for some reason.  The CacheEntry is failing to open because it can't find the security info.  I changed the failure into an assert to see the error:

Assertion failure: ((bool)(!!(!NS_FAILED_impl(rv)))), at c:/devel/mozilla-central/netwerk/cache2/CacheEntry.cpp:1172
#01: mozilla::net::nsHttpChannel::OnCacheEntryCheck (c:\devel\mozilla-central\netwerk\protocol\http\nshttpchannel.cpp:3256)
#02: mozilla::net::CacheEntry::InvokeCallback (c:\devel\mozilla-central\netwerk\cache2\cacheentry.cpp:678)
#03: mozilla::net::CacheEntry::InvokeCallbacks (c:\devel\mozilla-central\netwerk\cache2\cacheentry.cpp:612)
#04: mozilla::net::CacheEntry::InvokeCallbacks (c:\devel\mozilla-central\netwerk\cache2\cacheentry.cpp:566)
#05: mozilla::net::CacheEntry::Open (c:\devel\mozilla-central\netwerk\cache2\cacheentry.cpp:308)
#06: mozilla::net::CacheEntry::AsyncOpen (c:\devel\mozilla-central\netwerk\cache2\cacheentry.cpp:280)
#07: mozilla::net::CacheStorage::AsyncOpenURI (c:\devel\mozilla-central\netwerk\cache2\cachestorage.cpp:109)
#08: mozilla::net::nsHttpChannel::OpenCacheEntry (c:\devel\mozilla-central\netwerk\protocol\http\nshttpchannel.cpp:2828)
#09: mozilla::net::InterceptedChannelChrome::FinishSynthesizedResponse (c:\devel\mozilla-central\netwerk\protocol\http\interceptedchannel.cpp:218)
#10: mozilla::net::FinishSynthesizedResponse::Run (c:\devel\mozilla-central\netwerk\protocol\http\httpchannelparent.cpp:193)
(Reporter)

Updated

3 years ago
Flags: needinfo?(honzab.moz)
(Reporter)

Comment 7

3 years ago
Ehsan said he would take a look at this one.
Assignee: bkelly → ehsan

Updated

3 years ago
Blocks: 1131322
No longer blocks: 1059784

Comment 8

3 years ago
Michal, I have a vague (and maybe wrong) memory of there being existing issues with security info not getting cached in all cases?
Flags: needinfo?(michal.novotny)
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #8)
> Michal, I have a vague (and maybe wrong) memory of there being existing
> issues with security info not getting cached in all cases?

I remember only a problem with invalid security info caused by changing the format (bug 1083922) and a problem with storing security info before it is validated (bug 1040086).
Flags: needinfo?(michal.novotny)
tracking-e10s: --- → +
(Assignee)

Updated

3 years ago
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 10

3 years ago
I believe the sec-info is never set on the entry in e10s.  To investigate look when the metadata key/value for sec-info is being set in non-e10s.  then check it's not set in e10s.

jdm would know, but I can't add ni (vacation)
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 11

3 years ago
Ehsan, I can take this bug if you want.

Ben, please tell me how to reproduce this bug exactly.  I may check my suggestion from comment 10.
Flags: needinfo?(ehsan)
Flags: needinfo?(bkelly)
(Reporter)

Comment 12

3 years ago
Honza, try this:

1) enable e10s
2) visit: https://azasypkin.github.io/sw-tests/permanent-redirect/
3) maybe reload here to ensure SW is installed
4) click "reload" link

You should see a page showing:

  "Yay! You've been redirected correctly!"

In e10s you instead get a 404 because it tries to load the bogus page from github.

The redirect works in non-e10s mode.
Flags: needinfo?(bkelly)

Comment 13

3 years ago
(In reply to Honza Bambas (:mayhemer) from comment #11)
> Ehsan, I can take this bug if you want.

Yes please, that would be very nice.  :-)
Flags: needinfo?(ehsan)
(Assignee)

Updated

3 years ago
Assignee: ehsan → honzab.moz
(Assignee)

Comment 14

3 years ago
We fail to open the sec info (as Ben already pointed out).  Here is why:

ChannelInfo::ResurrectInfoOnChannel sets the correct sec-info on the child channel.  But too late.  The child channel side is already open and we don't send sec-info up any longer after that point.  Hence HttpBaseChannel::OverrideSecurityInfo needs to send the info up to the parent that will override the sec-info on the nsHttpChannel that will intercept too and carry this sec-info to the artificial cache entry.  Complicated path, but doable.
(Assignee)

Comment 15

3 years ago
(In reply to Honza Bambas (:mayhemer) from comment #14)
> We fail to open the sec info (as Ben already pointed out).  Here is why:
> 
> ChannelInfo::ResurrectInfoOnChannel sets the correct sec-info on the child
> channel.  But too late.  The child channel side is already open and we don't
> send sec-info up any longer after that point.  Hence
> HttpBaseChannel::OverrideSecurityInfo needs to send the info up to the
> parent that will override the sec-info on the nsHttpChannel that will
> intercept too and carry this sec-info to the artificial cache entry. 
> Complicated path, but doable.

It's yet a bit different: the channel is not yet opened, but we don't send the info up to the parent in the IPC constructor (open args).

Comment 16

3 years ago
Do you mean adding it to HttpChannelOpenArgs?  That makes sense to me...
(Assignee)

Comment 17

3 years ago
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #16)
> Do you mean adding it to HttpChannelOpenArgs?  That makes sense to me...

Exactly.  I'm doing reviews right now.  After that I'll fix and test this.  But feel free to steal it from me back ;)
(Reporter)

Comment 18

3 years ago
Honza, have you had a chance to look at this recently?
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 19

3 years ago
I'll do it now.
Flags: needinfo?(honzab.moz)
(Assignee)

Comment 20

3 years ago
Created attachment 8648879 [details] [diff] [review]
v1
Attachment #8648879 - Flags: review?(jduell.mcbugs)

Updated

3 years ago
Attachment #8648879 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/adffd0ca6322
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.