Closed
Bug 915024
Opened 11 years ago
Closed 10 years ago
e10s FTP broken with proxies
Categories
(Core Graveyard :: Networking: FTP, defect)
Core Graveyard
Networking: FTP
Tracking
(e10s+)
RESOLVED
FIXED
mozilla33
Tracking | Status | |
---|---|---|
e10s | + | --- |
People
(Reporter: jduell.mcbugs, Assigned: dragana)
References
Details
Attachments
(1 file, 3 obsolete files)
22.19 KB,
patch
|
dragana
:
review+
|
Details | Diff | Splinter Review |
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.
Flags: needinfo?(honzab.moz)
Comment 1•11 years ago
|
||
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?
Flags: needinfo?(honzab.moz)
Reporter | ||
Comment 2•11 years ago
|
||
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•11 years ago
|
||
(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 ?
Updated•11 years ago
|
tracking-e10s:
--- → +
Assignee | ||
Comment 4•10 years ago
|
||
Attachment #8443495 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 5•10 years ago
|
||
I almost forgot. Probably we will need a fix after bug 748117 gets through. adding ForcePending for HttpChannel as well. I will change this.
Assignee | ||
Comment 6•10 years ago
|
||
this is additional fix depending on bug 748117
Attachment #8444338 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Updated•10 years ago
|
Attachment #8443495 -
Attachment description: text/plain → Fix v1
Reporter | ||
Comment 7•10 years ago
|
||
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).
Attachment #8443495 -
Flags: review?(jduell.mcbugs) → feedback+
Reporter | ||
Comment 8•10 years ago
|
||
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.
Attachment #8444338 -
Flags: review?(jduell.mcbugs) → feedback+
Reporter | ||
Comment 9•10 years ago
|
||
Might as well combine the 2 patches in your next revision ("hg qfold" does that nicely, by the way).
Assignee | ||
Comment 10•10 years ago
|
||
this version also fix problem with tar.gz files
Attachment #8443495 -
Attachment is obsolete: true
Attachment #8444338 -
Attachment is obsolete: true
Attachment #8447969 -
Flags: review?(jduell.mcbugs)
Reporter | ||
Comment 11•10 years ago
|
||
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.
Attachment #8447969 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 12•10 years ago
|
||
Assignee: nobody → dd.mozilla
Attachment #8447969 -
Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8453882 -
Flags: review+
Assignee | ||
Comment 13•10 years ago
|
||
Assignee | ||
Comment 14•10 years ago
|
||
I am not allowed to add keywords, can you please mark it as checkin-needed
Flags: needinfo?(jduell.mcbugs)
Comment 15•10 years ago
|
||
Now you are!
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(jduell.mcbugs)
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
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
Keywords: checkin-needed
Comment 17•10 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Updated•9 months ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•