915024 - e10s FTP broken with proxies

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Jason Duell]

When using an HTTP proxy FTP winds up redirecting to an HTTP channel to service the request.  We were casting the resulting nsHttpChannel blindly to a ns nsFTPChannel, which caused bug 898156.  That's fixed, but for now we're just canceling the channel in that case.  We should instead work with the redirect.

There are two possible fixes here.  1) we do the full e10s redirect dance, replacing the FTPChannelChild with an HTTPChannelChild.  2) We may be able to get away with just grabbing the info we need from the HTTP channel and sending it to the FTPChannelChild "as if" we were never redirected.  It looks like all the info we send in OnStartRequest can be gotten from the HTTP channel (the main difference is we get last-modified in a different way than nsIFTPChannel). 

#2 would be simpler, but I may be overlooking something that would break if we don't wind up replacing the FTP channel with an HTTP channel in the child. Honza, do you know of any reason that could break things?

This isn't urgent, since the only consequence of the status quo is that we don't create thumbnails of FTP sites (I don't expect many/any B2G users to be using an HTTP proxy and doing FTP). But we should fix it.

Comment 1

[Honza Bambas (:mayhemer)]

I think #2 sounds good.  This is just an internal redirect actually.  

Is it just about changing 'aRequest->Cancel(NS_ERROR_NOT_IMPLEMENTED);' to something else and going on with OnStartRequest?

Comment 2

[Jason Duell]

IIRC the content-type of an index page is actually different when we deliver it via HTTP proxy vs regular FTP channel.  I'm not sure yet if the code that handles the data relies on just the content-type, or also expects it to match the type of channel (i.e. if it's happy getting the HTTP proxy content-type--"application/http-index-format" from what looks like an nsIFTPChannel.

Comment 3

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #2)
> IIRC the content-type of an index page is actually different when we deliver
> it via HTTP proxy vs regular FTP channel.  I'm not sure yet if the code that
> handles the data relies on just the content-type, or also expects it to
> match the type of channel (i.e. if it's happy getting the HTTP proxy
> content-type--"application/http-index-format" from what looks like an
> nsIFTPChannel.

Can you hack FTP channel to change the content-type to what HTTP would deliver it and try w/o a proxy ?

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: Fix v1

Comment 5

[Dragana Damjanovic [:dragana]]

I almost forgot. Probably we will need a fix after bug 748117 gets through. adding ForcePending for HttpChannel as well. I will change this.

Comment 6

[Dragana Damjanovic [:dragana]]

Attachment: additional_fix_depending_on_bug748117 - v1

this is additional fix depending on bug 748117

Comment 7

[Jason Duell]

Comment on attachment 8443495 [details] [diff] [review]
Fix v1

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

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +120,4 @@
>  
>    if (mPBOverride != kPBOverride_Unset) {
> +    nsCOMPtr<nsIPrivateBrowsingChannel> privateBrowsingChannel = do_QueryInterface(mChannel);
> +    privateBrowsingChannel->SetPrivate(mPBOverride == kPBOverride_Private ? true : false);

traditionally we always do a check vs null after a QI.  In this case I guess we're fine since we know we've got an nsFTPChannel.  But maybe it's nicer to add a separate temp variable:

  // later on mChannel could actually be an HTTP channel (if we're using an proxy), but 
  // for now this is safe.
  nsFTPChannel ftpChan = static_cast<nsFtpChannel*>(mChannel)

and then use than instead of the 3 QIs.

@@ +581,5 @@
>    mChannel->Cancel(aErrorCode);
> +  nsCOMPtr<nsIFTPChannel> ftpIChan = do_QueryInterface(mChannel);
> +  if (ftpIChan) {
> +    nsFtpChannel* ftpChan = static_cast<nsFtpChannel*>(mChannel.get());
> +    ftpChan->ForcePending(false);

OK, this is kind of ugly, but we need to also handle the case where an FTP-over-http-proxy channel is diverted back to the parent.  So if the QI to nsIFTPChannel fails, do a QI to nsIHTTPChannelInternal.idl and call forcePending through that interface instead.  I suppose a 'cleaner' solution would be to add some new "nsIForcePendingChannel" IDL that you could QI only once and it would work for both HTTP/FTP channels.  But I'm not sure it's worth the fuss.

@@ +621,5 @@
>    }
>  }
>  
> +//-----------------------------------------------------------------------------
> +// FTPChannelParent::nsIChnnelEventSink

nsIChannelEventSink  (missing an 'a')

@@ +634,5 @@
> +{
> +  nsCOMPtr<nsIFTPChannel> ftpChan = do_QueryInterface(newChannel);
> +  if (!ftpChan)
> +  {
> +    nsCOMPtr<nsIHttpChannel> httpChan = do_QueryInterface(newChannel);

put '{' on same line as "if".

Add comment:

  // when FTP is set to use HTTP proxying, we wind up getting redirected to an HTTP channel.

::: netwerk/protocol/ftp/FTPChannelParent.h
@@ +77,5 @@
>    virtual bool RecvDivertComplete() MOZ_OVERRIDE;
>  
>    virtual void ActorDestroy(ActorDestroyReason why) MOZ_OVERRIDE;
>  
> +  nsCOMPtr<nsIChannel> mChannel;

add comment:

  // if configured to use HTTP proxy for FTP, this can an an HTTP channel.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +1619,5 @@
> +NS_IMETHODIMP
> +HttpBaseChannel::GetLastModifiedTime(PRTime* lastModifiedTime)
> +{
> +  uint32_t lastMod;
> +  mResponseHead->GetLastModifiedValue(&lastMod);

add a check for !mResponseHead to the top of this function, and return NS_ERROR_NOT_AVAILABLE if it's null (i.e. if this gets called before we have a response).

Comment 8

[Jason Duell]

Comment on attachment 8444338 [details] [diff] [review]
additional_fix_depending_on_bug748117 - v1

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

Aha--I see you already thought about the diversion case :)

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +6,5 @@
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "mozilla/net/FTPChannelParent.h"
>  #include "nsFTPChannel.h"
> +#include "./../http/nsHttpChannel.h"

Add 'nsHttpChannel.h: to  netwerk/protocol/http/moz.build in the EXPORTS section and then just "include nsHttpChannel.h"

Except that you don't need to cast the channel to nsHttpChannel: you just need to QI it to nsIHttpChannelInternal (which has forcePending in the IDL).  So just include nsIHttpChannelInternal.h here.

@@ +249,5 @@
>      if (ftpIChan) {
>        nsFtpChannel* ftpChan = static_cast<nsFtpChannel*>(mChannel.get());
>        ftpChan->ForcePending(false);
>      }
> +    nsCOMPtr<nsIHttpChannel> httpIChan = do_QueryInterface(mChannel);

use nsIHttpChannelInternal as I mentioned and skip the cast.

Comment 9

[Jason Duell]

Might as well combine the 2 patches in your next revision ("hg qfold" does that nicely, by the way).

Comment 10

[Dragana Damjanovic [:dragana]]

Attachment: fix v2

this version also fix problem with tar.gz files

Comment 11

[Jason Duell]

Comment on attachment 8447969 [details] [diff] [review]
fix v2

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

Looks good.

Go ahead and make the little fixes, mark the next patch +r and if it runs OK through try, mark 'checkin-needed'.

::: netwerk/protocol/ftp/FTPChannelParent.cpp
@@ +120,5 @@
>  
> +  mChannel = chan;
> +
> +  // later on mChannel could actually be an HTTP channel (if we're using an proxy), but 
> +  // for now this is safe.

// later on mChannel may become an HTTP channel (we'll be redirected to one if we're
  // using a proxy), but for now this is safe

@@ +316,5 @@
> +  if (httpChan) {
> +    httpChan->GetLastModifiedTime(&lastModified);
> +    nsCOMPtr<nsIEncodedChannel> encodedChannel = do_QueryInterface(aRequest);
> +    if (encodedChannel)
> +      encodedChannel->SetApplyConversion(false);

Hmm. I suspect that the correct fix for the encodedChannel stuff will be the approach in bug 1012917 (also in my review queue :).  For now let's skip this part of the patch, and I'll look at how it to fix it when I review that bug.

::: netwerk/protocol/ftp/nsFTPChannel.h
@@ +90,5 @@
>      // Helper function for getting the nsIFTPEventSink.
>      void GetFTPEventSink(nsCOMPtr<nsIFTPEventSink> &aResult);
>  
>  public: /* Internal Necko use only. */
> +    NS_IMETHOD ForcePending(bool aForcePending);

Now that this is an IDL method we don't have an "internal necko use only" section: just get rid of the whole "public: /* Internal Necko use only. */" line.

Comment 12

[Dragana Damjanovic [:dragana]]

Attachment: fix v3

Comment 13

[Dragana Damjanovic [:dragana]]

https://tbpl.mozilla.org/?tree=Try&rev=7e523e615345

Comment 14

[Dragana Damjanovic [:dragana]]

I am not allowed to add keywords, can you please mark it as checkin-needed

Comment 15

[Josh Matthews [:jdm]]

Now you are!

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2b953915d607

I did my best to guess a commit message for this. In the future, please make sure your patch commit message follows the guidelines below. Thanks :)
https://developer.mozilla.org/en-US/docs/Developer_Guide/Committing_Rules_and_Responsibilities#Checkin_comment

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2b953915d607