586766 - e10s http: fix redirect/cancel race

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[[e-mail]]

http://tinderbox.mozilla.org/showlog.cgi?log=Electrolysis/1281606407.1281607261.2217.gz

test_httpcancel_wrap.js is failing; jduell speculates this is a race between redirection on the parent and cancelling (via OMR observer) on the child.

Possible fix is to suspend the parent in HttpChannelParentListener::OnRedirectResult, then resume the parent in HttpChannelChild::CompleteRedirectSetup after OMR notification.

Patch forthcoming.

Comment 1

[[e-mail]]

From IRC:

<jduell_> we suspend on the parent in HttpChannelParentListener::OnRedirectResult
[13:04]  <jduell_> We increment mSuspendCount in the child at the top of RecvRedirect3Complete
[13:04]  <jduell_> and then we call Resume after the OMRs in HttpChannelChild::CompleteRedirectSetup
[13:05]  <jduell_> We can get rid of the comment about TODO: OMR observers may have canceled
[13:05]  <jduell_> because if they did, it just works (they cause sendCancel)
[13:05]  <jduell_> But that's the place where we should resume--also after the Add to Loadgroup (because that should be able to cancel too)

Comment 2

[[e-mail]]

Wrong test quoted above -- this isn't causing any tests to fail yet. We'll need to add a test for this.

Comment 3

[Honza Bambas (:mayhemer)]

After some talk on IRC, if I follow correctly, we have two issues:

In general, on-modify-request observers may cancel the channel, and we define that when it happens, there is no network activity then, so:

1. on redirect target channel, on-modify-request observer may cancel the HttpChannelChild when called from CompleteRedirectSetup, that is after we have already started network/cache load in chrome.  We could move call to on-modify-request observers to HttpChannelChild::OnRedirectVerifyCallback and if the child gets canceled, we veto the redirect, or count on getting HttpChannelParent::RecvCancel.

2. on-modify-request observers are also called from nsHttpChannel::DoAuthRetry.  But these observes apparently are not able to cancel the net request.  Before we setup the new transaction and do AsyncRead on its pump, we never check the cancellation state.  Is that a bug?  So, we just have to send to the child OMR async message and if any OMR observer cancels the channel there, we'll get the cancellation event as in the previous case, but maybe too late.

Opinions?

Comment 4

[[e-mail]]

Attachment: proposed patch

This is what jduell proposed in comment 1. No test hackery yet.

I'm going to drop this for now in favor of getting test_httpcancel.js passing. :)

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to comment #3)
> We could move call to
> on-modify-request observers to HttpChannelChild::OnRedirectVerifyCallback

This might be wrong if we want OMR callbacks be notified only when the load actually going to happen.  If we do it here, the redirect may get canceled later on the chrome process, but OMR observer has already been notified.

Comment 6

[Honza Bambas (:mayhemer)]

Any progress here?

Comment 7

[[e-mail]]

Not from me. If you want to take this, go ahead -- you know what's going on better than I. :)

Regardless, it sounds like 1) is easy to implement. It sounds like 2), if it's a bug at all, is currently a bug in nsHttpChannel anyway -- so no need to fix it now.

Would moving the OMR notification call into HttpChannelChild::OnRedirectVerifyCallback change any of the channel information available to the OMR observer? (i.e. will the channel state appear the same?) If it won't impact the OMR observer then it sounds like an easy fix.

Comment 8

[Honza Bambas (:mayhemer)]

(In reply to comment #7)
> Would moving the OMR notification call into
> HttpChannelChild::OnRedirectVerifyCallback change any of the channel
> information available to the OMR observer? (i.e. will the channel state appear
> the same?)

I'm pretty sure the state is the same.  We need cookies added, before mListener is set, and original uri has been set.  It is exactly before call to SendRedirect2Result.  If OMR observer cancel the child, the message will get to the parent sooner then the answer about redirect verify, so it will be vetoed before AsyncCall on the new channel.  So, it is an easy fix.

As for 2) I'll add async call to OMR observers on the child process.  No need to do more at the moment I'd say.

-> me

Comment 9

[Honza Bambas (:mayhemer)]

Attachment: v1

- moved on-modify-request to HttpChannelChild::OnRedirectVerifyCallback
- renamed SendRedirect2Result to SendRedirect2Verify ; less confusing 

Patch on top of patch for bug 586738

Comment 10

[Jason Duell]

Attachment: cleaned up comments [Check in comment 11]

Looks good.

One question:  So this bug was causing test_httpcancel_wrap.js to fail, but we don't seem to have disabled that test.  Have we just been living with it as a random orange?  This bug not marked as "orange". 

The test wrapper also has a random "dump(and here)" line, which I've removed, as I don't see what it's for.

Comment 11

[Jason Duell]

http://hg.mozilla.org/mozilla-central/rev/434dbaad7c5b