Status

()

defect
RESOLVED FIXED
10 years ago
8 years ago

People

(Reporter: jduell.mcbugs, Assigned: jdm)

Tracking

Other Branch
x86
Windows Server 2003
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(5 attachments, 16 obsolete attachments)

5.66 KB, text/plain
Details
9.70 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
2.00 KB, patch
Details | Diff | Splinter Review
10.73 KB, patch
jduell.mcbugs
: review+
Details | Diff | Splinter Review
52.03 KB, patch
Details | Diff | Splinter Review
Reporter

Description

10 years ago
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.
(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.
Reporter

Updated

9 years ago
Blocks: 558617
Reporter

Updated

9 years ago
No longer blocks: fennecko
Reporter

Comment 2

9 years ago
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.
Blocks: fennecko
No longer blocks: 558617
Summary: Verify that FTP works unchanged for fennec → Make FTP work for fennec
(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.
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.
Reporter

Comment 5

9 years ago
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?
i do not think so.
Assignee: nobody → azakai
Reporter

Comment 7

9 years ago
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.
Posted patch Working patch, early version (obsolete) — Splinter Review
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 on attachment 444431 [details] [diff] [review]
Working patch, early version

jason, can you take a look.
Attachment #444431 - Flags: review?(jduell.mcbugs)
Content length is now reported properly. The hackish workaround for the test harness relying on content length has been removed.
Attachment #444431 - Attachment is obsolete: true
Attachment #444431 - Flags: review?(jduell.mcbugs)
Attachment #444460 - Flags: review?(jduell.mcbugs)
Reporter

Updated

9 years ago
Blocks: 559714
Takes into account bug 565223 - prevent crashes when trying to send data to a crashed child process.
Attachment #444460 - Attachment is obsolete: true
Attachment #445213 - Flags: review?
Attachment #444460 - Flags: review?(jduell.mcbugs)
Attachment #445213 - Flags: review? → review?(jduell.mcbugs)
Attachment #445213 - Flags: review?(jduell.mcbugs) → review?(josh)
Comment on attachment 445213 [details] [diff] [review]
Improved patch to take into account bug 565223

jdm, do you have cycles to review please?
Assignee

Comment 13

9 years ago
Can do.
Reporter

Comment 14

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

Comment 15

9 years ago
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.
Attachment #445213 - Flags: review?(josh) → review-
Posted patch Fixed&rebased patch (obsolete) — Splinter Review
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.
Attachment #445213 - Attachment is obsolete: true
Reporter

Comment 17

9 years ago
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++.
Assignee

Comment 18

9 years ago
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.
Yup, was missing a refcounting command... fixed in this version.
Attachment #450255 - Attachment is obsolete: true
Reporter

Updated

9 years ago
Attachment #450524 - Flags: review?(jduell.mcbugs)
Assignee

Comment 20

9 years ago
Please note the IPDL refcounting changes for HttpChannel in bug 572980 for whenever this patch is updated next.
Duplicate of this bug: 580287
Reporter

Comment 22

9 years ago
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 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!
Attachment #450524 - Flags: review?(jduell.mcbugs) → review-
Reporter

Comment 24

9 years ago
> +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.
> 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!))
Posted patch Patch v5 (obsolete) — Splinter Review
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?
Attachment #450524 - Attachment is obsolete: true
Attachment #461737 - Flags: review?
Assignee

Comment 27

9 years ago
>+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.
OS: Linux → Windows CE
Posted patch Patch v6 (obsolete) — Splinter Review
(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.
Attachment #461737 - Attachment is obsolete: true
Attachment #462098 - Flags: review?
Attachment #461737 - Flags: review?
Attachment #462098 - Flags: review? → review?(doug.turner)
Reporter

Comment 29

9 years ago
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.
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.)
Posted patch Patch v7 (obsolete) — Splinter Review
De-bitrotting for this high maintenance patch.
Attachment #462098 - Attachment is obsolete: true
Attachment #464593 - Flags: review?(doug.turner)
Attachment #462098 - Flags: review?(doug.turner)

Updated

9 years ago
tracking-fennec: --- → 2.0b1+
Comment on attachment 464593 [details] [diff] [review]
Patch v7

i understand that this bit rotted *again*.
Attachment #464593 - Flags: review?(doug.turner) → review-
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.
Attachment #464593 - Flags: review- → review?(doug.turner)
Attachment #464593 - Flags: review?(doug.turner) → review?(jduell.mcbugs)
Reporter

Comment 34

9 years ago
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).
Assignee: azakai → josh

Updated

9 years ago
tracking-fennec: 2.0b1+ → 2.0b2+
Assignee

Updated

9 years ago
Depends on: 598076
Assignee

Updated

9 years ago
Depends on: 598174
tracking-fennec: 2.0b2+ → 2.0-
Reporter

Comment 35

9 years ago
Comment on attachment 464593 [details] [diff] [review]
Patch v7

