Closed Bug 312760 (basechannel) Opened 19 years ago Closed 8 years ago

Provide base channel for use by protocol implementations

Categories

(Core :: Networking, enhancement, P2)

Product:
Core
Component:
Networking
Type:
enhancement

Tracking

()

Status:
RESOLVED INCOMPLETE
Milestone:
mozilla1.9alpha1

People

(Reporter: darin.moz, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 7 obsolete files)

v1 patch
185.25 KB, patch
Details | Diff | Splinter Review
v1 to v1.2 interdiff
107.44 KB, patch
Biesinger
: review+
Details | Diff | Splinter Review
v1.3 patch [landed-on-trunk]
282.48 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review
v1.2 patch - FTP
204.14 KB, patch
bzbarsky
: superreview+
Details | Diff | Splinter Review 
Provide base channel for use by protocol implementations

As mentioned here, http://wiki.mozilla.org/Necko, it'd be nice to have a base
nsIChannel implementation that all of our channels would inherit from.  This
would allow us to avoid a lot of problems that come up where there are subtle
differences in implementation between all the various channels.

Ideally, a channel implementation should only need to provide an nsIInputStream
implementation to the base channel, and it should be able to get and set meta
data on the channel (e.g., content-type, content-charset, content-length, etc.).
 It should also be able to implement additional interfaces (e.g., nsIHttpChannel).

We could also provide a nsIBaseChannel that would allow external code to make
use of this base channel (via COM aggregation), but that should be a secondary goal.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.9alpha
Alias: basechannel
I assume the problem of making this usable to JS (or python) is similar to the
nsIBaseChannel issue, right?
another possibility for making this usable outside of libnecko might be:

  interface nsIProtocolHandler2 {
    nsIInputStream newChannel(nsIURI);
  }

And have the caller (i.e., nsIIOService) handle the base channel wrapping,
although this would need a bit more thought because the protocol will need to
know the channel for setting properties on it etc.
> although this would need a bit more thought because the protocol will need to
> know the channel for setting properties on it etc.

Right.  And, it is easy enough to implement nsIProtocolHandler::NewChannel using
nsIInputStreamChannel in many cases.  However, nsIInputStreamChannel has its
limitations.  I'm going to post my early patch that provides a basic
nsBaseChannel that serves as a base class for nsInputStreamChannel and
nsDataChannel.  I think it will be easy to extend nsBaseChannel to provide
something that can be aggregated and easy to use from JS.  In a nut-shell,
nsBaseChannel assumes that the derived class will implement one key method:

  virtual nsresult OpenContentStream(PRBool async, nsIInputStream **result);

It also allows the derived class to implement the following method:

  virtual PRBool GetStatusArg(nsresult status, nsString &result);

This method is used in the nsIProgressEventSink implementation in nsBaseChannel
to determine 1) if OnStatus should be called and 2) what the statusArg parameter
should be.  The idea is that derived channels may feed the base channel
transport-layer status events via nsBaseChannel's nsITransportEventSink
implementation or they may configure the base channel to synthesize OnProgress
events from OnDataAvailable events.

It would be fairly straightforward to define a "nsIBaseChannelCallbacks"
interface that also exposed these methods, and to provide a component factory
for nsBaseChannel that hooked everything up and supported COM aggregation.
Attached patch v0.1 prototype patch (obsolete) — — Splinter Review 

    
    

    
  
The patch includes changes to merge necko2.dll into necko.dll.  This makes it so
that it is easy to share nsBaseChannel with all of the protocols under netwerk/.
 Given that libxul is the new build target going forward, I see no reason to
keep necko2.dll around.

I also found a bug in nsInputStringStream::Read where it was calling SetEOF for
no apparent reason.  That method isn't even implemented -- it just asserts :-(

I also make about:cache-entry do a non-blocking synchronous cache lookup instead
of doing the asynchronous lookup.  It makes the code simpler, makes it easy to
nsIInputStreamChannel (thereby eliminating a bunch of code), and doesn't really
impact the usage of about:cache-entry.

    
    

    
  
what's the meaning of "async" in this context?

    
    

    
  
(in this context:
  virtual nsresult OpenContentStream(PRBool async, nsIInputStream **result);
)

    
    

    
  
> what's the meaning of "async" in this context?

It specifies whether the function was called from nsIChannel::Open or
nsIChannel::AsyncOpen.  This is important since it may affect the nsIInputStream
returned.

    
  
Blocks: 312811

    
    

    
  
oh, ok... should it be just called "blocking", then?

    
    

    
  
I like "async" because it corresponds to "asyncOpen", but blocking/nonBlocking
could also be used.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 patch
        — 
          — Splinter Review
      

    


  
Initial patch for check-in.  This patch introduces nsBaseChannel and
nsBaseContentStream.  I've modified the data:, file:, and about:cache-entry
protocol handlers.  I've also implemented nsInputStreamChannel in terms of
nsBaseChannel, which means that this impacts javascript: and other protocols
that use nsInputStreamChannel.

I got rid of necko2.dll since there's no reason to keep it around anymore (and
data: was implemented in there).

    
  

        Attachment #199850 -
        Attachment is obsolete: true

        Attachment #200466 -
        Flags: review?(cbiesinger)

    
    

    
  
Comment on attachment 200466 [details] [diff] [review]
v1 patch

To be reviewed after FF 1.5 :-)

        Attachment #200466 -
        Flags: superreview?(bzbarsky)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.1 patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch includes changes to the gopher protocol handler to implement it in terms of nsBaseChannel.  I've also included a unit test for the file protocol handler.

If you've already started reviewing the v1 patch, then please carry-on with that.  This patch is mostly in addition to the v1 patch.  See attached interdiff from v1 to v1.1 if interested.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 to v1.1 interdiff (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
ok, done with reviewing patch v1. sorry for taking so long. my comments:

A general comment: There's still this open "make necko threadsafe" bug. While we introduce nsBaseChannel, should we think about that? For example, can we maybe require that channels deriving from nsBaseChannel can be opened on any thread?


Ah, this adds a contractid for the streamconv service. Nice :-)

necko2 changes:

+++ netwerk/build/Makefile.in	22 Oct 2005 21:11:54 -0000
-PROTOCOLS = $(filter file ftp http res,$(NECKO_PROTOCOLS))
+PROTOCOLS = $(filter data file ftp http gopher keyword res viewsource,$(NECKO_PROTOCOLS))

So, now that there is only one necko lib, is this $(filter) thing still needed?
Seems like you can just use $(NECKO_PROTOCOLS) as-is.

I think this might break camino:
/camino/Camino.xcode/project.pbxproj, line 15482 -- name = libnecko2.a;
/camino/Camino.xcode/project.pbxproj, line 15483 -- path = ../netwerk/build2/libnecko2.a;
/camino/Camino.xcode/project.pbxproj, line 17181 -- name = libnecko2.dylib;
/camino/Camino.xcode/project.pbxproj, line 17182 -- path = ../dist/Embed/components/libnecko2.dylib;

Don't you also need to remove necko2 from the various installer manifests?
/embedding/config/basebrowser-installer-win.pkg, line 200 -- components\necko2.dll
/embedding/config/basebrowser-mac-macho, line 112 -- components/libnecko2.dylib
/embedding/config/basebrowser-unix, line 181 -- components/libnecko2.so
/embedding/config/basebrowser-win, line 214 -- ; components\necko2.dll
/embedding/config/basebrowser-qnx, line 180 -- components/libnecko2.so 
...and a few others (especially xpinstall/ ones)

Then there's this reference:
/embedding/tests/cocoaEmbed/CocoaEmbed.pbproj/project.pbxproj, line 735 -- path = libnecko2.dylib;

What's with the MODULE=necko2 in the gopher and viewsource makefiles?
And, shouldn't it also be removed from the REQUIRES lines in various makefiles?

xpcom changes:
xpcom/io/nsStreamUtils.cpp
+    nsIOutputStream *outStr = NS_REINTERPRET_CAST(nsIOutputStream *, closure);

not NS_STATIC_CAST?

the rest:
nsNetUtil.h

Is there a reason for using contract IDs for almost everything, but not for the
IO service and standard URL?

netwerk/base/src/Makefile.in	
 REQUIRES	= xpcom \
[...]
+		  caps \
+		  xpconnect \
+		  js \

:(

This is for making scriptsecmanager observing redirects?


nsBaseChannel.h:
  void SetURI(nsIURI *uri) {
    NS_ASSERTION(uri, "must specify a non-null URI");
    NS_ASSERTION(!mURI, "must not modify URI");
    mURI = uri;
  }

Hm... would be kinda nice if that could be a constructor argument, but that
won't work with nsIInputStreamChannel :(

nsIInputStreamChannel.idl should probably document that setURI must be called
exactly once.

I can't quite figure out how nsBaseChannel::GetStatusArg is supposed to work.
When is it called? I suppose it's called by OnTransportStatus? And if
GetStatusArg returns false, only OnProgress is called; otherwise, both
OnProgress and OnStatus?

OK, reading the implementation, that seems to be the case :) But documenting
that a bit more clearly would be nice.

I also think that it should be documented more clearly that a true value for
async means that the stream should be non-blocking if possible, while a false
value means that it should be blocking.

  // Redirect to another channel
  nsresult Redirect(nsIChannel *newChannel, PRUint32 redirectFlags);

I'd like this to have more documentation as well... maybe something like:
  // Redirect to another channel. This method takes care of notifying observers
  // of this redirect as well as of opening the new channel. It also cancels
  // |this|. A failure return means that the redirect could not be performed (no
  // channel was opened; this channel wasn't canceled.)
  // redirectFlags consists of the flag values from nsIChannelEventSink.

Was there a special reason not to use doxygen-like comment markers? I think it
might be nice to automatically generate nsBaseChannel docs.

  // Called when the callbacks available to this channel may have changed.
  void CallbacksChanged() 

Hmm, might subclasses be interested in knowing about this too?

nsBaseChannel.cpp:

I think that this code does not handle a cancelled redirect correctly. It sets
mListener and mListenerContext to nsnull after calling Cancel(), so the listener
won't get any nsIRequestObserver notifications; and all this happened after
AsyncOpen, so it needs to get them.

nsBaseChannel::ContentLength64()
  return NS_SUCCEEDED(rv) ? len : -1;

So we did give up on compilers w/o a 64-bit type? fine with me :)

nsBaseChannel::PushStreamConverter

