586738 - e10s http: check HttpChannelChild::Redirect1Begin cancel logic

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[[e-mail]]

For the Cancel patch, I added the following two bits:
http://hg.mozilla.org/projects/electrolysis/file/d31ddf6547d7/netwerk/protocol/http/HttpChannelChild.cpp#l607
http://hg.mozilla.org/projects/electrolysis/file/d31ddf6547d7/netwerk/protocol/http/HttpChannelChild.cpp#l620

jduell and I decided that Cancel + redirect veto was the right thing to do in those failure cases, but we want to make sure.

Honza, thoughts?

Comment 1

[Honza Bambas (:mayhemer)]

Sorry, but this is completely wrong.

In HttpChannelChild::Redirect1Begin when we are unable to setup the new channel we should only consider this as a redirect veto, i.e. let the nsHttpChannel on chrome decide how to continue, the logic is btw always different with different types of redirecting, sometimes cancel, sometimes not.

In HttpChannelChild::Redirect3Complete we have to cancel the mRedirectChannelChild, not the old channel (*this).  The new channel is already running on the chrome (has been AsyncOpenned), so we have to stop it.  But as I can see, call to CompleteRedirectSetup should never fail.

However, CompleteRedirectSetup should add check 'if (mCanceled) return mStatus;', as AsyncOpen does.  But we may get here, while mCanceled is true, only when the new channel is canceled after we called all redirect sinks and sent the verify result to parent, or even later.  There is a tight race condition here, closely related, if not the same, to bug 586766.  Any ETA on (decision how to) fix that?

...

After going through this complicated code, I would personally rename Redirect2Result to Redirect2Verify because it is actually coming from OnRedirectVerifyCallback.  There is also HttpChannelParentListener::OnRedirectResult that has a different meaning (called after we know we are really redirecting or not) and these two names are a bit confusing IMHO.

Comment 3

[Honza Bambas (:mayhemer)]

--> me

Comment 4

[Honza Bambas (:mayhemer)]

Attachment: v1

- fixed the cancel logic
- fixed crash in Redirect1

Comment 5

[Honza Bambas (:mayhemer)]

Jason, any updates on reviewing this?

Comment 6

[Jason Duell]

Attachment: changes to comments, and fix unrelated LOG issue

Comment 7

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/495f10f3a139

Comment 8

[Honza Bambas (:mayhemer)]

> // Release ref to new channel.
Maybe next time leave a comment why we do/can do something like that...