281153 - Make nsIStreamLoader and nsIUnicharStreamLoader be nsIStreamListeners

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Son Le]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0 (MOOX M1)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.5) Gecko/20041109 Firefox/1.0 (MOOX M1)

From bug 245727, comment 13

Proposal to make nsIStreamLoader an nsIStreamListener and have users of
it open the channel itself as needed (similar to nsIDownloader).

That would also simplify the streamloader code a bit (since it does not need to
handle AsyncOpen failure, which currently makes it post an event).


Reproducible: Always

Comment 1

[Son Le]

Ok. As per :bi's original suggestion, channel openning responsibility will be
moved from nsIStreamLoader and nsIUnicharStreamLoader to the caller. The
question is how to handle this in existing code, especially in NS_NewStreamLoader().

Comment 2

[Darin Fisher]

> The question is how to handle this in existing code, especially in 
> NS_NewStreamLoader().

We'll need to analyze each consumer case-by-case to know how best to deal with
errors returned from AsyncOpen for that particular consumer.

Comment 3

[Son Le]

Ok. In the second NS_NewStreamLoader declaration in NetUtil.h, a new channel is
created and passed to nsStreamLoader to be opened. 

In nsStreamLoader, if the AsyncOpen fails it creates a proxy object to post the
fail message back to the observer.

Do we need the proxy? Would the following code work?

    rv = NS_NewChannel(getter_AddRefs(channel), uri, nsnull,
                       loadGroup, callbacks, loadFlags);
    if (NS_SUCCEEDED(rv)) {
        nsCOMPtr<nsIHttpChannel> httpChannel(do_QueryInterface(channel));
        if (httpChannel)
            httpChannel->SetReferrer(referrer);
        rv = NS_NewStreamLoader(result, observer, context);
        NS_ENSURE_SUCCESS(rv, rv);

        nsCOMPtr<nsIStreamListener> listener(do_QueryInterface(*result));

+       nsresult rv2 = channel->AsyncOpen(listener, context);
+       if (NS_FAILED(rv2)) {
+         observer->OnStreamComplete(*result, context, rv2, 0, nsnull);
+       }
    }

Comment 4

[Son Le]

Attachment: patch v0

Make classes inherit from nsIStreamListener and move channel openning code to
nsNetUtil.h.

This works out nicely as we don't have to touch any other related code and
gives us the flexibility to open/not open channels as required.

Comment 5

[Son Le]

Attachment: patch v0.1

fix tabs

Comment 6

[Darin Fisher]

Comment on attachment 174843 [details] [diff] [review]
patch v0.1

>Index: netwerk/base/public/nsIStreamLoader.idl
...
>+interface nsIStreamLoader : nsIStreamListener
...
>+    void init(in nsIStreamLoaderObserver aObserver,
>               in nsISupports aContext);

The aContext parameter is now out of place.  See nsIDownloader for
example, which does not have this.  The point of aContext before 
was to pass it along to nsIChannel::asyncOpen, but since this object
is no longer responsible for calling that method, there's no point
to giving it aContext.


>Index: netwerk/base/public/nsIUnicharStreamLoader.idl
...
>+interface nsIUnicharStreamLoader : nsIStreamListener
...
>+  void init(in nsIUnicharStreamLoaderObserver aObserver,
>             in nsISupports aContext,
>             in unsigned long aSegmentSize);

ditto.


>Index: netwerk/base/public/nsNetUtil.h
...
>+    if (NS_SUCCEEDED(rv) && aChannel) {
>+        rv = aChannel->AsyncOpen(*aResult, aContext);
>+        if (NS_FAILED(rv))
>+            aObserver->OnStreamComplete(*aResult, aContext, rv, 0, nsnull);

This is definitely wrong.  The OnStreamComplete callback must be asynchronous
to the caller.	I think we should do away with making NS_NewStreamLoader call
AsyncOpen like this if we are truly going to turn it into only a
nsIStreamListener implementation.

How hard would it be to correct the callers?  They should be able to handle
failure directly, or they should be able to decide if calling their own
observer directly is ok.

Comment 7

[Son Le]

Okay.

> How hard would it be to correct the callers?

Probably not too hard, but I don't understand enough of the code to confidently
make the change. Taking nsSound.cpp as an example
(http://lxr.mozilla.org/seamonkey/source/widget/src/windows/nsSound.cpp#188), I
can make the following changes to open the channel:

   nsCOMPtr<nsIStreamLoader> loader;
-  rv = NS_NewStreamLoader(getter_AddRefs(loader), aURL, this);
+  rv = NS_NewStreamLoader(getter_AddRefs(loader), this);
+  NS_ENSURE_SUCCESS(rv, rv);
+  
+  nsCAutoString spec;
+  aURL->GetSpec(spec);
+
+  nsCOMPtr<nsIURI> uri;
+  rv = NS_NewURI(getter_AddRefs(uri), spec);
+  NS_ENSURE_SUCCESS(rv, rv);
 
+  rv = NS_OpenURI(loader, nsnull, uri);
   return rv;

But then how do I get it to handle errors? There is debug code in the
OnStreamComplete() method for error logging, so should I cut and paste it to
here and open the channel synchronously instead?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Son Le, are you still working on this?  Or are you blocked on answers to the questions you asked in this bug?

Comment 9

[Son Le]

Attachment: patch v1

Second attempt. Easier than I expected as bug 310319 has pretty much eliminated the need for answers to my original questions.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'm not sure when I'll get to this -- I have several big reviews in front of it in the queue.  It's not likely to be less than a week, and possible to be several weeks.

You might want to check with darin and biesi to see whether one of them can do the review faster...

Comment 11

[Son Le]

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

no problem. 

darin, can you have a look?

Comment 12

[Christian :Biesinger (don't email me, ping me on IRC)]

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

note: this is not a full review, just a few things I noticed

The changed interfaces should have some comments about how they are meant to be used (i.e. call init on them, then call the channel's asyncOpen with them as listeners)

why remove the context from onDetermineCharset but not from all the other functions?


-      if (NS_SUCCEEDED(mLoader->Init(channel, this, nsnull)))
+      if (NS_SUCCEEDED(mLoader->Init(this))) {
+        channel->AsyncOpen(mLoader, nsnull);
         return;

you shouldn't return here when asyncOpen failed


Why add the include to nsDocument.h?

Comment 13

[Son Le]

Attachment: patch v1.1

(In reply to comment #12)

> The changed interfaces should have some comments about how they are meant to be
> used (i.e. call init on them, then call the channel's asyncOpen with them as
> listeners)

done.
 
> why remove the context from onDetermineCharset but not from all the other
> functions?

The context from onDetermineCharset is the one passed into init().
As aContext has been removed from init(), it's no longer available.

The other functions gets passed their context from the underlying channel.

> -      if (NS_SUCCEEDED(mLoader->Init(channel, this, nsnull)))
> +      if (NS_SUCCEEDED(mLoader->Init(this))) {
> +        channel->AsyncOpen(mLoader, nsnull);
>          return;
> 
> you shouldn't return here when asyncOpen failed

fixed. 

> Why add the include to nsDocument.h?

I get errors when building without it. I assume one of my changes is the cause, but not sure which one in particular.

Comment 14

[Christian :Biesinger (don't email me, ping me on IRC)]

>The context from onDetermineCharset is the one passed into init().
>As aContext has been removed from init(), it's no longer available.

It is available. It is passed to onStartRequest, onDataAvailable and onStopRequest. You shouldn't pass the context just to some of the methods IMO.

Comment 15

[Son Le]

Attachment: patch v1.2

Ahh, I see.

Updated patch to store the context in OnStartRequest() in both streamloaders and use it on the OnStreamComplete() and OnDetermineCharset() callbacks.

Comment 16

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 233361 [details] [diff] [review]
patch v1.2

netwerk/base/public/nsIStreamLoader.idl
The context
+ * argument in the asyncOpen() call will be returned on the onStreamComplete()
+ * callback.

hm... this sentence feels somewhat strange to me... how about "will be passed to the onStreamComplete() callback"?

netwerk/base/public/nsIUnicharStreamLoader.idl
+ * Asynchronously loads an unichar channel into a memory buffer.

There are no unichar channel. The result is also not really a memory buffer... it is a stream. So, maybe the comment should say "Asynchronously load a channel, converting the data to UTF-16".

netwerk/base/src/nsStreamLoader.cpp
+  mContext  = nsnull;

no point, this will be initialized by the nsCOMPtr constructor already.


Please don't add that include to nsDocument.h. That file should need at most a forward-declaration. Generally speaking, avoid adding includes to header files where possible, since more includes means longer compile times (especially when one of the included files changes).

Comment 17

[Son Le]

Attachment: patch v1.3

Fixed as per biesi's comments.

Couldn't get nsDocument to compile with forward declarations... but adding the nsIChannel.idl include back to nsIStreamLoader.idl worked.

Comment 18

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #17)
> Couldn't get nsDocument to compile with forward declarations... but adding the
> nsIChannel.idl include back to nsIStreamLoader.idl worked.

Well before you add it to an IDL file I'd rather see it added to nsDocument.h... What's the error?

Comment 19

[Son Le]

This is the error I'm getting:

../../../dist\include\xpcom\nsCOMPtr.h(197) : error C2504: 'nsIChannel' : base class undefined
        d:\mozilla\content\base\src\nsDocument.h(409) : see reference to class template instantiation 'nsDerivedSafe<T>' being compiled

Comment 20

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 233447 [details] [diff] [review]
patch v1.3

oh... sorry about that, then - the right solution was indeed to include nsIChannel.h in nsDocument.h. Sorry for doubting you. I didn't notice that there was a return for a channel comptr in that file.

r=biesi if you put the include in nsDocument.h and remove it from nsIStreamLoader.idl

Comment 21

[Son Le]

Attachment: patch v1.4

added nsIChannel include to nsDocument.h and remove from nsIStreamLoader.idl

Comment 22

[Boris Zbarsky [:bzbarsky]]

I'm pretty completely swamped right now.  I won't be able to get to this review for weeks at best....  Have you tried mailing Darin and asking him when he might be able to get to it?

Comment 23

[Darin Fisher]

Comment on attachment 233780 [details] [diff] [review]
patch v1.4

sr=darin.. nice job!

Comment 24

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk.  Thanks for the patch, and sorry about the review slowness!

Comment 25

[Doug Shelton]

This fix appears to have broken BeOS builds.  Please see bug 358367 for regression details.