clearing review--expecting new patch from jdm.
Attachment #464593 - Flags: review?(jduell.mcbugs)
Assignee

Updated

9 years ago
Attachment #464593 - Attachment is obsolete: true
Assignee

Comment 36

9 years ago
Posted patch Make FTP work for fennec (obsolete) — Splinter Review
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.
Assignee

Updated

9 years ago
Attachment #478841 - Attachment description: "Make FTP work for fennec" [ → Make FTP work for fennec
Attachment #478841 - Flags: review?(jduell.mcbugs)
Assignee

Comment 37

9 years ago
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
OS: Windows CE → Windows Server 2003
Assignee

Updated

9 years ago
Attachment #478841 - Attachment is obsolete: true
Attachment #478841 - Flags: review?(jduell.mcbugs)
Assignee

Comment 38

9 years ago
Posted patch Implement remote FTP (obsolete) — Splinter Review
I'm still unsure why the DataAvailableEvent mixup occurs, but I don't get any problems with the FTP ones renamed to be unique.
Assignee

Updated

9 years ago
Attachment #479866 - Flags: review?(jduell.mcbugs)
Assignee

Updated

9 years ago
Attachment #479866 - Attachment description: "Make FTP work for fennec" [ → Implement remote FTP
want in b2 because we want 598076.
tracking-fennec: 2.0- → 2.0b1+

Updated

9 years ago
tracking-fennec: 2.0b1+ → 2.0b2+
Reporter

Comment 40

9 years ago
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.
Attachment #479866 - Flags: review?(jduell.mcbugs) → review-
Assignee

Comment 41

9 years ago
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.
Assignee

Updated

9 years ago
Attachment #479866 - Attachment is obsolete: true
Assignee

Comment 42

9 years ago
Posted patch Make FTP work for fennec (obsolete) — Splinter Review
Lots and lots of changes made. Just saving my work; this is nearly the finished patch.
Assignee

Updated

9 years ago
Attachment #483108 - Attachment description: "Make FTP work for fennec" [ → Make FTP work for fennec
Assignee

Updated

9 years ago
Attachment #483108 - Attachment is obsolete: true
Assignee

Comment 44

9 years ago
Assignee

Comment 45

9 years ago
Assignee

Updated

9 years ago
Attachment #483478 - Attachment is obsolete: true
Assignee

Comment 47

9 years ago
Assignee

Updated

9 years ago
Attachment #483487 - Flags: review?(dwitte)
Assignee

Updated

9 years ago
Attachment #483477 - Flags: review?(dwitte)
Assignee

Comment 48

9 years ago
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.
Attachment #483488 - Flags: review?(jduell.mcbugs)
Assignee

Updated

9 years ago
Depends on: 564553
Assignee

Comment 49

9 years ago
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 :/
Attachment #483479 - Flags: feedback?(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.
Attachment #483479 - Flags: feedback?(ted.mielczarek) → feedback-
Assignee

Comment 51

9 years ago
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 :(
Attachment #483487 - Flags: review?(dwitte)
Assignee

Updated

9 years ago
Attachment #483487 - Attachment is obsolete: true
Assignee

Comment 53

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

Now passing tests.
Attachment #483722 - Flags: review?(dwitte)
Reporter

Updated

9 years ago
Attachment #483477 - Flags: review?(dwitte) → review+
Reporter

Updated

9 years ago
Attachment #483722 - Flags: review?(dwitte) → review+
Assignee

Updated

9 years ago
Blocks: 561085
Reporter

Comment 54

9 years ago
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 <azakai@mozilla.com>

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.
Attachment #483488 - Flags: review?(jduell.mcbugs) → review-
Assignee

Updated

9 years ago
Attachment #483488 - Attachment is obsolete: true
Assignee

Comment 55

9 years ago
Assignee

Comment 56

9 years ago
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.
Assignee

Comment 57

9 years ago
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.
Assignee

Comment 58

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

Nothing surprising here.
Attachment #484091 - Flags: review?(jduell.mcbugs)
Reporter

Comment 59

9 years ago
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
Attachment #484091 - Flags: review?(jduell.mcbugs) → review+
Assignee

Comment 60

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

Updated

9 years ago
Attachment #483722 - Attachment description: Part 1.5 - Generalize IPDL event queue some more. → Part 1.5 - Generalize IPDL event queue some more. [checked in, comment 60]
Assignee

Updated

9 years ago
Attachment #484091 - Attachment is obsolete: true
Assignee

Comment 61

9 years ago
Assignee

Updated

9 years ago
Attachment #484475 - Attachment is obsolete: true

Comment 63

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

Hopefully we can reland tomorrow.

Comment 64

9 years ago
Done.

http://hg.mozilla.org/mozilla-central/rev/f3a6cf176c00
http://hg.mozilla.org/mozilla-central/rev/e63ac78f181e
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
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)
Depends on: 613643
Depends on: 656992
You need to log in before you can comment on or make changes to this bug.