536289 - Make FTP work for fennec

Status: RESOLVED FIXED

(Core Graveyard :: Networking: FTP, defect)

Description

[Jason Duell]

I think the most obvious plan for FTP for the initial release of fennec e10s is to just keep it working as it already does, i.e. HTTP will be shipped off to the chrome process, but tab processes will continue to have a SocketTransport thread for FTP.

The main initial reason for sending HTTP to chrome is to reduce memory footprint (NSS, caching), and to keep cookies in sync.  Unless I'm mistaken, FTP has no state that we care about maintaining between tabs (do we store FTP passwords?), nor does the implementation take up a significant amount of space.

For security reasons we may eventually want to limit network access privileges to chrome, and at that point we'll need a different solution (probably having chrome both handle FTP and render it for the tab, so we don't need to change the network code itself).

We should make sure any proxy server info is propagated to child processes, so that they can use it for FTP connections.   Other than that, I think this bug will mainly be a matter of verifying that things still work as fennec ship time approaches.

Cc'ing Doug Turner, since he knows this code better than I.

Comment 1

[Michal Novotny [:michal]]

(In reply to comment #0)
> FTP has no state that we care about maintaining between tabs (do we store FTP
> passwords?), nor does the implementation take up a significant amount of space.

There is no auth cache for FTP. But there is a cache of idle control connections in nsFtpProtocolHandler.

Also don't forget that FTP code uses cache for directory listings, so this must be removed if it should run in tab process.

Comment 2

[Jason Duell]

So, we need to either 

1) Keep a very small cache around just for FTP directory listings, so that FTP can run in content for now. (or remove the need to hit the cache in the FTP code, but I'm guessing that's more work).

or 

2) Move FTP entirely into chrome.  The idea here is to support only downloads (not loading content into the browser).

I've been assuming #1 is a lot less work, and a good enough solution until we implement child process sandboxing.

Comment 3

[Michal Novotny [:michal]]

(In reply to comment #2)
> 1) Keep a very small cache around just for FTP directory listings, so that FTP
> can run in content for now. (or remove the need to hit the cache in the FTP
> code, but I'm guessing that's more work).

Do you mean to remove caching in FTP at all? Or do you mean to not use the cache in e10s builds but keep it in non-e10s builds? IMO the former shouldn't be too much work.

Also the question here is why FTP uses the cache for directory listings? I guess that it is here just to avoid repeated downloads of the same listings when user browses through the listings. In this case it makes sense to store them only in the memory cache and I would also prefer setting a short expiration time. Btw. we will probably need such in-memory cache for WyciwygChannel too.

Comment 4

[Doug Turner (:dougt)]

with the fb-cache, there really is no reason to cache LIST responses.  we could remove that....

however, i am not sure why we just wouldn't move ftp to chrome and support content loads.  it seams that if we are already doing the work to support HTTP in chrome, FTP would be pretty straight forward.

Comment 5

[Jason Duell]

So after our phone conference today the plan is to turn off the socket transport thread and either 1) not support FTP; 2) support FTP via chrome.

Bsmedberg suggested we only support downloading files via FTP links in web pages.  I.e. no displaying FTP content in the browser, including directory listings.  There was talk of providing directory listings, too, but I'm not sure we have a clear architecture for it.

Doug: is FTP support for downloading files enough for fennec?

Comment 6

[Doug Turner (:dougt)]

i do not think so.

Comment 7

[Jason Duell]

Attachment: FTP/e10s design notes

So we've going to try a full-fledged FTP e10s impl, like HTTP.  I've attached some notes for what I see as the logical design/implementation steps.

Comment 8

[Alon Zakai (:azakai)]

Attachment: Working patch, early version

This is a working patch for this bug (tested as described in the patch, in the file test_simple_wrap_ftp.js). However it is probably just an early version, given it's my first patch for Firefox... feedback is obviously very welcome.

Some notes on the patch:

1. I moved DIE_WITH_ASYNC_OPEN_MSG and ENSURE_CALLED_BEFORE_ASYNC_OPEN from HttpBaseChannel.h to NeckoCommon.h, as they are also useful for FTP channels. Not sure though if this is what should be done.
2. Should FTP channels have contentLength? In the current patch the check for that is worked around (with an ugly hack), so it is ignored.
3. The patch uses NECKO_E10S_HTTP for now - I didn't define a new env. var to check.

Comment 9

[Doug Turner (:dougt)]

Comment on attachment 444431 [details] [diff] [review]
Working patch, early version

jason, can you take a look.

Comment 10

[Alon Zakai (:azakai)]

Attachment: Improved patch with content length reported properly

Content length is now reported properly. The hackish workaround for the test harness relying on content length has been removed.

Comment 11

[Alon Zakai (:azakai)]

Attachment: Improved patch to take into account bug 565223

Takes into account bug 565223 - prevent crashes when trying to send data to a crashed child process.

Comment 12

[Doug Turner (:dougt)]

Comment on attachment 445213 [details] [diff] [review]
Improved patch to take into account bug 565223

jdm, do you have cycles to review please?

Comment 13

[Josh Matthews [:jdm]]

Can do.

Comment 14

[Jason Duell]

jdm: make sure to check that refcnting works.  I seem to remember this patch doing things a bit differently than HTTP.

Comment 15

[Josh Matthews [:jdm]]

Comment on attachment 445213 [details] [diff] [review]
Improved patch to take into account bug 565223

Before I get into the gritty stuff (which is mostly nits), the glaring issue right now is the lack of a Send__delete__ call anywhere,  meaning the IPDL protocol will stay alive forever.  Given that you're currently calling Release() in DeallocPFTPChannel, calling Send__delete__ in OnStopRequest might make sense.  I don't have a complete understanding of how/why the memory management for FTP channels differs from that of HTTP, however.  Now, on with the show!

>diff --git a/netwerk/protocol/ftp/src/FTPBaseChannel.cpp b/netwerk/protocol/ftp/src/FTPBaseChannel.cpp

>+FTPBaseChannel::FTPBaseChannel(nsIURI *uri, nsIProxyInfo *pi)

Nit: snuggle pointers against the type, please.

>+nsresult
>+FTPBaseChannel::Init()
>+{
>+  nsresult rv = nsHashPropertyBag::Init();
>+  if (NS_FAILED(rv)) return rv;
>+
>+  return NS_OK;
>+}

This looks like it could just become |return nsHashPropertyBag::Init()|, right?

>+NS_IMPL_ISUPPORTS4(FTPBaseChannel,
>+                   nsIChannel,
>+                   nsIInterfaceRequestor,
>+                   nsITransportEventSink,
>+                   nsIFTPChannel);

This doesn't seem to match with the header, which uses |NS_DECL_ISUPPORTS_INHERITED|.

>diff --git a/netwerk/protocol/ftp/src/FTPChannelChild.cpp b/netwerk/protocol/ftp/src/FTPChannelChild.cpp
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.

Nit: 2010

>+  NS_ENSURE_TRUE(gNeckoChild != nsnull, NS_ERROR_FAILURE);

Nit: style guide says don't use |!= nsnull|

>+  // TODO: Combine constructor and AsyncOpen to save one IPC msg
>+  gNeckoChild->SendPFTPChannelConstructor(this);

It might be nice to fix this TODO right now.

>+  //SetStreamListener(listener);

Either uncomment or delete this line, please.

>+  // TODO: serialize mConnectionInfo across to the parent, and set it on
>+  // the new channel somehow?
>+
>+  // TODO: serialize mCaps across to the parent, and set it on
>+  // the new channel somehow?
>+
>+  // TODO: need to notify (child-side) http-on-modify-req observers
>+
>+  // TODO: add self to loadgroup?

Are all of these actually necessary for ftp?  At the very least, fix up the reference to http in there.

>+//-----------------------------------------------------------------------------
>+// FTPChannelChild::PFTPChannelChild
>+//-----------------------------------------------------------------------------

This header doesn't match up with the first two functions that follow.

>+NS_IMETHODIMP
>+FTPChannelChild::IsPending(PRBool *result)
>+{
>+  *result = mIsPending; // Differently than nsBaseChannel.
>+  return NS_OK;
>+}

This comment doesn't actually tell me anything.

>+NS_IMETHODIMP
>+FTPChannelChild::OpenContentStream(PRBool async,
>+                                   nsIInputStream **stream,
>+                                   nsIChannel** channel)
>+{
>+  NS_RUNTIMEABORT("FTPChannel*Child* should never have OpenContentStream called!");
>+  return NS_OK;
>+}

Nit: snuggle *s with the type.

