Last Comment Bug 657076 - e10s FTP: Improve error handling in FTPChildChannel
: e10s FTP: Improve error handling in FTPChildChannel
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Networking: FTP (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla12
Assigned To: Nicholas Hurley [:nwgh][:hurley]
:
Mentors:
Depends on:
Blocks:
  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:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


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

Description 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 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 Nicholas Hurley [:nwgh][:hurley] 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 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 Jason Duell [:jduell] (needinfo me) 2012-01-10 23:28:01 PST
  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] 2012-01-11 18:07:01 PST
https://hg.mozilla.org/mozilla-central/rev/e42fbf39edbf

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