should maybe document that this should be called before onStartRequest. Hm...
the subclass doesn't know when that is.... hm... maybe say that this should be
called in OpenContentStream? (the base class itself could, of course, call this
at other places too; for example to push an unknowndecoder in front of the
listener in OnStartRequest if the content type is unknown)

Ah, I see that you are doing that :)

    if (invalidatesContentLength)
      SetContentLength64(0);

zero?

nsBaseChannel::GetOriginalURI(nsIURI **aURI)
{
  *aURI = OriginalURI();
  NS_IF_ADDREF(*aURI);

OK, so... OriginalURI() returns null only if both mOriginalURI and mURI are
null. However, other functions (GetName, f.ex.) crash in such a case.

Shouldn't you consistently nullcheck, or consistently not nullcheck, mURI?

Hmm. Should setting the content length property also set mContentLength, to keep
the two in sync? Or is that the responsibility of the caller?


    mProgressSink->OnStatus(this, nsnull, status, statusArg.get());
    mProgressSink->OnProgress(this, nsnull, progress, progressMax);

nsnull? Not mListenerContext?

nsBaseContentStream.h:

   When the stream
// has data or encounters an error, it should be sure to dispatch a pending
// callback if one exists.

I think this should mention that this is done by calling DispatchCallback() (I
didn't notice the existence of this function at first)

nsBaseContentStream.cpp:
  NS_ENSURE_ARG(flags == 0, "ignoring non-default flags");

well actually you aren't ignoring them...


So end-of-file is signalled by calling CloseWithStatus(NS_BASE_STREAM_CLOSED)?

The stream needs to be threadsafe, right? (Meaning that it must be safe to read
it on a background thread, though only one thread at a time).

On which thread does DispatchCallback need to be called? And CloseWithStatus?  I
believe that there's a race condition here, if the thread isn't the one of
mTarget. But the common case is probably to call this from ReadSegments, right?


Should you mention that nsBaseContentStream itself behaves like an empty stream?
(Hm... I guess that's only really the case if the caller close it too :) )

nsBaseContentStream::Available(PRUint32 *result)
{
  *result = 0;
   return mStatus;

The *result line is wrongly indented

nsInputStreamChannel:
NS_IMETHODIMP
nsInputStreamChannel::SetContentStream(nsIInputStream *stream)
{
  NS_ENSURE_TRUE(!mContentStream, NS_ERROR_ALREADY_INITIALIZED);

should maybe document that this can't be reinitialized

nsAboutCacheEntry:
+    NS_NAMED_LITERAL_CSTRING(buffer,
+        "The cache entry you selected is not available.");

Well, it could be that it is currently being loaded. An edge case?

+    nsresult GetContentStream(nsIURI *, nsIInputStream **);

I'd put "virtual" in front of it, to make it clearer that it is a virtual
function..

-[scriptable, uuid(7e835f60-5fea-11d3-a177-0050041caf44)]
-interface nsIDataChannel : nsIChannel {
-
-};

Ah, I've been wanting to do that for a while :-)

nsFileChannel.cpp:
  // We don't have to call PL_DestroyEvent here if PostEvent fails because of
  // the way we are allocated.
  return pool->PostEvent(this);

This does assume that PL_InitEvent doesn't allocate anything...


I think FileCopyEvent::DoCopy should close the stream once it's done writing.
(looks to me like the closing is currently only done when the input stream is
destroyed)

    // fixup content length and type
    if (ContentLength64() < 0) {
      PRUint32 avail;
      rv = stream->Available(&avail);

bleh, we do have 64-bit APIs to do this. Why not ask the nsIFile?

    NS_ADDREF(*result = stream);

(second one)

this could use .swap, right?



    
    

    
  
Awesome review comments... thanks for taking the time to look this over carefully.


(In reply to comment #15)
> A general comment: There's still this open "make necko threadsafe" bug. While
> we introduce nsBaseChannel, should we think about that? For example, can we
> maybe require that channels deriving from nsBaseChannel can be opened on any
> thread?

That bug is almost impossible to solve when you consider how we process redirects.  To implement it, we need a way to have a channel on a background thread redirect safely to some channel that only works on the main thread.  So, I'm not going to try to solve the threadsafe channel problem here.  Main thread only for now.


> +++ netwerk/build/Makefile.in   22 Oct 2005 21:11:54 -0000
> -PROTOCOLS = $(filter file ftp http res,$(NECKO_PROTOCOLS))
> +PROTOCOLS = $(filter data file ftp http gopher keyword res
> viewsource,$(NECKO_PROTOCOLS))
> 
> So, now that there is only one necko lib, is this $(filter) thing still needed?
> Seems like you can just use $(NECKO_PROTOCOLS) as-is.

Good catch, thank you.


> I think this might break camino:
> /camino/Camino.xcode/project.pbxproj, line 15482 -- name = libnecko2.a;
> /camino/Camino.xcode/project.pbxproj, line 15483 -- path =
> ../netwerk/build2/libnecko2.a;
> /camino/Camino.xcode/project.pbxproj, line 17181 -- name = libnecko2.dylib;
> /camino/Camino.xcode/project.pbxproj, line 17182 -- path =
> ../dist/Embed/components/libnecko2.dylib;

OK, I'll have to figure out how to fix the Camino build.  Thanks for catching that.


> Don't you also need to remove necko2 from the various installer manifests?
> /embedding/config/basebrowser-installer-win.pkg, line 200 --
> components\necko2.dll
> /embedding/config/basebrowser-mac-macho, line 112 -- components/libnecko2.dylib
> /embedding/config/basebrowser-unix, line 181 -- components/libnecko2.so
> /embedding/config/basebrowser-win, line 214 -- ; components\necko2.dll
> /embedding/config/basebrowser-qnx, line 180 -- components/libnecko2.so 
> ...and a few others (especially xpinstall/ ones)

Yeah, this was a TODO... but best not to forget


> What's with the MODULE=necko2 in the gopher and viewsource makefiles?
> And, shouldn't it also be removed from the REQUIRES lines in various makefiles?

Changed to MODULE=necko


> xpcom changes:
> xpcom/io/nsStreamUtils.cpp
> +    nsIOutputStream *outStr = NS_REINTERPRET_CAST(nsIOutputStream *, closure);
> 
> not NS_STATIC_CAST?

fixed


> the rest:
> nsNetUtil.h
> 
> Is there a reason for using contract IDs for almost everything, but not for 
> the IO service and standard URL?

I think it's better for code to CreateInstance and GetService by ContractID
than by ClassID.  Since people often copy existing code, I thought it good to make this code do what I'd want others to do.  When it comes to overriding component factories, it is better to override by ContractID  so that the original implementation can still be fetched by ClassID if desired.


> netwerk/base/src/Makefile.in    
>  REQUIRES       = xpcom \
> [...]
> +                 caps \
> +                 xpconnect \
> +                 js \
> 
> :(
> 
> This is for making scriptsecmanager observing redirects?

Right... your patch in that other bug will allow us to eliminate this dependency.  I'm just moving it from protocols/http to base/src.


> nsBaseChannel.h:
>   void SetURI(nsIURI *uri) {
>     NS_ASSERTION(uri, "must specify a non-null URI");
>     NS_ASSERTION(!mURI, "must not modify URI");
>     mURI = uri;
>   }
> 
> Hm... would be kinda nice if that could be a constructor argument, but that
> won't work with nsIInputStreamChannel :(
> 
> nsIInputStreamChannel.idl should probably document that setURI must be called
> exactly once.

Good idea.


> I can't quite figure out how nsBaseChannel::GetStatusArg is supposed to work.
> When is it called? I suppose it's called by OnTransportStatus? And if
> GetStatusArg returns false, only OnProgress is called; otherwise, both
> OnProgress and OnStatus?
> 
> OK, reading the implementation, that seems to be the case :) But documenting
> that a bit more clearly would be nice.

fixed


> I also think that it should be documented more clearly that a true value for
> async means that the stream should be non-blocking if possible, while a false
> value means that it should be blocking.

fixed


>   // Redirect to another channel
>   nsresult Redirect(nsIChannel *newChannel, PRUint32 redirectFlags);
> 
> I'd like this to have more documentation as well... maybe something like:
>   // Redirect to another channel. This method takes care of notifying observers
>   // of this redirect as well as of opening the new channel. It also cancels
>   // |this|. A failure return means that the redirect could not be performed
> (no
>   // channel was opened; this channel wasn't canceled.)
>   // redirectFlags consists of the flag values from nsIChannelEventSink.

I like that text too... thanks.


> Was there a special reason not to use doxygen-like comment markers? I think 
> it might be nice to automatically generate nsBaseChannel docs.

No good reason really.  I was just writing these docs for someone who would be reading the header files directly in an effort to understand and maintain the necko channels.  I didn't think it would be much use on a website.


>   // Called when the callbacks available to this channel may have changed.
>   void CallbacksChanged() 
> 
> Hmm, might subclasses be interested in knowing about this too?

Yeah, I recall thinking that I might have to revisit that when working on porting FTP and HTTP to basechannel.  For now, I'd prefer to avoid an additional virtual function call.  We can always add it if needed.


> nsBaseChannel.cpp:
> 
> I think that this code does not handle a cancelled redirect correctly. It sets
> mListener and mListenerContext to nsnull after calling Cancel(), so the
> listener
> won't get any nsIRequestObserver notifications; and all this happened after
> AsyncOpen, so it needs to get them.

Nope.  That's not how redirects work.  Study the HTTP code.  This is taken from there.  I once tried to make OnStartRequest and OnStopRequest fire with NS_BINDING_REDIRECTED when a redirect occurs, but most listeners don't care.  They just want the data and they are happy to wait for OnStartRequest when the final channel is ready to send them data.  On the contrary, Load Group observers need this information since they care about channels coming and going from the Load Group.


> nsBaseChannel::ContentLength64()
>   return NS_SUCCEEDED(rv) ? len : -1;
> 
> So we did give up on compilers w/o a 64-bit type? fine with me :)

Yes.  In fact, so did WTC for NSPR.  I forget the bug number, but it was the one where I fixed a bug in PR_GetPhysicalMemorySize.  For Gecko 1.9, I'd like to see us get rid of all the messy prlong.h macros and nsInt64 ugliness.  WTC argues that there are no compilers worth caring about that don't support 64 bit integers natively.


> nsBaseChannel::PushStreamConverter
> 
> should maybe document that this should be called before onStartRequest. Hm...
> the subclass doesn't know when that is.... hm... maybe say that this should be
> called in OpenContentStream? (the base class itself could, of course, call this
> at other places too; for example to push an unknowndecoder in front of the
> listener in OnStartRequest if the content type is unknown)

Actually, it doesn't work for the subclass to call this inside OpenContentStream (because mListener is not setup yet -- I assert this in PushStreamConverter).  They basically may call it 
right before they start streaming data.  In the gopher case, this happens once they finish writing out the request data to the socket.


>     if (invalidatesContentLength)
>       SetContentLength64(0);
> 
> zero?

Oops.. that should be -1.



> nsBaseChannel::GetOriginalURI(nsIURI **aURI)
> {
>   *aURI = OriginalURI();
>   NS_IF_ADDREF(*aURI);
> 
> OK, so... OriginalURI() returns null only if both mOriginalURI and mURI are
> null. However, other functions (GetName, f.ex.) crash in such a case.
> 
> Shouldn't you consistently nullcheck, or consistently not nullcheck, mURI?

A channel must always have a URI before it is used.  That said, nsInputStreamChannel should not crash if it lacks a URI, so I might need to do something here.


> Hmm. Should setting the content length property also set mContentLength, to
> keep
> the two in sync? Or is that the responsibility of the caller?

mContentLength?  There is no such member of nsBaseChannel.  GetContentLength reads the property value and converts it from a 64-bit int to a 32-bit int, taking care of overflow errors (returning -1 if the 64-bit value cannot be expressed using a 32-bit int).


>     mProgressSink->OnStatus(this, nsnull, status, statusArg.get());
>     mProgressSink->OnProgress(this, nsnull, progress, progressMax);
> 
> nsnull? Not mListenerContext?

Heh.. interesting, the docs say that it should be, and the FTP channel
implements it that way, but every other channel passes null.  I'll do
as the docs say since I don't think it could hurt anything, and it seems
good.


> nsBaseContentStream.h:
> 
>    When the stream
> // has data or encounters an error, it should be sure to dispatch a pending
> // callback if one exists.
> 
> I think this should mention that this is done by calling DispatchCallback() (I
> didn't notice the existence of this function at first)

fixed


> nsBaseContentStream.cpp:
>   NS_ENSURE_ARG(flags == 0, "ignoring non-default flags");
> 
> well actually you aren't ignoring them...

I changed that to NS_ASSERTION in the v1.1 patch.


> So end-of-file is signalled by calling CloseWithStatus(NS_BASE_STREAM_CLOSED)?

Yeah, basically.


> The stream needs to be threadsafe, right? (Meaning that it must be safe to read
> it on a background thread, though only one thread at a time).

The subclass of nsBaseChannel gets to decide.  If it returns a blocking stream from OpenContentStream (when passed async=true), then it needs to be prepared to be read on a background thread.  But, in that case it will only be treated as a nsIInputStream and not as a nsIAsyncInputStream.


> On which thread does DispatchCallback need to be called? 

On the main thread only.


> And CloseWithStatus? 

Again, see above.  In the background thread case, this could happen on a background thread.  In the case where the stream is a nsIAsyncInputStream, this would only happen on the main thread.


> I
> believe that there's a race condition here, if the thread isn't the one of
> mTarget. But the common case is probably to call this from ReadSegments, right?

When mTarget is involved, everything is on the main thread.

I should document this better.



> Should you mention that nsBaseContentStream itself behaves like an empty
> stream?
> (Hm... I guess that's only really the case if the caller close it too :) )

Right.


> nsBaseContentStream::Available(PRUint32 *result)
> {
>   *result = 0;
>    return mStatus;
> 
> The *result line is wrongly indented

Actually, the return line is bad... fixed.


> nsInputStreamChannel:
> NS_IMETHODIMP
> nsInputStreamChannel::SetContentStream(nsIInputStream *stream)
> {
>   NS_ENSURE_TRUE(!mContentStream, NS_ERROR_ALREADY_INITIALIZED);
> 
> should maybe document that this can't be reinitialized

done


> nsAboutCacheEntry:
> +    NS_NAMED_LITERAL_CSTRING(buffer,
> +        "The cache entry you selected is not available.");
> 
> Well, it could be that it is currently being loaded. An edge case?

"not available" to about:cache ;-)  I decided not to fuss with about:cache
too much.  It doesn't have to be perfect.


> +    nsresult GetContentStream(nsIURI *, nsIInputStream **);
> 
> I'd put "virtual" in front of it, to make it clearer that it is a virtual
> function..

Check again... it's just an ordinary member function.


> -[scriptable, uuid(7e835f60-5fea-11d3-a177-0050041caf44)]
> -interface nsIDataChannel : nsIChannel {
> -
> -};
> 
> Ah, I've been wanting to do that for a while :-)

:-)


> nsFileChannel.cpp:
>   // We don't have to call PL_DestroyEvent here if PostEvent fails because of
>   // the way we are allocated.
>   return pool->PostEvent(this);
> 
> This does assume that PL_InitEvent doesn't allocate anything...

It doesn't, but yeah.... maybe it is a bad idea to code that assumption here.
However, PL_InitEvent has a void return value, which implies that it will never fail, so it should not be allocating anything either.


> I think FileCopyEvent::DoCopy should close the stream once it's done writing.
> (looks to me like the closing is currently only done when the input stream is
> destroyed)

This is fixed in the v1.1 patch.


>     // fixup content length and type
>     if (ContentLength64() < 0) {
>       PRUint32 avail;
>       rv = stream->Available(&avail);
> 
> bleh, we do have 64-bit APIs to do this. Why not ask the nsIFile?

Indeed.. fixed.


>     NS_ADDREF(*result = stream);
> 
> (second one)
> 
> this could use .swap, right?

Yup.  I just need to make sure that the "T" in "nsCOMPtr<T>" matches the return type.  Fixed.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.2 patch (obsolete)
        — 
          — Splinter Review
      

    


  
v1.1 with revisions based on biesi's review comments.

        Attachment #201956 -
        Attachment is obsolete: true

    
    

    
  


  

        Attachment #201957 -
        Attachment is obsolete: true

        Attachment #202001 -
        Flags: review?(cbiesinger)

    
    

    
  
(In reply to comment #16)
> That bug is almost impossible to solve when you consider how we process
> redirects.  To implement it, we need a way to have a channel on a background
> thread redirect safely to some channel that only works on the main thread.  

Oh... good point.

> > Is there a reason for using contract IDs for almost everything, but not for 
> > the IO service and standard URL?
> 
> I think it's better for code to CreateInstance and GetService by ContractID
> than by ClassID.  Since people often copy existing code, I thought it good to
> make this code do what I'd want others to do.  When it comes to overriding
> component factories, it is better to override by ContractID  so that the
> original implementation can still be fetched by ClassID if desired.

Yeah... the main point of that question was, why is the IO service and
standardurl different from the rest?

> Right... your patch in that other bug will allow us to eliminate this
> dependency.  I'm just moving it from protocols/http to base/src.

Yeah, I wanted to avoid that dependency in netwerk/base, so I made that
patch :)

> They basically may call it 
> right before they start streaming data.  In the gopher case, this happens once
> they finish writing out the request data to the socket.

Hm, yeah... onStartRequest doesn't happen until there's at least one byte
in the stream, right?

> > Shouldn't you consistently nullcheck, or consistently not nullcheck, mURI?
> 
> A channel must always have a URI before it is used.  That said,
> nsInputStreamChannel should not crash if it lacks a URI, so I might need to do
> something here.

Well, here again, my point was that in some places it is checked, while in
others it isn't; I'm arguing for some consistency here :-)

> > I
> > believe that there's a race condition here, if the thread isn't the one of
> > mTarget. But the common case is probably to call this from ReadSegments, right?
> 
> When mTarget is involved, everything is on the main thread.

Sorry, I misread the file channel code at first (I thought it was calling
DispatchCallback on the background thread).
 


    
    

    
  
> Yeah... the main point of that question was, why is the IO service and
> standardurl different from the rest?

ah, the 1.2 patch fixes this.

    
    

    
  
Comment on attachment 202001 [details] [diff] [review]
v1 to v1.2 interdiff

-                    , protected nsIInterfaceRequestor
-                    , protected nsITransportEventSink
+                    , public nsIInterfaceRequestor
+                    , public nsITransportEventSink

hm, why this change?

netwerk/base/src/nsBaseContentStream.cpp
+nsBaseContentStream::DispatchCallback(PRBool async)
+  } else {
+    callback.swap(mCallback);

is there a way to assert that mCallbackTarget is on the current thread?

nsFileChannel.cpp
+  mCopyEvent.Dest()->Close();

you aren't closing the stream in the blocking case, right? (why not do this in DoCopy()?)


stopped at +MakeFileInputStream(nsIFile *file, nsCOMPtr<nsIInputStream> &stream,
will continue later today

    
    

    
  
> Yeah... the main point of that question was, why is the IO service and
> standardurl different from the rest?

I didn't finish my previous answer, sorry.  I was going to say that there isn't a good reason, and so I went ahead and changed the code to use ContractIDs universally.  I'm not even sure why I left those two unconverted originally.


> Hm, yeah... onStartRequest doesn't happen until there's at least one byte
> in the stream, right?

It happens once the "content stream" sends the first OnInputStreamReady event.  An empty file doesn't even have a byte of data ;-)


> Well, here again, my point was that in some places it is checked, while in
> others it isn't; I'm arguing for some consistency here :-)

I agree that it'd be nice to have the code null-check consistently.  I was hoping to avoid having to null-check mURI before each use (or at the top of each function) since in all cases, except for nsInputStreamChannel, there is no way for mURI to be null.

    
    

    
  
(In reply to comment #21)
> (From update of attachment 202001 [details] [diff] [review] [edit])
> -                    , protected nsIInterfaceRequestor
> -                    , protected nsITransportEventSink
> +                    , public nsIInterfaceRequestor
> +                    , public nsITransportEventSink
> 
> hm, why this change?

Well, it turns out that in order to implement the content streams for the various protocols, I end up wanting to call some of these nsBaseChannel methods and treat nsBaseChannel as a nsITransportEventSink.  So, they need to be public.  It also turns out that the distinction between public and protected is so negligible since no one outside of the file protocol handler will have direct access to nsFileChannel.


> netwerk/base/src/nsBaseContentStream.cpp
> +nsBaseContentStream::DispatchCallback(PRBool async)
> +  } else {
> +    callback.swap(mCallback);
> 
> is there a way to assert that mCallbackTarget is on the current thread?

Yes.  mCallbackTarget->IsOnCurrentThread could be used.  I added some #ifdef DEBUG code to assert that.


> nsFileChannel.cpp
> +  mCopyEvent.Dest()->Close();
> 
> you aren't closing the stream in the blocking case, right?

Good catch!


> (why not do this in DoCopy()?)

Yeah, that would seem to be better.  I'll make that change.

    
    

    
  
Comment on attachment 202001 [details] [diff] [review]
v1 to v1.2 interdiff

nsGopherChannel.cpp:
Hm, so, in nsGopherContentStream::AsyncWait... what if the base channel's
AsyncWait succeeds, but OpenSocket fails. Then this will return failure,
but have mCallback/mCallbackTarget set. Isn't that a bad thing?

I wonder if a subclass of nsBaseContentStream/nsBaseChannel for code like
gopher's makes sense... but then, gopher is currently the only user of
this.

+    if (buffer[0] == '\0' || (buffer[0] == '/' && buffer[1] == '\0')) {
+    } else {
+        char *sel = buffer.BeginWriting() + 2;

I think you should assert that buffer[1] != '\0', because this code would
access uninitialized memory if so...


Hm... don't the other (non-gopher) stream converters set their content
type on the channel? I guess gopher gets around it by never claiming to be
of type text/gopher-dir. But might this cause an infinite loop if a web
server sends that type, because the new channel still claims to be of type
text/gopher-dir? (not sure if uriloader defends against that..)




test_file_protocol.js
Listener.QueryInterface doesn't QI to nsISupports

I also believe the "member variables" of it are really static variables, as
they're declared in the prototype, but I'm not sure about that (don't know
JS well enough)


      do_throw("onDataAvailable before onStopRequest event!");

You mean "after"?

      do_throw("Failed to load file: " + status);

might want to print it as hex

  dest.append("junk.dat");

I think using createUnique too would be a good idea. as-is this is a potential
security problem when someone runs make check (as tinderboxes ought to)

        Attachment #202001 -
        Flags: review?(cbiesinger) → review+

    
    

    
  
Thanks for the review!


> Hm, so, in nsGopherContentStream::AsyncWait... what if the base channel's
> AsyncWait succeeds, but OpenSocket fails. Then this will return failure,
> but have mCallback/mCallbackTarget set. Isn't that a bad thing?

Hmm... good point.  I think I assumed that the caller would close the stream if it got an error returned from AsyncWait.  In this case, I get to assume the caller is nsInputStreamPump.  That said, it does seem odd to store the callback when the function fails.


> I wonder if a subclass of nsBaseContentStream/nsBaseChannel for code like
> gopher's makes sense... but then, gopher is currently the only user of
> this.

How do you mean?  The file channel also uses this stuff, and FTP and HTTP will too shortly enough.

The new gopher implementation is smaller (in compiled code size) and doesn't have to worry about as many channel API rules and restrictions.  I'll admit that the new implementation is a bit more complicated, but I think that at least 50% of the code is now gopher specific protocol stuff instead of channel implementation boilerplate.


> +    if (buffer[0] == '\0' || (buffer[0] == '/' && buffer[1] == '\0')) {
> +    } else {
> +        char *sel = buffer.BeginWriting() + 2;
> 
> I think you should assert that buffer[1] != '\0', because this code would
> access uninitialized memory if so...

Good catch.  I think the old code suffered from this problem too.


> Hm... don't the other (non-gopher) stream converters set their content
> type on the channel? I guess gopher gets around it by never claiming to be
> of type text/gopher-dir. But might this cause an infinite loop if a web
> server sends that type, because the new channel still claims to be of type
> text/gopher-dir? (not sure if uriloader defends against that..)

Hmm... yes, some of them do.  For example, the indexed-to-html and text-to-html converters set the content type of the channel to text/html.  The bin-hex-decoder and unknown-decoder also update the content type of the channel.

How would a web server send the text/gopher-dir type?  The MIME type is restricted to a pre-defined set indicated by the 'type' byte in the URL.
Or, are you talking about an HTTP response with that content type?


> I also believe the "member variables" of it are really static variables, as
> they're declared in the prototype, but I'm not sure about that (don't know
> JS well enough)

Not a concern because the properties of the prototype are cloned onto instances of Listener.  Those properties are all cloned by value.  If one of those properties were an Object instance, then it would be cloned by reference, which would lead to the problem you're worried about.

    
    

    
  


  
This patch contains revisions for all of the issues biesi pointed out.

        Attachment #202000 -
        Attachment is obsolete: true

        Attachment #202588 -
        Flags: superreview?(bzbarsky)

    
  

        Attachment #200466 -
        Flags: superreview?(bzbarsky)

    
    

    
  
bz pointed out that I am not handling an error returned from nsHashPropertyBag:: Init.  It would be nice if the nsHashPropertyBag took care of this problem for me.

    
    

    
  
I went ahead and added a "nsresult Init()" method to nsBaseChannel that must be called by the subclasses up-front.  On one hand it sort of sucks to have to do this just for nsHashPropertyBag, but I guess there might be reason to add other initialization checks in the future, so what the heck.

    
    

    
  
Comment on attachment 202588 [details] [diff] [review]
v1.3 patch [landed-on-trunk]

>Index: xpcom/io/nsStreamUtils.h
>+/**
>+ * This function is intended to be passed to nsIInputStream::ReadSegments to
>+ * copy data from the nsIInputStream into a character buffer passed as the
>+ * aClosure parameter to the ReadSegments function.

Could we explicitly document the assumptions this makes about buffer size in terms of aClosure, aToOffset, and aCount?  That is, make it easy for callers to pass in a big enough buffer...


>Index: netwerk/base/src/nsBaseChannel.cpp

>+nsBaseChannel::SetContentLength64(PRInt64 len)
>+  // XXX: Storing the content-length as a property may not be what we want.
>+  //      It has the drawback of being copied if we redirect this channel.

So we should just clear it out on the post-redirect channel after we do the copy, imo.

>+nsBaseChannel::OnStopRequest(nsIRequest *request, nsISupports *ctxt,
>+  // No need to suspend pump in this scope since we will not be receiving
>+  // anymore events from it.

"any more"

I was wondering...  This class doesn't seem to have a problem with having Open/AsyncOpen() called on it after OnStopRequest.  Should it throw if that happens?

>Index: netwerk/base/src/nsBaseContentStream.cpp
>+nsBaseContentStream::AsyncWait(nsIInputStreamCallback *callback, PRUint32 flags,
>+  // Our _only_ consumer is nsInputStreamPump

How about we assert that here?  That is, assert that |callback| QIs to nsIInputStreamPump?

>Index: netwerk/base/src/nsBaseContentStream.h

>+// The subclass typically overrides the default Available, ReadSegments and
>+// CloseWithStatus methods.

Should we specify that the base class MUST override those methods?  Or at least
ReadSegments?  I guess if it doesn't we'll just look like we're at EOF to a
caller, which is not too bad....  Either way here, I guess.

>+// If the stream is non-blocking

Which one?  The base, or the subclass?  Or is that the same thing?

> When the caller receives this
>+// error code, they may choose to call the stream's AsyncWait method.

s/they/he/.  And comma after "method", not full stop.  ;)

> When the stream
>+// has data or encounters an error, it should be sure to dispatch a pending
>+// callback if one exists (see DispatchCallback).

This is talking about the subclass stream, not the base, right?  Should
probably disambiguate.

>Index: netwerk/protocol/file/src/nsFileChannel.cpp
>+class nsFileCopyEvent : public PLEvent {
>+  PR_STATIC_CALLBACK(void *) HandleEvent(PLEvent *ev) {
>+  PR_STATIC_CALLBACK(void) DestroyEvent(PLEvent *ev) {

As dbaron recently pointed out, those should actually be declared with C, not C++, linkage to be the same type as what PL_InitEvent actually takes...  See what http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.h#111 does, for example.  Perhaps we should get those typedefs pushed into nspr or something...

>Index: netwerk/streamconv/converters/nsGopherDirListingConv.cpp
> nsGopherDirListingConv::Convert(nsIInputStream *aFromStream,

Why remove the implementation of this method?  I guess it's not used very much, but.....

Plus the nits on irc.

sr=bzbarsky with that.

        Attachment #202588 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
(In reply to comment #25)
> > I wonder if a subclass of nsBaseContentStream/nsBaseChannel for code like
> > gopher's makes sense... but then, gopher is currently the only user of
> > this.
> 
> How do you mean?  The file channel also uses this stuff, and FTP and HTTP will
> too shortly enough.

Sorry for being unclear; I meant the structure of "Write a request to the socket; pass on everything that's received to the listener".

> How would a web server send the text/gopher-dir type?  The MIME type is
> restricted to a pre-defined set indicated by the 'type' byte in the URL.
> Or, are you talking about an HTTP response with that content type?

Yes, I was thinking of a HTTP response with a Content-Type: text/gopher-dir header.

> Not a concern because the properties of the prototype are cloned onto instances
> of Listener.  Those properties are all cloned by value.  If one of those
> properties were an Object instance, then it would be cloned by reference, which
> would lead to the problem you're worried about.

Ah, ok.


in reply to bz:
> s/they/he/.

not "it"? :-)

    
    

    
  
> >+ * This function is intended to be passed to nsIInputStream::ReadSegments to
> >+ * copy data from the nsIInputStream into a character buffer passed as the
> >+ * aClosure parameter to the ReadSegments function.
> Could we explicitly document the assumptions this makes about buffer size in
> terms of aClosure, aToOffset, and aCount?  That is, make it easy for callers 
> to pass in a big enough buffer...

I give a pointer to the documentation for nsWriteSegmentFun.  That seems clean and sufficient to me.  Why duplicate what is written there?


> >Index: netwerk/base/src/nsBaseChannel.cpp
> 
> >+nsBaseChannel::SetContentLength64(PRInt64 len)
> >+  // XXX: Storing the content-length as a property may not be what we want.
> >+  //      It has the drawback of being copied if we redirect this channel.
> 
> So we should just clear it out on the post-redirect channel after we do the
> copy, imo.

That is one solution.  Note, biesi and I discussed this issue in another bug, where he mentioned wanting a way to flag properties as "not to be copied on redirect."  I'd like a general purpose solution, or I'd like to reserve properties for use by Necko consumers.  In this particular case, given that the content length is not assigned until after a redirect occurs, there is no problem.  Moreover, we aren't using nsBaseChannel with any channels that issue redirects yet.


> I was wondering...  This class doesn't seem to have a problem with having
> Open/AsyncOpen() called on it after OnStopRequest.  Should it throw if that
> happens?

Maybe.  We haven't had that kind of check in place for any of our other channels, and it hasn't caused any problems yet.  Adding it probably means adding a new member variable to remember if we've been opened.  I don't think
we need to worry about this issue.


> >Index: netwerk/base/src/nsBaseContentStream.cpp
> >+nsBaseContentStream::AsyncWait(nsIInputStreamCallback *callback, PRUint32 flags,
> >+  // Our _only_ consumer is nsInputStreamPump
> 
> How about we assert that here?  That is, assert that |callback| QIs to
> nsIInputStreamPump?

Hmm... what if nsInputStreamPump implemented nsIInputStreamCallback using a helper class?  The point of the comment is to re-iterate what is said in nsBaseChannel.h, so it is obvious that we are ignoring some of the possible range of input parameters to AsyncWait.


> >Index: netwerk/base/src/nsBaseContentStream.h
> 
> >+// The subclass typically overrides the default Available, ReadSegments and
> >+// CloseWithStatus methods.
> 
> Should we specify that the base class MUST override those methods?  Or at 
> least ReadSegments?  I guess if it doesn't we'll just look like we're at EOF 
> to a caller, which is not too bad....  Either way here, I guess.

The subclass need not override the methods if it is happy with the default implementation.  See nsFileChannel.cpp for an example of a basecontentstream
subclass that doesn't override Available and CloseWithStatus.


> >+// If the stream is non-blocking
> 
> Which one?  The base, or the subclass?  Or is that the same thing?

An instance of the subclass is an instance of the baseclass.  They are
the same stream object :)


> > When the stream
> >+// has data or encounters an error, it should be sure to dispatch a pending
> >+// callback if one exists (see DispatchCallback).
> 
> This is talking about the subclass stream, not the base, right?  Should
> probably disambiguate.

I don't know why you make the distinction.  The "stream" refers to the  nsIInputStream instance, which means that both the code in nsBaseContentStream.cpp and the subclass' code must understand this rule.


> >Index: netwerk/protocol/file/src/nsFileChannel.cpp
> >+class nsFileCopyEvent : public PLEvent {
> >+  PR_STATIC_CALLBACK(void *) HandleEvent(PLEvent *ev) {
> >+  PR_STATIC_CALLBACK(void) DestroyEvent(PLEvent *ev) {
> 
> As dbaron recently pointed out, those should actually be declared with C, not
> C++, linkage to be the same type as what PL_InitEvent actually takes...  See
> what http://lxr.mozilla.org/seamonkey/source/content/base/src/nsDocument.h#111
> does, for example.  Perhaps we should get those typedefs pushed into nspr or
> something...

Bah.  Why worry about this now (when it has never been a problem)?  We have this kind of code repeated everywhere.  If we should clean it up, I'd rather see a bug filed to do so with a tree-wide patch.


> >Index: netwerk/streamconv/converters/nsGopherDirListingConv.cpp
> > nsGopherDirListingConv::Convert(nsIInputStream *aFromStream,
> 
> Why remove the implementation of this method?  I guess it's not used very 
> much, but.....

biesi asked the same question.  The answer is that 1) the method is not used, 2) most other ::Convert methods are unimplemented, and 3) the method cannot be implemented properly since there is no way to get the URI of the gopher request.  I think the distinction between the Convert and AsyncConvert methods is just silly.  A stream converter should be a nsIInputStream "filter", and there should be a way for the consumer to pass some context to the filter.


    
    

    
  
> Why duplicate what is written there?

I don't see where the buffer size assumption is written there.  Am I missing something?

> The subclass need not override the methods if it is happy with the default
> implementation.

Right.  I guess I just don't see an impl that doesn't override ReadSegments...  Either way, though.

> An instance of the subclass is an instance of the baseclass.  They are
> the same stream object  :) 

Yes, but the question is whether this is something subclass implementors have to take care of or whether our base stream handles it for them.  That's what's not clear to me from the comment.

> I don't know why you make the distinction. 

Again, because it's not clear whether this is telling me (the subclass impementor) that I have to take care of it in all cases, or some cases, or what.  Based on the impl, I have to take care of it in some cases, but the base stream does in others, right?  In particular, I have to do it when I have data, since the base stream has no way to tell?

> Why worry about this now (when it has never been a problem)?

Just pointing it out....  I'm fine with a separate tree-wide change here; it's just something to watch out for in terms of suddenly not building if, say, gcc actually starts enforcing this like MSVC sorta-does (sorta, because the CALLBACK thing makes it ignore the linkage).

OK on the rest of the replies.

    
    

    
  
(In reply to comment #32)
> > Why duplicate what is written there?
> 
> I don't see where the buffer size assumption is written there.  Am I missing
> something?

I think I was missing something ;-)  I see what you're referring to now.  I need to make it clear that the buffer pointed to by aClosure should correspond in size to the aCount parameter passed to the ReadSegments method.  I'll update the comments accordingly.


> > The subclass need not override the methods if it is happy with the default
> > implementation.
> 
> Right.  I guess I just don't see an impl that doesn't override ReadSegments... 
> Either way, though.

True, there isn't one right now.  It might make sense for me to expand the NS_DECL_NSIINPUTSTREAM and NS_DECL_NSIASYNCINPUTSTREAM macros, and then document the nsBaseContentStream's implementation of those methods.  This would be good since subclasses that override some of those methods will care about the behavior of the base class's implementation.


> > An instance of the subclass is an instance of the baseclass.  They are
> > the same stream object  :) 
> 
> Yes, but the question is whether this is something subclass implementors have
> to take care of or whether our base stream handles it for them.  That's what's
> not clear to me from the comment.

Well, it depends on what methods the subclass overrides.  The author of the subclass needs to understand the API that they are implementing, and while nsBaseContentStream insulates them from several details, it does not insulate them from _that_ detail.


> > I don't know why you make the distinction. 
> 
> Again, because it's not clear whether this is telling me (the subclass
> impementor) that I have to take care of it in all cases, or some cases, or
> what.  Based on the impl, I have to take care of it in some cases, but the 
> base stream does in others, right?  In particular, I have to do it when I have 
> data, since the base stream has no way to tell?

Right.  You have to signal the callback when you want your ReadSegments to be called again.  ReadSegments is the way that you push both data and _errors_ to your consumer.  OnInputStreamReady just tells the consumer that the stream is either readable or closed, but it can only find out by calling Read or ReadSegments.

    
    

    
  
> I need to make it clear that the buffer pointed to by aClosure should
> correspond in size to the aCount parameter passed to the ReadSegments method. 

Sounds good.

> and while nsBaseContentStream insulates them from several details, it does not
> insulate them from _that_ detail.

Right.  I guess it wasn't clear to me from the comments what it _does_ take care of...

    
    

    
  
Comment on attachment 202588 [details] [diff] [review]
v1.3 patch [landed-on-trunk]

Just one little nit from the peanut gallery:

>Index: netwerk/base/src/nsInputStreamChannel.cpp
>===================================================================
>- * The Initial Developer of the Original Code is
>- * Netscape Communications Corporation.
>- * Portions created by the Initial Developer are Copyright (C) 1998
>+ * The Initial Developer of the Original Code is Google Inc.
>+ * Portions created by the Initial Developer are Copyright (C) 2005

I appreciate the changes are quite drastic, but are they that drastic?  It seems kind of cruel to take credit for what people have done before you.

    
    

    
  
> Just one little nit from the peanut gallery:
> 
> >Index: netwerk/base/src/nsInputStreamChannel.cpp
> >===================================================================
> >- * The Initial Developer of the Original Code is
> >- * Netscape Communications Corporation.
> >- * Portions created by the Initial Developer are Copyright (C) 1998
> >+ * The Initial Developer of the Original Code is Google Inc.
> >+ * Portions created by the Initial Developer are Copyright (C) 2005
> 
> I appreciate the changes are quite drastic, but are they that drastic?  It
> seems kind of cruel to take credit for what people have done before you.
> 

Would you rather I rename the file nsInputStreamChannel2.cpp?  There isn't a shred of code remaining from the old code! ;-)

    
    

    
  
> Sorry for being unclear; I meant the structure of "Write a request to the
> socket; pass on everything that's received to the listener".

I see.  The old gopher code actually wrote to the socket output stream inside the AsyncOpen call.  In some cases, it needed to prompt the user before doing so, and I decided that issuing a prompt from within AsyncOpen might not be so good because it would introduce a modal event queue, which might not be something the caller of AsyncOpen is expecting.  Instead, I moved the setup code to happen from a PLEvent, which is driven by calling AsyncWait on the socket output stream from nsGopherContentStream::OpenSocket.


> > How would a web server send the text/gopher-dir type?  The MIME type is
> > restricted to a pre-defined set indicated by the 'type' byte in the URL.
> > Or, are you talking about an HTTP response with that content type?
> 
> Yes, I was thinking of a HTTP response with a Content-Type: text/gopher-dir
> header.

Hmm... interesting.  I think nsURILoader has code to protect against this problem.  See nsDocumentOpenInfo::ConvertData

    
    

    
  
> I appreciate the changes are quite drastic, but are they that drastic?

Yes.  All the old code except a few header includes is gone in the files where Darin changed the license.  Trust me, I'd nit on licenses if it were nit-worthy.

> I think nsURILoader has code to protect against this problem.  See
> nsDocumentOpenInfo::ConvertData

Actually, I'm not sure we do protect against this; we've run into problems with binhex as a result, iirc...  Again, unless I'm missing something.

    
    

    
  
(In reply to comment #37)
> > Sorry for being unclear; I meant the structure of "Write a request to the
> > socket; pass on everything that's received to the listener".
> 
> I see.  The old gopher code actually wrote to the socket output stream inside
> the AsyncOpen call.  In some cases, it needed to prompt the user before doing
> so

Ah, ok. So most channels can actually write to the socket in AsyncOpen (OpenContentStream), and just return the socketinputstream. OK.


    
    

    
  
> > I think nsURILoader has code to protect against this problem.  See
> > nsDocumentOpenInfo::ConvertData
> 
> Actually, I'm not sure we do protect against this; we've run into problems with
> binhex as a result, iirc...  Again, unless I'm missing something.

I based my comment on what the code says here:
http://lxr.mozilla.org/mozilla/source/uriloader/base/nsURILoader.cpp#689

    
    

    
  
Comment on attachment 202588 [details] [diff] [review]
v1.3 patch [landed-on-trunk]

This patch + nits has landed on the trunk.

        Attachment #202588 -
        Attachment description: v1.3 patch → v1.3 patch [landed-on-trunk]

    
    

    
  
It's interesting... in a multi-DLL build of mozilla, the codesize win of sharing nsBaseChannel seems to have been negated by the codesize bloat of converting all those ClassIDs to ContractIDs in nsNetUtil.h

We should therefore see more of a codesize win with the static firefox build.

    
    

    
  
Darin, this is probably the cause of the Sunbird build bustage on Windows. I filed bug 316254 about this. Could you please look at it. Thanks

    
    

    
  
> I based my comment on what the code says here:
> http://lxr.mozilla.org/mozilla/source/uriloader/base/nsURILoader.cpp#689

All that comment means is that if we're dispatching a type "X" and someone says "well, I'll take X but only if it's converted to Y first", and we find a converter from "X" to "Y" and run it, we should redispatch the type as "Y" even if the converter neglects to update the type on the channel.  If we don't then we get an infinite loop.

This does not cover the case when "Y" is "*/*" (that is, we're doing our general conversion attempt, not a specific one based on the return value of DoContent).  In this case, we'll run the converter, then re-query the channel for the type; if the type is unchanged we'll run the same converter again, etc.

Which of these two situations happens here?



    
    

    
  
> All that comment means is that if we're dispatching a type "X" and someone 
> says "well, I'll take X but only if it's converted to Y first", and we find a
> converter from "X" to "Y" and run it, we should redispatch the type as "Y" 
> even if the converter neglects to update the type on the channel.  If we 
> don't then we get an infinite loop.

So, in other words, there's no need to worry about an infinite loop if a HTTP server sends us content with a type of text/gopher-dir.  That's the concern that biesi raised, and so I think we are fine according to that comment in the code.

    
    

    
  
I don't think we'd even hit the gopher converter from that code, actually.  At least not as things stand...  ;)

So yes, it should be safe.

    
  
Blocks: 311009

    
    

    
  
Did your patch break gopher? Last week I noticed that gopher://gopher.floodgab.com/ only displayed the header whereas a middle of Oct build (trunk) displayed the page fine. Wondered if it was just a transient problem so never filed a bug but did look and found this bug.
Anyways built a new Seamonkey (code DLed yesterday, Nov 14/05) and now all I get is  page load errors (address not found) while the page displays fine in my OSes gopher client.
Anyways suggest checking the above Gopher address to see if Gopher is still working.
ps the page displayed fine if going from a URL but not if typed into the address field.

    
    

    
  
I only see the index line on that page with both Oct 15 and Oct 1 Seamonkey trunk builds...  Of course that hostname doesn't resolve, which probably explains why _that_ happens.

    
    

    
  
Sorry, typo, should of been gopher://gopher.floodgap.com/ which today is loading fine as well as a couple of other gopher pages that did not load yesterday. DNS hiccup?
Anyways nice to have a working gopher client. Thanks

    
    

    
  
This caused bug 316372 (by changing the behavior of input stream channel's AsyncOpen to throw in some cases where it didn't before).
Depends on: 316372

    
    

    
  


  
Not ready for review.  Just posting this here so it doesn't get lost.

    
  
Blocks: 302227
Blocks: 251475

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1 patch - FTP patch (obsolete)
        — 
          — Splinter Review
      

    


  
This patch migrates our FTP implementation to nsBaseChannel.  A bunch of code was removed (from 4639 lines of code down to 3614).

The FTP code was never easy to follow, but hopefully the new code is a bit simpler.  There's no more data request forwarder for instance, and I also removed the funny bit of code that was listening for NS_NET_STATUS_CONNECTED_TO, which never actually did anything interesting when that event happened.

I tested range requests, FTP upload, and suspend/resume.  I also verified using the refcount balancer that none of the main FTP objects are leaking w/ this patch.

        Attachment #205339 -
        Attachment is obsolete: true

        Attachment #211219 -
        Flags: superreview?(bzbarsky)

        Attachment #211219 -
        Flags: review?(cbiesinger)

    
    

    
  
I'm not likely to get to this for at least a week and a half.  :(

    
    

    
  
I'll do this review tomorrow.

    
    

    
  
Comment on attachment 211219 [details] [diff] [review]
v1 patch - FTP patch

OK, that "tomorrow" didn't quite work out, sory.

Index: xpcom/io/nsStreamUtils.h

Why include nsIInputStream.h? All you do is declare pointers to it, right?

Index: netwerk/base/src/nsBaseChannel.cpp
+  // Store the listener early so that OpenContentStream and the stream's
+  // AsyncWait method (called by AsyncRead) can have access to it.  See

Hm, storing the listener but not the context? If OpenContentStream does
call PushStreamConverter, this will pass the wrong context...

Though the context argument here seems to be pretty much ignored, and
nsIStreamConverter{,Service} doesn't define what it is supposed to be,
sigh.


Looking at the streamlistener accessors here... It seems that they are
only required for the cache tee stuff. Maybe a better way to expose this
would be a function like InstallCacheListener(nsICacheEntry*) on
nsBaseChannel which does what nsFtpChannel::InstallCacheListener does now?
That way, other channels can reuse it (HTTP might have a use for it too).

Index: netwerk/base/src/nsInputStreamPump.cpp
+            if (rv == NS_BASE_STREAM_WOULD_BLOCK || (NS_SUCCEEDED(rv) && avail > 0))

Might want to limit this to 80 chars

Index: netwerk/protocol/ftp/public/nsIFTPChannel.idl
+// This interface may be defined as a notification callback on the FTP
+// channel.  It allows a consumer to receive a log of the FTP control
+// connection conversation.
 [scriptable, uuid(455d4234-0330-43d2-bbfb-99afbecbfeb0)]
 interface nsIFTPEventSink : nsISupports
 
Maybe use doxygen syntax here?

nsFTPChannel.cpp

(This does lose the behaviour that Suspend/Resume works even after the
connection drops, but that's probably ok, as callers can use
nsIResumableChannel)

nsFtpConnectionThread.h:
+// The nsFtpState object is the content stream for the channel.  It implements
+// nsIStreamListener, so it can read data from the control connection.

Hm, really? It doesn't seem to implement that (and it's also not needed
for reading data, with your patch)

All the similar-named cache-related functions on this class are a bit
confusing at first, maybe it would be good to add a comment to all of them
briefly describing what they do.

nsFtpConnectionThread.cpp:
             nsXPIDLString formatedString;

Wanna fix that typo? :) (formated -> formatted)

I'm not sure that the entity ID changes are quite correct. You should
compare the supplied entity ID even if startPos is 0, compare
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIResumableChannel.idl#52

+    nsCOMPtr<nsIURL> aURL(do_QueryInterface(mChannel->URI(), &rv));
+    if (NS_FAILED(rv))
+        return rv;
 
Could make this an assertion... this code should only get URIs that were
created by the ftp protocol handler.


Hm... the cache code doesn't check for INHIBIT_CACHING... it probably
should.


So... why not move the code in the FTP_INIT block from OnCallbackPending
to Init? Is there a need for it to be asynchronous?

In OnInputStreamReady:
Why are you calling AsyncWait here? Wouldn't it be enough to have it
called from OnCallbackPending?

In R_pasv:
When you create a data stream here, don't you need to call AsyncWait if
there's a pending callback?



nsFtpControlConnection.h
+    // Called when an error occurs on the control connection.
+    // @param status
+    //        If this is a success code, then data is non-null.  Otherwise,
+    //        this value contains an error code.
+    virtual void OnControlError(nsresult status) = 0;

What data is this comment talking about?

nsFtpControlConnection.cpp

In OnInputStreamReady, is there a point in reading the data from the
stream in the case where we have no listener?

+    // If listener is null, then simply disconnect the listener.  Otherwise,
+    // ensure that we are listening.

Might be good to mention this behaviour in the documentation for WaitData.

Hm, as a sidenote, that asyncWait clears the previous listener is also not
documented for nsIAsyncInputStream...

nsFtpProtocolHandler.cpp
+//    set NSPR_LOG_MODULES=ftp:5
+        gFTPLog = PR_NewLogModule("nsFtp");

That doesn't match :-)

        Attachment #211219 -
        Flags: review?(cbiesinger) → review+

    
    

    
  
> OK, that "tomorrow" didn't quite work out, sory.

no prob ;-)


> Index: xpcom/io/nsStreamUtils.h
> 
> Why include nsIInputStream.h? All you do is declare pointers to it, right?

nsCOMPtr<nsIInputStream> :-(


> Index: netwerk/base/src/nsBaseChannel.cpp
> +  // Store the listener early so that OpenContentStream and the stream's
> +  // AsyncWait method (called by AsyncRead) can have access to it.  See
> 
> Hm, storing the listener but not the context? If OpenContentStream does
> call PushStreamConverter, this will pass the wrong context...
> 
> Though the context argument here seems to be pretty much ignored, and
> nsIStreamConverter{,Service} doesn't define what it is supposed to be,
> sigh.

I think that we should assign mListenerContext early as well.  I'll create
a subroutine to scope everything that needs to succeed in order for mListener
and mListenerContext to remain non-null.  That'll be nicer than adding explicit code to clear those variables on each early return.


> Looking at the streamlistener accessors here... It seems that they are
> only required for the cache tee stuff. Maybe a better way to expose this
> would be a function like InstallCacheListener(nsICacheEntry*) on
> nsBaseChannel which does what nsFtpChannel::InstallCacheListener does now?
> That way, other channels can reuse it (HTTP might have a use for it too).

Good idea, but I think I'll defer that until I port the HTTP code.


> Index: netwerk/protocol/ftp/public/nsIFTPChannel.idl
> +// This interface may be defined as a notification callback on the FTP
> +// channel.  It allows a consumer to receive a log of the FTP control
> +// connection conversation.
>  [scriptable, uuid(455d4234-0330-43d2-bbfb-99afbecbfeb0)]
>  interface nsIFTPEventSink : nsISupports
> 
> Maybe use doxygen syntax here?

OK.


> nsFTPChannel.cpp
> 
> (This does lose the behaviour that Suspend/Resume works even after the
> connection drops, but that's probably ok, as callers can use
> nsIResumableChannel)

Yeah, I know.  I decided to make this change because I don't think we want to build so much complexity into channel implementations (e.g., HTTP).  The downloader should instead be taught how to use nsIResumableChannel.


> nsFtpConnectionThread.h:
> +// The nsFtpState object is the content stream for the channel.  It implements
> +// nsIStreamListener, so it can read data from the control connection.
> 
> Hm, really? It doesn't seem to implement that (and it's also not needed
> for reading data, with your patch)

Fixed.  I think it should say "nsIInputStreamCallback" instead.


> All the similar-named cache-related functions on this class are a bit
> confusing at first, maybe it would be good to add a comment to all of them
> briefly describing what they do.

Good idea.  I've gone and documented a bunch of them.  I should document really document all of the methods :-/  All in good time, I hope.


> I'm not sure that the entity ID changes are quite correct. You should
> compare the supplied entity ID even if startPos is 0...

Good catch.  I fixed this by having nsFtpChannel record a "resume requested" flag that is assigned the value of (mStartPos || !mEntity.IsEmpty()).


> +    nsCOMPtr<nsIURL> aURL(do_QueryInterface(mChannel->URI(), &rv));
> +    if (NS_FAILED(rv))
> +        return rv;
> 
> Could make this an assertion... this code should only get URIs that were
> created by the ftp protocol handler.

Good idea.



> Hm... the cache code doesn't check for INHIBIT_CACHING... it probably
> should.

Thanks.


> So... why not move the code in the FTP_INIT block from OnCallbackPending
> to Init? Is there a need for it to be asynchronous?

Well... I had the idea that it would be nice to have access to nsBaseContentStream::CallbackTarget() so that I would not need to get the current thread's event queue again.  In other words, I didn't want to start reading data until my stream's AsyncWait method gets called.  Now, that happens to get called directly from nsInputStreamPump::AsyncRead, which is called from nsBaseChannel::AsyncOpen.  So, the net result is very similar.


> In OnInputStreamReady:
> Why are you calling AsyncWait here? Wouldn't it be enough to have it
> called from OnCallbackPending?

Good catch.  Yes, I'll make that change.


> In R_pasv:
> When you create a data stream here, don't you need to call AsyncWait if
> there's a pending callback?

No, it turns out that that happens in response to the RETR request which follows PASV and a few other commands.  If we get a 1xx response to the RETR request, then we begin streaming data.


> nsFtpControlConnection.h
> +    // Called when an error occurs on the control connection.
> +    // @param status
> +    //        If this is a success code, then data is non-null.  Otherwise,
> +    //        this value contains an error code.
> +    virtual void OnControlError(nsresult status) = 0;
> 
> What data is this comment talking about?

Whoops.  The two callbacks used to be merged into one function.  That's just leftover cruft.  Fixed.


> nsFtpControlConnection.cpp
> 
> In OnInputStreamReady, is there a point in reading the data from the
> stream in the case where we have no listener?

Well, I figured that it wouldn't hurt to consume the data no matter what.
The listener will only be null when we are shutting down the FTP control connection anyways.


> +    // If listener is null, then simply disconnect the listener.  Otherwise,
> +    // ensure that we are listening.
> 
> Might be good to mention this behaviour in the documentation for WaitData.

Agreed.


> Hm, as a sidenote, that asyncWait clears the previous listener is also not
> documented for nsIAsyncInputStream...

True.  I added a comment on nsIAsync{In,Out}putStream for that.


> nsFtpProtocolHandler.cpp
> +//    set NSPR_LOG_MODULES=ftp:5
> +        gFTPLog = PR_NewLogModule("nsFtp");
> 
> That doesn't match :-)

Thanks!


Revised patch coming up.

    
    

    
  
>> Index: xpcom/io/nsStreamUtils.h
>> 
>> Why include nsIInputStream.h? All you do is declare pointers to it, right?
>nsCOMPtr<nsIInputStream> :-(

I don't see an addition of a comptr in that file in the patch?

> Well, I figured that it wouldn't hurt to consume the data no matter what.

ok. (but you don't necessarily read all of it, just 4 KB)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.1 patch (obsolete)
        — 
          — Splinter Review
      

    


  
bz: I just need a SR from you so that I can land this.  I think biesi did a pretty thorough review, so if you want to just give this a quick SR, I think that'd be fine.  Of course, if you can provide more feedback that'd be ever better.  I do think however that it is essential that I get this into the trunk ASAP to maximize testing from the community.  Thanks!

        Attachment #211219 -
        Attachment is obsolete: true

        Attachment #212663 -
        Flags: superreview?(bzbarsky)

        Attachment #211219 -
        Flags: superreview?(bzbarsky)

    
    

    
  
> That doesn't actually seem to clear mPump if NS_FAILED(rv)...

Fixed.  I was clearing mPump at the callsite of BeginPumpingData, but I must have deleted that line for some reason.  Not sure.  Anyways, it's back, and the comments have been fixed accordingly.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        v1.2 patch - FTP
        — 
          — Splinter Review
      

    


  
Revised patch per comments from biesi.  Interdiff:

diff -u netwerk/base/src/nsBaseChannel.cpp netwerk/base/src/nsBaseChannel.cpp
--- netwerk/base/src/nsBaseChannel.cpp  22 Feb 2006 00:20:23 -0000
+++ netwerk/base/src/nsBaseChannel.cpp  22 Feb 2006 01:21:20 -0000
@@ -233,6 +233,7 @@
   // important that the pending flag is set when we call into the stream and
   // especially when we call into the loadgroup.  However, we have to release
   // this reference if we are not going to return success from this method.
+  // Our caller takes care to release mPump if we return an error.
   mPump = new nsInputStreamPump();
   if (!mPump)
     return NS_ERROR_OUT_OF_MEMORY;
@@ -485,6 +486,7 @@
  
   rv = BeginPumpingData();
   if (NS_FAILED(rv)) {
+    mPump = nsnull;
     mListener = nsnull;
     mListenerContext = nsnull;
   }

        Attachment #212663 -
        Attachment is obsolete: true

        Attachment #212668 -
        Flags: superreview?(bzbarsky)

        Attachment #212663 -
        Flags: superreview?(bzbarsky)

    
  
Priority: -- → P2

    
    

    
  
Comment on attachment 212668 [details] [diff] [review]
v1.2 patch - FTP

>Index: xpcom/io/nsIAsyncInputStream.idl
>      * @param aCallback
>-     *        This object is notified when the stream becomes ready.
>+     *        This object is notified when the stream becomes ready.  This
>+     *        parameter may be null to clear an existing callback.

Should aEventTarget always be null in this case perhaps?  Or otherwise indicate what happens with the other args to this method?

>Index: xpcom/io/nsIAsyncOutputStream.idl

>      * @param aCallback

Same here.

>Index: netwerk/base/src/nsBaseChannel.cpp
>+nsBaseChannel::BeginPumpingData()

>+  // By assigning mPump, we flag this channel as pending (see IsPending).  It's
>+  // important that the pending flag is set when we call into the stream and
>+  // especially when we call into the loadgroup.  However, we have to release
>+  // this reference if we are not going to return success from this method.
>+  // Our caller takes care to release mPump if we return an error.

So... is it actually relevant to this method that we set mPump at this point?  That is, does IsPending() get called from inside AsyncRead or something?  If not, then the first sentence of this comment seems to be misplaced.  The second sentence and third sentence should probably be replaced with "Callers of this method are responsible for nulling out mPump if we fail" or something.

>@@ -445,42 +469,36 @@ nsBaseChannel::AsyncOpen(nsIStreamListen

>+  // Store the listener early so that OpenContentStream and the stream's
>+  // AsyncWait method (called by AsyncRead) can have access to it.  See
>+  // PushStreamConverter and the StreamConverter accessors.

"... which can be called by some AsyncWait implementations" maybe?

>  However, by taking
>+  // a reference to it, we have to be sure to release that if we are not going
>+  // to return success from this method.

"However, make sure to release the listener and the context if we're not going to return success from this method."

>Index: netwerk/base/src/nsBaseChannel.h

>+  // Called when the callbacks available to this channel may have changed.
>+  virtual void OnCallbacksChanged() {
>+  }

Shouldn't this be private?  That is, do we really want subclasses calling this?

>+  void SetStreamListener(nsIStreamListener *listener) {
...
>+  nsIStreamListener *StreamListener() {

And these should probably be protected?  Or are they actually called by things that aren't subclasses?

>+  // Called to setup mPump and call AsyncRead on it.
>+  nsresult BeginPumpingData();

This should probably document that if this method fails the caller is responsible for nulling out mPump.

>Index: netwerk/base/src/nsBaseContentStream.h

>+  // Called from the base stream's AsyncWait method when a pending callback
>+  // is installed on the stream.
>+  virtual void OnCallbackPending() {}

Shouldn't this be private (so subclasses can't call it)?

Also, this is called when callback is cleared too, right?  Should the name and/or comment reflect that?

>Index: netwerk/base/src/nsInputStreamPump.cpp

>+InspectInputStream(nsIInputStream *stream, void *closure, const char *segment,

Maybe call this CheckForAvailableData?

>@@ -503,27 +513,27 @@ nsInputStreamPump::OnStateTransfer()
>+            // Check to see if stream will have more data.  If so, then stay in

I assume there is a reason this is using ReadSegments instead of Available()?  If so, it's worth documenting here, I think...  It's nonobvious to me why Available() can't be used.

>+            if (rv == NS_BASE_STREAM_WOULD_BLOCK || (NS_SUCCEEDED(rv) &&
>+                                                     avail > 0))

I think this would be more readable with the part after the "||" on a separate line (and all of that part would be able to be on that line).

>Index: netwerk/protocol/ftp/public/nsIFTPChannel.idl
>+/**
>+ * This interface may be used to determine if a channel is a FTP channel.
>+ */

So... we have exactly one real consumer of this in the tree (nsSecureBrowserUIImpl.cpp).  And even then, proxied FTP won't have an FTP channel, right?

So perhaps we should consider fixing the nsSecureBrowserUIImpl code and eliminating this interface?  If you agree, please file a followup bug on that (cc kaie), and reference the bug number here.

Also, does the code in calendar/resources/content/publish.js need adjusting?  Or is it saved by XPConnect interface flattening?

>+    /**
>+     * XXX document this method!
>+     */

Please file a bug to do that and reference the bug# here instead of just having an XXX comment?

>Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp

So is it me, or is mUploadLength unused?  As is UploadLength()?  If so, should they be removed?  If not, what did I miss?

>Index: netwerk/protocol/ftp/src/nsFtpProtocolHandler.cpp

>+#define IDLE_CONNECTION_LIMIT 8 /* TODO pref me */

File bug, reference in comment?

>Index: netwerk/protocol/gopher/src/nsGopherChannel.cpp

>+nsGopherContentStream::OnCallbackPending()

So this can get called when mCallback is null (if a null callback is passed to AsyncWait).  Is that desirable?  In that case, do we still want to be opening sockets, calling AsyncWait ourselves, etc?

I have to admit that I pretty much skimmed the guts of the FTP changes.  But they look generally ok.

sr=bzbarsky with these nits picked.

        Attachment #212668 -
        Flags: superreview?(bzbarsky) → superreview+

    
    

    
  
> Should aEventTarget always be null in this case perhaps?  Or otherwise 
> indicate what happens with the other args to this method?

Hmm... I think we should allow any value of aEventTarget.  This means that I need to fix some of the AsyncWait implementations (pipe and socket streams).


> >+  // By assigning mPump, we flag this channel as pending...
> 
> So... is it actually relevant to this method that we set mPump at this point? 
> That is, does IsPending() get called from inside AsyncRead or something?

It is possible.  From AsyncRead, the nsInputStreamPump calls AsyncWait on the given input stream.  I want to flag the channel as pending for consistency.


> If not, then the first sentence of this comment seems to be misplaced.  The 
> second sentence and third sentence should probably be replaced with...

Agreed.


> >Index: netwerk/base/src/nsBaseChannel.h
> 
> >+  // Called when the callbacks available to this channel may have changed.
> >+  virtual void OnCallbacksChanged() {
> >+  }
> 
> Shouldn't this be private?  That is, do we really want subclasses calling 
> this?

Oh, interesting... I hadn't considering doing that, but yeah... I should do that.  Same goes for the rest of the methods that subclasses are expected to implement but not call. 



> >+  void SetStreamListener(nsIStreamListener *listener) {
> ...
> >+  nsIStreamListener *StreamListener() {
> 
> And these should probably be protected?  Or are they actually called by things
> that aren't subclasses?

It turns out that many of the methods on nsBaseChannel are called by the content stream for the channel.  Unfortunately, making a content stream class a friend of a subclass of nsBaseChannel does not make the content stream class a friend of nsBaseChannel.  As a result, I ended up making most things public on nsBaseChannel.  This happens to be the case for FTP.  I could define a method on nsFTPChannel that calls nsBaseChannel::SetStreamListener, but that seemed a bit overkill to me.


> >Index: netwerk/base/src/nsBaseContentStream.h
> 
> >+  // Called from the base stream's AsyncWait method when a pending callback
> >+  // is installed on the stream.
> >+  virtual void OnCallbackPending() {}
> 
> Shouldn't this be private (so subclasses can't call it)?

Yes, good idea.


> Also, this is called when callback is cleared too, right?  Should the name
> and/or comment reflect that?

I changed it so that it is now only called when the callback is assigned a non-null value.  I think that is what I intended.  Thanks for catching the glitches with "null input to AsyncWait" handling.


> >Index: netwerk/base/src/nsInputStreamPump.cpp
> 
> >+InspectInputStream(nsIInputStream *stream, void *closure, const char *segment,
> 
> Maybe call this CheckForAvailableData?

I went with GetBytesAvailable.


> >@@ -503,27 +513,27 @@ nsInputStreamPump::OnStateTransfer()
> >+            // Check to see if stream will have more data.  If so, then stay in
> 
> I assume there is a reason this is using ReadSegments instead of Available()? 
> If so, it's worth documenting here, I think...  It's nonobvious to me why
> Available() can't be used.

Yeah, the reason does need to be documented.  It is very subtle.  We could have a stream that is seekable that is at the end-of-file, but not closed.  Such a stream would return 0 bytes available and NS_OK, which we could not distinguish from a stream that is still waiting for more data to become available.  ReadSegments is used to distinguish those cases.  Have I ever mentioned my disdain for nsIInputStream?


> >+            if (rv == NS_BASE_STREAM_WOULD_BLOCK || (NS_SUCCEEDED(rv) &&
> >+                                                     avail > 0))
> 
> I think this would be more readable with the part after the "||" on a separate
> line (and all of that part would be able to be on that line).

It turns out that I don't need the "> 0" bit, which makes everything fit on
one line ;-)


> >Index: netwerk/protocol/ftp/public/nsIFTPChannel.idl
> >+/**
> >+ * This interface may be used to determine if a channel is a FTP channel.
> >+ */
> 
> So... we have exactly one real consumer of this in the tree
> (nsSecureBrowserUIImpl.cpp).  And even then, proxied FTP won't have an FTP
> channel, right?

So, I debated about getting rid of nsIFTPChannel in this patch, but due to the secure browser UI consumer, I decided not to do so yet.  Yeah, proxied FTP results in a HTTP channel being used.


> So perhaps we should consider fixing the nsSecureBrowserUIImpl code and
> eliminating this interface?  If you agree, please file a followup bug on that
> (cc kaie), and reference the bug number here.

ok.  see bug 328914.


> Also, does the code in calendar/resources/content/publish.js need adjusting? 
> Or is it saved by XPConnect interface flattening?

right, xpconnect flattening to the rescue!


> >+    /**
> >+     * XXX document this method!
> >+     */
> 
> Please file a bug to do that and reference the bug# here instead of just having
> an XXX comment?

Another lame interface.  It should be fixed to be less lame.  See bug 328915.


> >Index: netwerk/protocol/ftp/src/nsFTPChannel.cpp
> 
> So is it me, or is mUploadLength unused?  As is UploadLength()?  If so, should
> they be removed?  If not, what did I miss?

Hmm... the old code ignored the length parameter passed to SetUploadStream.  Hmm... HTTP also ignores it (sort of).  HTTP will use the supplied value to set the Content-Length header, but it does not enforce that the stream has the number of bytes prescribed.  I would add debug code to assert that Available matches the length given, but then that isn't strictly required.  The stream might say that it has only 1 byte available right now, but when read, it might actually supply the specified number of bytes.  OK, in that case, I'll jsut ignore the supplied length parameter.


> >Index: netwerk/protocol/ftp/src/nsFtpProtocolHandler.cpp
> 
> >+#define IDLE_CONNECTION_LIMIT 8 /* TODO pref me */
> 
> File bug, reference in comment?

That sounds like excessive bug noise to me for something that isn't that big of a deal.  Who's going to care?

    
    

    
  
> many of the methods on nsBaseChannel are called by the content stream for the
> channel.

Ah, ok.  Leaving those public is fine, then.

> >+#define IDLE_CONNECTION_LIMIT 8 /* TODO pref me */

If we don't have obvious plans to make it a pref, then I guess no need for a bug... either way.



    
  

        Attachment #212668 -
        Attachment description: v1.2 patch - FTP → v1.2 patch - FTP [landed on trunk]

    
    

    
  
Comment on attachment 212668 [details] [diff] [review]
v1.2 patch - FTP

OK, this patch is on the trunk now.

        Attachment #212668 -
        Attachment description: v1.2 patch - FTP [landed on trunk] → v1.2 patch - FTP [landed-on-trunk]

    
    

    
  
This hit Tp pretty hard. btek 936ms -> 972ms, luna 895ms -> 945ms, planetoid 365ms -> 388ms (all approx. averages). Ts seems to be (mildly) affected too.

    
    

    
  
(In reply to comment #66)
> This hit Tp pretty hard. btek 936ms -> 972ms, luna 895ms -> 945ms, planetoid
> 365ms -> 388ms (all approx. averages). Ts seems to be (mildly) affected too.

Thanks jag.  I'm on it.

    
    

    
  
I backed out my patch to clear the Tp regression.

    
  

        Attachment #212668 -
        Attachment description: v1.2 patch - FTP [landed-on-trunk] → v1.2 patch - FTP

    
    

    
  
So for what it's worth, if we can't figure out whence the Tp issue comes and need to experiment I think it would be reasonable to arrange a carpool so this can be landed and then tweaked to see how perf is affected...

That's if we can't figure out what's going on in general, of course.

    
    

    
  
I installed the Tp test on my linux box this morning, and my windows box is building an opt tree right now so that I can hopefully reproduce the regression and fix it locally.

    
    

    
  
The performance regression was caused by my changes to nsInputStreamPump.  I backed those out on the trunk for now and landed the rest of the changes.  Those changes were not strictly necessary, but they seemed like the right thing to do.  I'll probably experiment a bit futher to understand exactly why those changes were so expensive.  (FWIW, I noticed no Tp change with my local pageloader setup.)

    
    

    
  
Could it be a matter of us firing onload later (off the next PLEvent or something)?

    
    

    
  
I just stumbled on this:

http://lxr.mozilla.org/mozilla/source/netwerk/protocol/http/src/nsHttpChannel.cpp#643

Should that have been removed?

Also, what's left to do in this bug? Just the nsInputStreamPump changes? Maybe that would be better as a new bug?

    
    

    
  
> Should that have been removed?

A quick look at nsHttpChannel.h shows that HTTP does not use nsBaseChannel yet.

I believe this also answers your second question...

I do think we want to do this for 1.9.
Flags: blocking1.9?

    
    

    
  
Shaver, we don't think this is important for gecko itself, but it could be important for addons to implement generic protocol handlers. Do you think that is important enough to find somebody to work on this?
Flags: blocking1.9?

    
    

    
  
If it could be aggregated by JS impls as well, perhaps, but otherwise I think it's of middling value at best to the current add-on community.

    
    

    
  
Yeah, the idea is to have a way exposed to JS of aggregating this, last I talked to Darin.

    
    

    
  
-> reassign to default owner
Assignee: darin.moz → nobody
Status: ASSIGNED → NEW
QA Contact: benc → networking
Depends on: 390938

    
    

    
  
going to take a stab at the http part. this will either result in a patch or a 'reassign to default owner' in a month or two ;)
Assignee: nobody → dwitte

    
    

    
  
Reassigning to nobody. If anyone wants to work on this, feel free!
Assignee: dwitte → nobody
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → INCOMPLETE

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: