657076 - e10s FTP: Improve error handling in FTPChildChannel

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Jason Duell]

We need to make analogous fixes for FTP that we did in HTTP in bug 637339.

FTP should rename CancelEarly to RecvFailedAsyncOpen.

I don't see much else from 637339 that needs doing.  We don't have to import all the AsyncAbort logic, since we don't have the equivalent of of on-request observers to cancel things after we've committed to returning NS_OK, and we already handle the case where AsyncOpen fails on on the parent.

Oh, we need to add the same checks for if(mChannel) to Recv{suspend/resume/SetPriority} that we added for HTTP (same case: client can send these after AsyncOpen, but there's actually no channel on the parent, because it's asyncOpen failed). 

No templates or member function pointers needed! :)

Comment 1

[Jason Duell]

One more thing: to be polite, we should really FlushEventQueue asynchronously versus Resume, i.e. call AsyncCall.   I'm really fine with you duplicating the code for AsyncCall in FTPChannelChild (I don't see anything like it in nsBaseChannel: that might be the place to add it?).

Comment 2

[u408661]

Attachment: patch v1

First run at a patch (finally). Copied AsyncCall code instead of moving into base channel because... ugh, we don't need more layers of derived classes in that mess, especially not base/derived template classes. Unless we want to play the "How many C++ features can we put in one bugfix?" game.

Comment 3

[Jason Duell]

Comment on attachment 541109 [details] [diff] [review]
patch v1

Review of attachment 541109 [details] [diff] [review]:
-----------------------------------------------------------------

+r with a pair of fixes, below.

Took me a while to page this stuff back in.  For a bit I thought we'd need to use the same HttpAsyncAborter code for FTP, too (since it's possible that a child channel could be suspended before the FailAsyncOpen msg is received from the parent) but fortunately for e10s the EventQueue logic takes care of delaying DoFailedAsyncOpen until the right moment.  

I'll take care of updating and landing the patch.

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +420,3 @@
>  {
>    if (mCanceled)
>      return;

We need to remove this check, for the same reason as we removed it in HTTP in bug 637339:  even if the child-side necko client has canceled the channel, if we returned NS_OK on the child but AsyncOpen on the parent failed (that's when we arrive here at DoFailedAsyncOpen), we need to make sure we call OnStop/OnStop.

@@ +420,5 @@
>  {
>    if (mCanceled)
>      return;
>  
>    mCanceled = true;

Also remove setting of mCanceled.  Failed asyncOpen != cancel.  Also follows changes we did for HTTP.

Comment 4

[Jason Duell]

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

Also needed to move mIsPending = false to happen after OnStart call but before OnStop (like HttpBaseChannel does).

Tested manually--there's no feasible way to write tests for this.

Comment 5

[Ed Morley [:emorley]]

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