>+bool
>+FTPChannelChild::RecvOnStartRequest(const PRInt32& aContentLength)
>+{
>...
>+  if (!NS_SUCCEEDED(rv)) {

Nit: NS_FAILED

>+bool
>+FTPChannelChild::RecvOnDataAvailable(const nsCString& data,
>+                                     const PRUint32& offset,
>+                                     const PRUint32& count)
>+{
>...
>+  if (!NS_SUCCEEDED(rv)) {

Nit: NS_FAILED

>+  rv = mFTPListener->OnDataAvailable(this, mListenerContext,
>+                                         stringStream, offset, count);

Nit: align indent.

>+  if (!NS_SUCCEEDED(rv)) {

Nit: NS_FAILED

>+bool
>+FTPChannelChild::RecvOnStopRequest(const nsresult& statusCode)
>+{
>...
>+  mStatus = statusCode;

I don't see mStatus being used anywhere else in this patch than here and FTPChannelBase::FTPChannelBase. Remove it if that's the case?

>+  mFTPListener = 0;
>+  mListenerContext = 0;

Nit: s/0/nsnull/

>+  if (!NS_SUCCEEDED(rv)) {

Nit: NS_FAILED

>+}} // mozilla::net

Nit: I believe we're discouraging this.  Separate lines, with |// namespace X| please.

>diff --git a/netwerk/protocol/ftp/src/FTPChannelChild.h b/netwerk/protocol/ftp/src/FTPChannelChild.h
>+ * Portions created by the Initial Developer are Copyright (C) 2009

Nit: 2010

>+  FTPChannelChild(nsIURI *uri, nsIProxyInfo *pi) : FTPBaseChannel(uri, pi)

Nit: *s with types.

>+  //! Sends a remote call using e10s to a parent process

I'm a bit on the fence about this comment.  Is it really necessary?

>+  NS_IMETHOD AsyncOpen(::nsIStreamListener *listener, nsISupports *aContext);
>+
>+  NS_IMETHOD IsPending(PRBool *result);
>+
>+  NS_IMETHOD OpenContentStream(PRBool async,
>+                               nsIInputStream **stream,
>+                               nsIChannel** channel);

Nit: *s with types.

>diff --git a/netwerk/protocol/ftp/src/FTPChannelParent.cpp b/netwerk/protocol/ftp/src/FTPChannelParent.cpp

>+ * Portions created by the Initial Developer are Copyright (C) 2009

Nit: 2010

>+// C++ file contents

Nit: Ewww.  Remove this please.

>+FTPChannelParent::FTPChannelParent()
>+ : mIPCClosed(false)

Nit: Indentation.

>+//-----------------------------------------------------------------------------
>+// FTPChannelParent::nsIStreamListener
>+//-----------------------------------------------------------------------------
>+
>+NS_IMETHODIMP
>+FTPChannelParent::OnDataAvailable(nsIRequest *aRequest,
>+                                  nsISupports *aContext,
>+                                  nsIInputStream *aInputStream,
>+                                  PRUint32 aOffset,
>+                                  PRUint32 aCount)
>+{
>...
>+  char * p = data.BeginWriting();

Nit: *s with type.

>+  if (!NS_SUCCEEDED(rv) || bytesRead != aCount) {

Nit: NS_FAILED

>+NS_IMETHODIMP
>+FTPChannelParent::GetInterface(const nsIID& uuid, void **result)

Nit: *s with type.

>+
>+
>+}} // mozilla::net

Nit: Nix the extra newline, separate lines for the namespace closing braces.

>diff --git a/netwerk/protocol/ftp/src/FTPChannelParent.h b/netwerk/protocol/ftp/src/FTPChannelParent.h
>+ * Portions created by the Initial Developer are Copyright (C) 2009

Nit: 2010

>diff --git a/netwerk/protocol/ftp/src/Makefile.in b/netwerk/protocol/ftp/src/Makefile.in

>+EXPORTS_mozilla/net += \
>+		FTPChannelParent.h \
>+		FTPChannelChild.h  \
>+  $(NULL)

Nit: Indentation.

>diff --git a/netwerk/protocol/ftp/src/PFTPChannel.ipdl b/netwerk/protocol/ftp/src/PFTPChannel.ipdl
>+ * Portions created by the Initial Developer are Copyright (C) 2009

Nit: 2010

>+//-------------------------------------------------------------------

Nit: Unnecessary.

>+
>+
>+} // namespace net
>+} // namespace mozilla

Nit: Nix the extra newline.

>diff --git a/netwerk/protocol/ftp/src/ipdl.mk b/netwerk/protocol/ftp/src/ipdl.mk

>+IPDLSRCS =          \
>+  PFTPChannel.ipdl \
>+  $(NULL)

Nit: make the \s line up, please.

>diff --git a/netwerk/protocol/ftp/src/nsFTP.cpp b/netwerk/protocol/ftp/src/nsFTP.cpp
>+/* -*- Mode: C++; tab-width: 4; indent-tabs-mode: nil; c-basic-offset: 4 -*- */
>+/* vim:set ts=4 sw=4 sts=4 et cin: */

tab-width: 8; c-basic-offset: 2
ts=8 sw=2 sts=2

>diff --git a/netwerk/protocol/ftp/src/nsFTP.h b/netwerk/protocol/ftp/src/nsFTP.h
>+ * Portions created by the Initial Developer are Copyright (C) 2001

Nit: 2010

>+#ifndef nsFTP_h__
>+#define nsFTP_h__
>...
>+#endif // nsFTP_h__

Nit: no trailing underscores

>diff --git a/netwerk/protocol/ftp/src/nsFTPChannel.h b/netwerk/protocol/ftp/src/nsFTPChannel.h
>+#include "FTPBaseChannel.h"
>+
>+#include "FTPBaseChannel.h"
>+

Let's avoid such obvious duplicate inclusion.

>+    nsFtpChannel(nsIURI *uri, nsIProxyInfo *pi) : FTPBaseChannel(uri, pi)

Nit: *s with types.

>diff --git a/netwerk/protocol/ftp/src/nsFtpProtocolHandler.cpp b/netwerk/protocol/ftp/src/nsFtpProtocolHandler.cpp
>+#ifdef MOZ_IPC
>+#include "mozilla/net/NeckoChild.h"
> #endif
>-#define LOG(args) PR_LOG(gFTPLog, PR_LOG_DEBUG, args)
>+
>+using namespace mozilla::net;
>+#ifdef MOZ_IPC
>+#include "mozilla/net/FTPChannelChild.h"
>+#endif

Let's combine these two MOZ_IPC blocks.

>diff --git a/netwerk/test/unit/test_simple_ftp.js b/netwerk/test/unit/test_simple_ftp.js
>+//  Simple FTP test with remote server
>+//
>+//  In objdir/netwerk/test
>+//   In child: export SOLO_FILE=test_simple_wrap_ftp.js; make check-one
>+//   In parent: export SOLO_FILE=_tests/xpcshell/test_necko/unit/test_simple_ftp.js; make check-one

Unnecessary instructions.

>+var body = /files are in \/pub\/mozilla.org/; // text appearing in that file on that server

Is there any reason this needs to be a regex?

>+var dbg=1;
>+if (dbg) { print("============== START =========="); }

Let's scrap this variable and all the debug output lines.  If you really want to keep some in, at least prune them sensibly.

>+function setupChannel(path) {
>+  if (dbg) { print("============== setupChannel: in"); }
>+  var ios = Components.classes["@mozilla.org/network/io-service;1"].
>+                       getService(Components.interfaces.nsIIOService);

Nit: I believe the style is . on the next line.

>+  var ftpchan = chan.QueryInterface(Components.interfaces.nsIFTPChannel);

This line seems to be useless.

>+  do_check_eq(true, data.search(body) != -1);

do_check_true, please.

>diff --git a/netwerk/test/unit_ipc/test_simple_wrap_ftp.js b/netwerk/test/unit_ipc/test_simple_wrap_ftp.js
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/test/unit_ipc/test_simple_wrap_ftp.js
>@@ -0,0 +1,14 @@
>+//
>+// Run test script in content process instead of chrome (xpcshell's default)
>+//
>+// Test with e.g.
>+//    cd $objdir/netwerk/test 
>+//    export SOLO_FILE=test_simple_wrap_ftp.js NECKO_E10S_HTTP=1 ; make check-one
>+// Options: check-interactive, MOZ_IPC_MESSAGE_LOG=1, MOZ_DEBUG_CHILD_PROCESS=1
>+//    export SOLO_FILE=test_simple_wrap_ftp.js NECKO_E10S_HTTP=1 MOZ_IPC_MESSAGE_LOG=1 ; make check-one
>+//    export SOLO_FILE=test_simple_wrap_ftp.js NECKO_E10S_HTTP=1 MOZ_IPC_MESSAGE_LOG=1 MOZ_DEBUG_CHILD_PROCESS=1 ; make check-interactive

Scrap these comments; they're already out of date with regards with the E10S_HTTP var and this stuff belongs in proper MDC docs.

Comment 16

[Alon Zakai (:azakai)]

Attachment: Fixed&rebased patch

jdm: Thanks for the detailed comments!

Attached is a fixed patch, which is also updated to the current trunk.

I set __delete__ to be called like the HttpChannel currently does, basically it should be entirely parallel.

Comment 17

[Jason Duell]

I personally prefer to "snuggle" *'s with the variable.  The necko code generally agrees with me.  Do we have rules on this?  I don't see any in

  https://developer.mozilla.org/en/Mozilla_Coding_Style_Guide

Also, the style guide just says not to use "int *p = NULL;".  Either nsnull or 0 are fine.  I like 0 better because it's one less mozilla-ism, and thus reads like standard C++.

Comment 18

[Josh Matthews [:jdm]]

From the guide:

"Declarations
Canonical C++ style favors T* p for a typename T and a declarator name p. This style requires programmer discipline to avoid T* p, q; where T* p; T* q; was intended. For new C++ code, this is the best style to use."

It's best to stick with the surrounding style; I'm not really picky, but be consistent please.

Also, the new patch is still not quite right - the comment above the AddRef indicates that DeallocPFTPChannel will Release the actor (which is wrong), and there is still no corresponding Release anywhere.

Comment 19

[Alon Zakai (:azakai)]

Attachment: Fixed version with proper AddRef()/Release()

Yup, was missing a refcounting command... fixed in this version.

Comment 20

[Josh Matthews [:jdm]]

Please note the IPDL refcounting changes for HttpChannel in bug 572980 for whenever this patch is updated next.

Comment 22

[Jason Duell]

Bug 572980 explains why we made the changes we did in bug 572980.  As jdm says, FTP will need the same fixes.  Alon, please update when you get a chance (not urgent).

Comment 23

[Doug Turner (:dougt)]

Comment on attachment 450524 [details] [diff] [review]
Fixed version with proper AddRef()/Release()

+
+FTPBaseChannel::FTPBaseChannel(nsIURI* uri, nsIProxyInfo* pi)
+  : mProxyInfo(pi)
+  , mStartPos(0)
+  , mResumeRequested(PR_FALSE)
+  , mLastModifiedTime(0)
+  , mFTPListener(0)
+  , mIsPending(PR_FALSE)
+  , mWasOpened(PR_FALSE)

You want to order these in the same order as they are declared in the header to avoid warnings.

Also, mFTPListener doesn't need to be there -- it is a com pointer.

+NS_IMPL_RELEASE_WITH_DESTROY(FTPChannelChild, RefcountHitZero())

Do you really have to do this?  What does the parent really need -- surely not a reference to this child object.


Maybe the refcount fixes will address this.


In ODA, 

+    // TODO:  what to do here?  Cancel request?  Very unlikely to fail.


Is there a follow up bug?  is the http cancel bug addressing this?


+  rv = mFTPListener->OnDataAvailable(this, mListenerContext,
+                                         stringStream, offset, count);


Line things up.


It really sucks that we are just passing the ftp data over as a string.  That is a lot of data if you are, say, downloading a file.  Will 564553 address this?


In the 3 on* methods, we check to see what the result of mFTPListener is, yet do nothing with the result.  Lets just cast the result to void:

(void)mFTPListener->On*

I do not think we should pass back a false to the IPC code.

What are the enum FTPChannelChildState for?  I can not tell from reading the patch.


+bool
+FTPChannelParent::RecvAsyncOpen(const IPC::URI& aURI)
+{
+  nsCOMPtr<nsIURI> uri = aURI;
+
+  nsCString uriSpec;
+  uri->GetSpec(uriSpec);
+  LOG(("FTPChannelParent RecvAsyncOpen [this=%x uri=%s]\n",
+       this, uriSpec.get()));

#ifdef DEBUG

No need to get the spec just to log it in normal non-debug builds


+  nsCOMPtr<nsIIOService> ios(do_GetIOService(&rv));
+  if (NS_FAILED(rv))
+    return false;       // TODO: send fail msg to child, return true


If ios fails, we are fucked.  I do not think we need to check the result.  jduell?


+  nsFtpChannel* ftpChan = static_cast<nsFtpChannel*>(chan.get());

This is only true if the scheme is FTP and someone didn't override the protocol, right?  QI!

These comments "// TODO: send fail msg to child, return true" make me nervous.  if AsyncOpen fails for some reason, the child process will never know about it right now, correct?


I totally hate FTPChannelParent::OnDataAvailable().  Reading the date just to serialize it over IPC makes me cry.

Why do we need nsFTP.cpp|h just to declare gFTPLog?  let move that to some common existing file.

I am completely frightened about the ownership model here.  Might be helpful to write up something about what owns what during a simple network load.

Getting close, but r- for now.  Love to see a new patch!

Comment 24

[Jason Duell]

> +NS_IMPL_RELEASE_WITH_DESTROY(FTPChannelChild, RefcountHitZero())

Yes, this goes away once FTP follow the new HTTP refcount logic.

> It really sucks that we are just passing the ftp data over as a string.  That
> is a lot of data if you are, say, downloading a file.  Will 564553
> address this?  

The download mgr will be taking the channel if we download a file.  This is just for directory browsing.  We should wait until bug 562444 is done and then copy whatever the HTTP channel needs to do to the FTP channel.

> If ios fails, I do not think we need to check the result. jduell?

Fine with me.

> +  nsFtpChannel* ftpChan = static_cast<nsFtpChannel*>(chan.get());
> 
> This is only true if the scheme is FTP and someone didn't override the
> protocol, right?  QI!

We officially don't care about allowing anyone to override HTTP/FTP channel impls.  So as long as we know this is FTP (and if it's PFTPProtocol, we do), we're fine casting to nsFtpChannel.  We do this all over the place in the HTTP channel now.

> These comments "// TODO: send fail msg to child, return true" make me nervous. 
> if AsyncOpen fails for some reason, the child process will never know about it
> right now, correct?

Part of HTTP cancel bug.  We'll copy the logic once we've nailed it down for HTTP.

> I am completely frightened about the ownership model here.  Might be helpful
> to write up something about what owns what during a simple network load.

Don't be frightened, little bear!  Things are cleaner now with the new HTTP ownership model.

Comment 25

[Doug Turner (:dougt)]

> Don't be frightened, little bear!  Things are cleaner
> now with the new HTTP ownership model.

I have scars from chasing memory leak bugs.  Give me a base class so that we can put all of the problems in one pile (or on one plate (yours preferably!))

Comment 26

[Alon Zakai (:azakai)]

Attachment: Patch v5

Rebased/fixed version, following the comments here. All except for:

> Why do we need nsFTP.cpp|h just to declare gFTPLog?  let move that to some common existing file.

I wasn't sure what to do about this - which existing file should I use?

Comment 27

[Josh Matthews [:jdm]]

>+bool
>+FTPChannelChild::RecvOnStopRequest(const nsresult& statusCode)

>+  // Corresponding AddRef in AsyncOpen().
>+  this->Release();

This is incorrect; you should be calling Send__delete__() or the protocol will never die.

Comment 28

[Alon Zakai (:azakai)]

Attachment: Patch v6

(In reply to comment #27)
> >+bool
> >+FTPChannelChild::RecvOnStopRequest(const nsresult& statusCode)
> 
> >+  // Corresponding AddRef in AsyncOpen().
> >+  this->Release();
> 
> This is incorrect; you should be calling Send__delete__() or the protocol will
> never die.

Ok, fixed in this patch.

Comment 29

[Jason Duell]

Sorry to keep the goalpost moving.  We'll need to add the buffering that HTTP added in bug 559200 to FTP as well.

We could do this in a a followup bug, or in this one.

Comment 30

[Alon Zakai (:azakai)]

I support leaving this for a followup bug - otherwise this bug will never end, as new HTTP improvements are continuously added as requirements.

(Also, it would be nice to find a way to share the buffering code somehow, so every protocol doesn't end up having to reimplement it, and that would also be outside the scope of this bug.)

Comment 31

[Alon Zakai (:azakai)]

Attachment: Patch v7

De-bitrotting for this high maintenance patch.

Comment 32

[Doug Turner (:dougt)]

Comment on attachment 464593 [details] [diff] [review]
Patch v7

i understand that this bit rotted *again*.

Comment 33

[Alon Zakai (:azakai)]

Comment on attachment 464593 [details] [diff] [review]
Patch v7

> i understand that this bit rotted *again*.

That was my mistake, I saw scary changes to the code around it and thought so. But a closer look showed that wasn't the case (those changes, like HTTP redirect stuff, aren't relevant to FTP).

The patch needs a tiny amount of fuzz and such, but can be reviewed as is.

Comment 34

[Jason Duell]

Giving to jdm, to make 1) memory lifetime match HTTP if it doesn't yet (I assume there's no need to have dual model with longer life for document loads, since no NSS),  2) IPDL buffering, and 3) integration with the download mgr (ask bcrowder if you have trouble).

Comment 35

[Jason Duell]

Comment on attachment 464593 [details] [diff] [review]
Patch v7

clearing review--expecting new patch from jdm.

Comment 36

[Josh Matthews [:jdm]]

Attachment: Make FTP work for fennec

Lifetime is correct, downloads work, buffering is in place and you can now even browse ftp pages with my changes! Took it for a spin around ftp.mozilla.org; everything seems to be in order.

Comment 37

[Josh Matthews [:jdm]]

The important changes that I made were:

* the addition of a contentType parameter to the OnStartRequest message which sets the child channel's content type
* AsyncOpen is now ironically a synchronous ipdl message.  It makes error handling really easy.
* message queueing

Comment 38

[Josh Matthews [:jdm]]

Attachment: Implement remote FTP

I'm still unsure why the DataAvailableEvent mixup occurs, but I don't get any problems with the FTP ones renamed to be unique.

Comment 39

[Doug Turner (:dougt)]

want in b2 because we want 598076.

Comment 40

[Jason Duell]

Comment on attachment 479866 [details] [diff] [review]
Implement remote FTP

I think this might still be able to land in time for fennec 2.0b2, but it needs more work than I expected.

I haven't had time to figure out all the things that may be wrong with this patch, but there seem to be a lot:

1) AFAICT we're passing an empty URI to the parent during AsyncOpen. I'm not sure how this code works at all with that.

