Closed Bug 1172992 Opened 8 years ago Closed 8 years ago

Synthesized 301 response does not redirect in e10s

Categories

(Core :: DOM: Service Workers, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
e10s + ---
firefox43 --- fixed

People

(Reporter: bkelly, Assigned: mayhemer)

References

()

Details

Attachments

(1 file)

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.
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)
This may be a dupe of bug 1170795.
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).
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.
I'm investigating.
Assignee: nobody → bkelly
Status: NEW → ASSIGNED
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)
Flags: needinfo?(honzab.moz)
Ehsan said he would take a look at this one.
Assignee: bkelly → ehsan
Blocks: ServiceWorkers-B2G
No longer blocks: ServiceWorkers-v1
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)
Flags: needinfo?(honzab.moz)
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)
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)
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)
(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: ehsan → honzab.moz
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.
(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).
Do you mean adding it to HttpChannelOpenArgs?  That makes sense to me...
(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 ;)
Honza, have you had a chance to look at this recently?
Flags: needinfo?(honzab.moz)
I'll do it now.
Flags: needinfo?(honzab.moz)
Attached patch v1Splinter Review
Attachment #8648879 - Flags: review?(jduell.mcbugs)
Attachment #8648879 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/adffd0ca6322
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.