1172992 - Synthesized 301 response does not redirect in e10s

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, defect)

Description

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

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!

Comment 2

[(no longer active)]

This may be a dupe of bug 1170795.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

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

[(no longer active)]

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.

Comment 5

[Ben Kelly [:bkelly, not reviewing]]

I'm investigating.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

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)

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

Ehsan said he would take a look at this one.

Comment 8

[Jason Duell]

Michal, I have a vague (and maybe wrong) memory of there being existing issues with security info not getting cached in all cases?

Comment 9

[Michal Novotny [:michal]]

(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).

Comment 10

[Honza Bambas (:mayhemer)]

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)

Comment 11

[Honza Bambas (:mayhemer)]

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.

Comment 12

[Ben Kelly [:bkelly, not reviewing]]

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.

Comment 13

[(no longer active)]

(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.  :-)

Comment 14

[Honza Bambas (:mayhemer)]

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.

Comment 15

[Honza Bambas (:mayhemer)]

(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

[(no longer active)]

Do you mean adding it to HttpChannelOpenArgs?  That makes sense to me...

Comment 17

[Honza Bambas (:mayhemer)]

(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 ;)

Comment 18

[Ben Kelly [:bkelly, not reviewing]]

Honza, have you had a chance to look at this recently?

Comment 19

[Honza Bambas (:mayhemer)]

I'll do it now.

Comment 20

[Honza Bambas (:mayhemer)]

Attachment: v1

Comment 21

[Honza Bambas (:mayhemer)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=15c547d302c0

Comment 22

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/adffd0ca6322

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/adffd0ca6322