Last Comment Bug 657076 - e10s FTP: Improve error handling in FTPChildChannel
: e10s FTP: Improve error handling in FTPChildChannel
Product: Core
Classification: Components
Component: Networking: FTP (show other bugs)
: unspecified
: All All
-- normal (vote)
: mozilla12
Assigned To: OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also
: Patrick McManus [:mcmanus]
Depends on:
  Show dependency treegraph
Reported: 2011-05-13 17:10 PDT by Jason Duell [:jduell] (needinfo me)
Modified: 2012-01-11 18:07 PST (History)
1 user (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch v1 (7.83 KB, patch)
2011-06-22 10:56 PDT, OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description User image Jason Duell [:jduell] (needinfo me) 2011-05-13 17:10:40 PDT
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 User image Jason Duell [:jduell] (needinfo me) 2011-05-13 17:35:48 PDT
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 User image OFFLINE UNTIL 27 FEB 2017 Nicholas Hurley [:nwgh][:hurley] (also 2011-06-22 10:56:47 PDT
Created attachment 541109 [details] [diff] [review]
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 User image Jason Duell [:jduell] (needinfo me) 2012-01-10 19:24:45 PST
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 User image Jason Duell [:jduell] (needinfo me) 2012-01-10 23:28:01 PST

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 User image Ed Morley [:emorley] 2012-01-11 18:07:01 PST

Note You need to log in before you can comment on or make changes to this bug.