2) We should make AsyncOpen async, unless someone wiser than me (cjones?  bsmedberf?) thinks it's actually ok to be sync.  

3) We're missing a fair amount of error handling

4) The inheritance model is sloppy at best.   With HTTP, we split out all the common logic for parent/child into HttpBaseChannel.  For FTP, we've already got nsBaseChannel, which was designed more to abstract channels for the socket transport thread.  This patch adds another base class, FTPBaseChannel, which inherits from nsBaseChannel.  That slurps up more than is ideal (FTPChannelChild doesn't need a lot of the nsBaseChannel implementation), though I suppose we might be able to live with that.  But I'm noticing that many if not most/all of the new items in FTPChannelChild seems to actually not be shared between the parent/child, and instead belong in one or the other class.  So FTPBaseChannel may deserve to die.  I don't have enough time to go over it right now to say for sure.

> I totally hate FTPChannelParent::OnDataAvailable()...  It really sucks that we
> are just passing the ftp data over as a string.  That is a lot of data if you
> are, say, downloading a file.  Will 564553 address this?

This is how Http is doing it too right now.  We may be able to optimize a bit with shared memory at some point.  Definitely a followup.


> AsyncOpen is now ironically a synchronous ipdl message.  It makes error
> handling really easy.

No, not ironic:  Bad.  My understanding is that the content process' main thread will block for the IPDL round trip if sync.  I'm not sure what that would affect in this case, but it just seems like a bad idea if we can do things async, which we can.  The error handling doesn't look that bad--just use the same SendCancelEarly() logic that the HttpChannel uses. (If you can get cjones or bsmedberg to agree that this is ok as sync, it can stay sync.)

>diff --git a/netwerk/ipc/NeckoCommon.h b/netwerk/ipc/NeckoCommon.h
> 
>+#define ENSURE_CALLED_BEFORE_ASYNC_OPEN()                                      \
>+  if (mIsPending || mWasOpened) {                                              \
>+    nsPrintfCString msg(1000,                                                  \
>+                        "*&*&*&*&*&*&*&**&*&&*& FATAL ERROR: '%s' "            \
>+                        "called after AsyncOpen: %s +%d",                      \
>+                        __FUNCTION__, __FILE__, __LINE__);                     \
>+    NECKO_MAYBE_ABORT(msg);                                                    \

Now that these errors are only sometimes fatal, we should change this message.  The functions that NECKO_MAYBE_ABORT calls already add msg prefixes, so let's just nuke everything up to '%s' in the format string.

Please s/FATAL NECKO ERROR/NECKO ERROR/ in DROP_DEAD, too.


>diff --git a/netwerk/protocol/ftp/FTPBaseChannel.h b/netwerk/protocol/ftp/FTPBaseChannel.h
>+class FTPBaseChannel : public nsIFTPChannel
>+                     , public nsBaseChannel
>+{
>+public:
>+
>+  NS_IMETHODIMP GetLastModifiedTime(PRTime* lastModifiedTime) {
>+    *lastModifiedTime = mLastModifiedTime;
>+    return NS_OK;
>+  }
>+
>+  NS_IMETHODIMP SetLastModifiedTime(PRTime lastModifiedTime) {
>+    mLastModifiedTime = lastModifiedTime;
>+    return NS_OK;
>+  }

Is it kosher to declare XPCOM functions as inline?  I suppose they might get optimized for some C++ callers, but they're going to need to exist as regular functions for JS, etc.   Ask an XPCOM guru and make sure this can't cause any mischief.  And if it turns out that these can never actually get inlined, move them to the .cpp file.

>+protected:
>+  nsCOMPtr< ::nsIStreamListener >   mFTPListener;
>+  nsCOMPtr<nsISupports>             mListenerContext;

Are the extra spaces and global :: needed for nsIStreamListener, or are you just being dramatic?   :)

Why are we creating mFTPListener here, when nsBaseChannel already has mListener?

It looks like some of the other members created here--mIsPending/WasOpened are needed only by FTPChannelChild, while the others are nsFtpChannel-only.  Move them out of this class and see if there's actually any reason to have this class.


>diff --git a/netwerk/protocol/ftp/FTPChannelChild.cpp b/netwerk/protocol/ftp/FTPChannelChild.cpp
+
>+NS_IMETHODIMP
>+FTPChannelChild::AsyncOpen(::nsIStreamListener* listener, nsISupports* aContext)
>+{
>+  LOG(("FTPChannelChild::AsyncOpen [this=%x]\n", this));
>+
>+  NS_ENSURE_TRUE((gNeckoChild), NS_ERROR_FAILURE);
>+  NS_ENSURE_ARG_POINTER(listener);
>+  NS_ENSURE_TRUE(!mIsPending, NS_ERROR_IN_PROGRESS);
>+  NS_ENSURE_TRUE(!mWasOpened, NS_ERROR_ALREADY_OPENED);
>+
>+  // Port checked in parent, but duplicate here so we can return with error
>+  // immediately, as we've done since before e10s.
>+  nsresult rv;
>+  rv = NS_CheckPortSafety(nsBaseChannel::URI()); // Need to disambiguate,
>+                                                 // because in the child ipdl,
>+                                                 // a typedef URI is defined...

Why are we passing in a new, empty URI here instead of mURI?  This looks very wrong to me.  nsBaseChannel and nsHttpChannel and HttpChannelChild all use mURI here.  There doesn't seem to be any point in checking an empty URI.

>+  if (NS_FAILED(rv))
>+    return rv;
>+
>+  // FIXME: like bug 558623, merge constructor+SendAsyncOpen into 1 IPC msg
>+  gNeckoChild->SendPFTPChannelConstructor(this);
>+  mFTPListener = listener;
>+  mListenerContext = aContext;
>+
>+  // add ourselves to the load group. 
>+  if (mLoadGroup)
>+    mLoadGroup->AddRequest(this, nsnull);
>+
>+  SendAsyncOpen(IPC::URI(nsBaseChannel::URI()), &rv);

Now I'm really confused.  We're passing an empty, new URI to AsyncOpen, too?  And you've tested this code and it works?  What am I missing here?

>+void
>+FTPChannelChild::DoOnStartRequest(const PRInt32& aContentLength,
>+                                  const nsCString& aContentType)
>+{
>+  LOG(("FTPChannelChild::RecvOnStartRequest [this=%x]\n", this));
>+
>+  SetContentLength(aContentLength);
>+  SetContentType(aContentType);
>+  (void)mFTPListener->OnStartRequest(this, mListenerContext);

We should be checking the return code from OnStartRequest and Canceling the channel if it failed.   Same for OnDataAvailable (but not onStop--we ignore the return val from that).

>diff --git a/netwerk/protocol/ftp/FTPChannelChild.h b/netwerk/protocol/ftp/FTPChannelChild.h
>+  FTPChannelChild(nsIURI* uri, nsIProxyInfo* pi) : FTPBaseChannel(uri, pi),
>+                                                   mIPCOpen(false)
>+  { }
>+  virtual ~FTPChannelChild();

Add LOG msg in constructor (you're already call in it the destructor).

>diff --git a/netwerk/protocol/ftp/FTPChannelParent.cpp b/netwerk/protocol/ftp/FTPChannelParent.cpp

>+bool
>+FTPChannelParent::RecvAsyncOpen(const IPC::URI& aURI, nsresult* rv)
>+{
>+  nsCOMPtr<nsIURI> uri(aURI);
>+
>+#ifdef DEBUG
>+  nsCString uriSpec;
>+  uri->GetSpec(uriSpec);
>+  LOG(("FTPChannelParent RecvAsyncOpen [this=%x uri=%s]\n",
>+       this, uriSpec.get()));
>+#endif
>+
>+  nsCOMPtr<nsIIOService> ios(do_GetIOService(rv));
>+  if (NS_FAILED(*rv))
>+    return true;
>+
>+  nsCOMPtr<nsIChannel> chan;
>+  *rv = NS_NewChannel(getter_AddRefs(chan), uri, ios);
>+  if (NS_FAILED(*rv))
>+    return true;
>+
>+  nsFtpChannel* ftpChan = static_cast<nsFtpChannel*>(chan.get());
>+
>+  *rv = ftpChan->AsyncOpen(this, nsnull);
>+  return true;

Change all error cases to SendCancelEarly.

>+NS_IMETHODIMP
>+FTPChannelParent::OnDataAvailable(nsIRequest* aRequest,
>+                                  nsISupports* aContext,
>+                                  nsIInputStream* aInputStream,
>+                                  PRUint32 aOffset,
>+                                  PRUint32 aCount)
>+{
>+  nsCString data;
>+  data.SetLength(aCount);
>+  char* p = data.BeginWriting();
>+  PRUint32 bytesRead;
>+  rv = aInputStream->Read(p, aCount, &bytesRead);
>+  data.EndWriting();
>+  if (NS_FAILED(rv) || bytesRead != aCount) {
>+    return rv;              // TODO: figure out error handling
>+  }

Replace the stream copy logic with NS_ReadInputStreamToString(), as in HttpChannelParent.

>diff --git a/netwerk/protocol/ftp/PFTPChannel.ipdl b/netwerk/protocol/ftp/PFTPChannel.ipdl
>+  sync AsyncOpen(URI uri) returns (nsresult rv);

async

>diff --git a/netwerk/protocol/ftp/nsFTP.h b/netwerk/protocol/ftp/nsFTP.h

>+#ifdef MOZ_IPC
>+// e10s mess: See bug 545995
>+#if defined(PR_LOG) && !defined(ALLOW_LATE_NSHTTP_H_INCLUDE)
>+#error "If nsFTP.h #included it must come before any IPDL-generated files or other files that #include prlog.h"
>+#endif
>+#include "mozilla/net/NeckoChild.h"
>+#undef LOG
>+#endif // MOZ_IPC

Hmm, looks like we ought to rename ALLOW_LATE_NSHTTP_H_INCLUDE something less HTTP-specific.  Can't think of a great name.  Maybe ALLOW_LATE_PROTO_H_INCLUDE?

>diff --git a/netwerk/protocol/ftp/nsFTPChannel.h b/netwerk/protocol/ftp/nsFTPChannel.h

> private:
>-    nsCOMPtr<nsIProxyInfo>    mProxyInfo; 

Why is mProxyInfo moved to FTPBaseChannel?  It looks like only nsFTPChannel uses it, so it should probably stay here.


>     nsCOMPtr<nsIFTPEventSink> mFTPEventSink;
>     nsCOMPtr<nsIInputStream>  mUploadStream;
>-    PRUint64                  mStartPos;
>     nsCString                 mEntityID;
>-    PRPackedBool              mResumeRequested;
>-    PRTime                    mLastModifiedTime;

Ditto for mStartPos, mResumeRequested, and mLastModifiedTime.

>diff --git a/netwerk/protocol/ftp/nsFtpProtocolHandler.cpp b/netwerk/protocol/ftp/nsFtpProtocolHandler.cpp

>+#ifdef MOZ_IPC
>+    if (IsNeckoChild()) {
>+        LOG(("NECKO_E10S_HTTP set: using experimental interprocess FTP\n"));
>+        channel = new FTPChannelChild(uri, proxyInfo);
>+    } else

It's not experimental--It's production-ready! :)  Scrap LOG msg (we don't have one in this place for HTTP, and following log msgs will make clear which we created).

>diff --git a/netwerk/test/unit/test_simple_ftp.js b/netwerk/test/unit/test_simple_ftp.js
>new file mode 100644
>--- /dev/null
>+++ b/netwerk/test/unit/test_simple_ftp.js
>@@ -0,0 +1,41 @@
>+//
>+//  Simple FTP test with remote server
>+//
>+
>+var server = 'ftp.mozilla.org';

We're going to have to clear things with the infrastructure folks (ask ted) to see if it's ok to have an xpcshell test rely on an external server, even one (presumably) as reliable as the mozilla ftp server.

Haven't had time to look over the test logic.

Comment 41

[Josh Matthews [:jdm]]

Excellent points all around; thanks.  I don't have the answers to most of your questions yet, and I should probably make a habit of reading more of the changes from patches that I inherit.  One note about the blank URI thing - nsBaseChannel::URI() returns the base channel URI which is set by the BaseFTPChannel constructor, so there's nothing magical happening here.

Comment 42

[Josh Matthews [:jdm]]

Attachment: Make FTP work for fennec

Lots and lots of changes made. Just saving my work; this is nearly the finished patch.

Comment 43

[Josh Matthews [:jdm]]

Attachment: Part 1: Move IPC::InputStream to a more general location.

Comment 44

[Josh Matthews [:jdm]]

Attachment: Part 2: Make FTP work for fennec.

Comment 45

[Josh Matthews [:jdm]]

Attachment: Part 3: Tests.

Comment 46

[Josh Matthews [:jdm]]

Attachment: Part 1.5 - Generalize IPDL event queue some more.

Comment 47

[Josh Matthews [:jdm]]

Attachment: Part 2: Make FTP work for fennec.

Comment 48

[Josh Matthews [:jdm]]

Comment on attachment 483488 [details] [diff] [review]
Part 2: Make FTP work for fennec.

This is as ready for review as it'll ever be.  Downloads work flawlessly, browsing ftp.mozilla.org is a breeze, and the various bizarre inheritance choices of before are history.  I also sidestepped the nsFTP.h thing, and the patch in general is much more pleasant to read.

Comment 49

[Josh Matthews [:jdm]]

Comment on attachment 483479 [details] [diff] [review]
Part 3: Tests.

Ted, what's the story on tests using servers like ftp.mozilla.org?  I'm not all that keen to write a JS impl of an ftp server :/

Comment 50

[(not currently active) Ted Mielczarek]

Comment on attachment 483479 [details] [diff] [review]
Part 3: Tests.

Don't do it. They'll fail randomly depending on network conditions. I agree that writing ftpd.js kind of sucks, but you don't have to implement a full-featured server or anything. You can implement the bare minimum required to test our implementation and land it. Someone else can flesh it out if they need to test more things later.

Comment 51

[Josh Matthews [:jdm]]

Comment on attachment 483487 [details] [diff] [review]
Part 1.5 - Generalize IPDL event queue some more.

Nevermind, I ran the IPDL changes through try and test_httpsuspend_wrap.js failed on every single box.  It's unfortunate that I'm developing on a platform that can't run ipc xpcshell tests :(

Comment 52

[Josh Matthews [:jdm]]

Attachment: Part 1.5 - Generalize IPDL event queue some more. [checked in, comment 60]

Comment 53

[Josh Matthews [:jdm]]

Comment on attachment 483722 [details] [diff] [review]
Part 1.5 - Generalize IPDL event queue some more. [checked in, comment 60]

Now passing tests.

Comment 54

[Jason Duell]

Comment on attachment 483488 [details] [diff] [review]
Part 2: Make FTP work for fennec.

Looks good.  A few minor-to-moderate code changes needed.  +r with those.

>+++ b/netwerk/protocol/ftp/FTPChannelChild.cpp

>+NS_IMETHODIMP
>+FTPChannelChild::IsPending(PRBool* result)
>+{
>+  *result = mIsPending && mIPCOpen;
>+  return NS_OK;
>+}

I don't see why you need the mIPCOpen check--seems like all paths to mIPCOpen = false set mIsPending = false first.  But I'm groggy, so may have missed something, and I guess it does not harm to check.

>+  nsresult rv = NS_NewByteInputStream(getter_AddRefs(stringStream),
>+                                      data.get(),
>+                                      count,
>+                                      NS_ASSIGNMENT_DEPEND);

You capture but don't check rv here--Cancel(rv) if NS_FAILED.

>diff --git a/netwerk/protocol/ftp/FTPChannelChild.h b/netwerk/protocol/ftp/FTPChannelChild.h

>+class FTPChannelChild : public PFTPChannelChild
>+                      , public nsBaseChannel


Meh.  nsBaseChannel is still a fat class to inherit from, and we don't use a lot of it.  But hey, it works, and trying to further slice the inheritance hierarchy is probably not worth it.  So probably the right call overall, if a little kludgy.


>+  NS_IMETHOD AsyncOpen(::nsIStreamListener* listener, nsISupports* aContext);

So I see that gcc barfs if we get rid of the :: before nsIStreamListener here ("error: ‘class nsIStreamListener’ is inaccessible within this context"), but I have no idea why it needs the global scope operator here, while nsISupports, for instance, does not.  Anyone know? 

>+  NS_OVERRIDE bool IsSuspended();

What does NS_OVERRIDE mean exactly?  The #define merely says that it's to "help DeHydra.  IsSuspended is neither itself an override, nor is overridden, so I'm having trouble grokking what's going on here.   We're not using this anywhere else in necko--if it's useful we should open a bug to spread it to HttpChannelChild and elsewhere.

>+  NS_OVERRIDE bool RecvOnStartRequest(const PRInt32& aContentLength,
>+                                      const nsCString& aContentType,
>+                                      const nsCString& aEntityID,
>+                                      const IPC::URI& aURI);
>+  NS_OVERRIDE bool RecvOnDataAvailable(const nsCString& data,
>+                                       const PRUint32& offset,
>+                                       const PRUint32& count);
>+  NS_OVERRIDE bool RecvOnStopRequest(const nsresult& statusCode);
>+  NS_OVERRIDE bool RecvCancelEarly(const nsresult& statusCode,
>+                                   const bool& triggerOnStartRequest);

Ditto for NS_OVERRIDE uses here.

>+  enum {
>+    NOT_CANCELED,
>+    CANCELED_LOCALLY,
>+    CANCELED_REMOTELY
>+  } mCanceled;

Is there a reason why you're handling cancelling differently than in the HTTP case?  For HTTP, we only use CancelEarly for errors leading up to AsyncOpen.   You are using CancelEarly in FTP for things like failed stream-to-string copy in OnDataAvailable in the parent, while HTTP just calls plain old Cancel (which handles sending OnStart/Stop with the correct error msg to the child automatically).  You are then inserting a bunch of checks for 

 if (mCanceled == CANCELED_REMOTELY)
    return

in most of the IPDL RecvFoo functions.   It looks like if you change OnDataAvailable to just Cancel, you can (like HTTP) keep mCanceled to a simple true/false value, and skip the checks for mCanceled in OnStart/Stop (you'd still need one for OnData).

Oh--I see you're also using CancelEarly purely within the child, too (i.e. not as a direct result of the parent sending a CancelEarly msg, but when a listener returns NS_FAIL from OnStart/Data).  Hrm.  That's a different model than how we do it in HTTP (we just Cancel, which winds up sending a Cancel msg back to the parent).  I hesitate to have two different models, so I'd like to see all this code resemble HTTP's unless there's a good reason why we should do FTP differently.  I don't see one.

One good reason not to do things the way you're doing them is that if your code sees an NS_FAIL from OnStartRequest's listener, you wind up deleting the channel without actually canceling the underlying nsFTPChannel, which will continue to slurp data off the socket, but just throw the data away silently, which is a bad use of bandwidth (would that happen for file downloads?  If so, that would definitely suck).

>+    CANCELED_LOCALLY,

I was going to tell you that it should be "CANCELLED", but after refering [sic] to a dictionary it turns out that the single 'l' form is perfectly correct :)


>diff --git a/netwerk/protocol/ftp/FTPChannelParent.cpp b/netwerk/protocol/ftp/FTPChannelParent.cpp

>+ * Contributor(s):
>+ *   Alon Zakai <[e-mail]>

Why not be famous?  Add your name to all the files you've modified.

>diff --git a/netwerk/protocol/ftp/PFTPChannel.ipdl b/netwerk/protocol/ftp/PFTPChannel.ipdl

>+sync protocol PFTPChannel

If we can make GetLastModifiedTime async, s/sync//

>+parent:
>+  DeleteSelf();

From my quick glance, it looks like all the spots in the child that call this could simply call _send_delete instead?   Doesn't seem to be any reason for sending msg to the parent to have it call delete instead.

>+  sync GetLastModifiedTime() returns (PRTime time);

Make GetLastModifiedTime async.  It appears that the only caller of SetLastModifiedTime is nsFTPConnectionThread, on chrome, and it appears to do so before OnStartRequest (step through to make sure). So we can probably 1) eliminate the SetLastModifiedTime you've added to RecvAsyncOpen (make child calls to SetLastModifiedTime before AsyncOpen fail with NOT_AVAILABLE), 2) send the last mod time to the child in OnStartReq, and 3) either forbid setting SetLastModifiedTime entirely on the child (I'm guessing that'd be fine), or allowing it to be set after AsyncOpen only.  Either way you'd be able to get rid of the GetLastModifiedTime IPDL msg, as GetLastModifiedTime will just be 'return mLastModified" so long as you update it at RecvOnStartReq and if/when SetLastModifiedTime is called.

>diff --git a/netwerk/protocol/ftp/nsFtpProtocolHandler.cpp b/netwerk/protocol/ftp/nsFtpProtocolHandler.cpp

> NS_IMETHODIMP
> nsFtpProtocolHandler::NewProxiedChannel(nsIURI* uri, nsIProxyInfo* proxyInfo,
>                                         nsIChannel* *result)
> {
> 
>+    *result = channel.forget().get();

Hmm, frustrating that the already_AddRefed<T> that forget() returns isn't assignable to to nsIChannel.   But "forget(result)" works, and is both shorter and prettier.  Use that.

Comment 55

[Josh Matthews [:jdm]]

Attachment: Part 2: Make FTP work for fennec.

Comment 56

[Josh Matthews [:jdm]]

Almost all comments addressed as recommended.  Bugzilla interdiff works fine if you'd like to take a gander at them.  The only change I didn't make is in regards to this:

>> NS_IMETHODIMP
>> nsFtpProtocolHandler::NewProxiedChannel(nsIURI* uri, nsIProxyInfo* proxyInfo,
>>                                         nsIChannel* *result)
>> {
>> 
>>+    *result = channel.forget().get();
>
>Hmm, frustrating that the already_AddRefed<T> that forget() returns isn't
>assignable to to nsIChannel.   But "forget(result)" works, and is both shorter
>and prettier.  Use that.

forget(result) doesn't work before channel is an nsRefPtr<nsBaseChannel> while result is an nsIChannel**, so there are errors that can't be resolved without ugly casts.

Comment 57

[Josh Matthews [:jdm]]

Also for historical purposes, the DeleteSelf was necessary because of the way I was using CancelEarly from the child.  I ended up running into a race when cancelling an ftp download because there were still messages in flight.

Also, in regards to NS_OVERRIDE, you're right about IsSuspended being a bogus usage of it.  NS_OVERRIDE annotations are used by the static analysis scripts to ensure that derived implementations are actually overriding base virtual functions, and it wouldn't go amiss to sprinkle their fairy dust over the rest of the e10s netwerk changes as well.

I think I'll file a followup for moving FTPChannelChild away from nsBaseChannel.

Comment 58

[Josh Matthews [:jdm]]

Comment on attachment 484091 [details] [diff] [review]
Part 2: Make FTP work for fennec.

Nothing surprising here.

Comment 59

[Jason Duell]

Comment on attachment 484091 [details] [diff] [review]
Part 2: Make FTP work for fennec.

Looks good.  +r with one fix and couple nits.

> forget(result) doesn't work...  without ugly casts.

huh-gcc likes it fine w/o any cast, but perhaps devstudio doesn't?  Just curious.   

> NS_OVERRIDE annotations are used by the static analysis scripts
> to ensure that derived implementations are actually overriding base virtual
> functions

I thought we used NS_MUST_OVERRIDE to flag functions that need overriding, and the the scripts were smart enough to automatically detect if they're overridden.

> It wouldn't go amiss to sprinkle [NS_OVERRIDE] fairy dust over
> the rest of the e10s netwerk changes as well

Feel free to open a bug for that.  We don't use NS_OVERRIDE anywhere else in necko right now, so there's probably plenty of potential uses for whatever it is that it does.

>diff --git a/netwerk/protocol/ftp/FTPChannelChild.cpp b/netwerk/protocol/ftp/FTPChannelChild.cpp

>+  nsCOMPtr<nsIInputStream> stringStream;
>+  nsresult rv = NS_NewByteInputStream(getter_AddRefs(stringStream),
>+                                      data.get(),
>+                                      count,
>+                                      NS_ASSIGNMENT_DEPEND);
>+  if (NS_FAILED(rv))
>+    Cancel(rv);

You should also return here instead of going on to call OnDataAvail on the listener.

>+void
>+FTPChannelChild::DoOnStopRequest(const nsresult& statusCode)
>+{
>+
>+  Send__delete__(this);

Just to be nice, add boilerplate warning from HTTP above Send__delete__:

    // This calls NeckoChild::DeallocPHttpChannel(), which deletes |this| if IPDL
    // holds the last reference.  Don't rely on |this| existing after here.

>diff --git a/netwerk/protocol/ftp/FTPChannelChild.h b/netwerk/protocol/ftp/FTPChannelChild.h

> I think I'll file a followup for moving FTPChannelChild away from
> nsBaseChannel.

If you like--the payoff isn't huge. 

Meanwhile add comment:

    // This class inherits logic from nsBaseChannel that not needed for an e10s
    // child channel, but it works.  At some point we could slice up
    // nsBaseChannel and have a new class that has only the common logic for
    // nsFTPChannel/FTPChannelChild.

>+class FTPChannelChild : public PFTPChannelChild
>+                      , public nsBaseChannel

Comment 60

[Josh Matthews [:jdm]]

Pushed the IPDL generalization patch so wyciwyg channels could land: http://hg.mozilla.org/mozilla-central/rev/fe519f659d60

Comment 61

[Josh Matthews [:jdm]]

Attachment: Part 2: Make FTP work for fennec.

Comment 62

[Josh Matthews [:jdm]]

Attachment: Part 2: Make FTP work for fennec.

Comment 63

[[e-mail]]

I landed this, but it bounced -- the inputstream patch it depends on had test failures.

Hopefully we can reland tomorrow.

Comment 64

[[e-mail]]

Done.

http://hg.mozilla.org/mozilla-central/rev/f3a6cf176c00
http://hg.mozilla.org/mozilla-central/rev/e63ac78f181e

Comment 65

[Mark Banner (:standard8)]

I pushed a bustage fix for this for non-IPC builds:

http://hg.mozilla.org/mozilla-central/rev/112490ab7b58

(the required macro was moved to an IPC-only header file)