261083 - Channels do not uniformly query their notification callbacks

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Darin Fisher]

Channels do not uniformly query their notification callbacks.

Channels should query nsIChannel::notificationCallbacks first and then fallback
to querying nsILoadGroup::notificationCallbacks if a loadgroup is set.

We should probably create a helper function for this and stick it in
nsNetUtils.h so that protocol handlers outside of necko can share the
implementation.

Comment 1

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

if you want to fix mailnews too:
http://lxr.mozilla.org/seamonkey/source/mailnews/base/util/nsMsgProtocol.cpp#655
http://lxr.mozilla.org/seamonkey/source/mailnews/imap/src/nsImapProtocol.cpp#8392

(hmm, can't find more cases...)

Comment 2

[Darin Fisher]

Attachment: v1 uber patch

OK, this patch tries to fix all of the places where we poke the
notificationCallbacks corresponding to a nsIChannel instance be it from within
the nsIChannel implementation or by some consumer.

To deal with things like nsISocketTransport::securityCallbacks, I ended up
creating a nsIInterfaceRequestor aggregation object in xpcom/base.  It's only
available to C++ code for now via NS_NewInterfaceRequestorAggregation.	It's
not great, and I didn't really want to add this, but we need to be able to deal
with those pesky securityCallbacks from multiple XPCOM components (HTTP and
MailNews to be precise and possibly others in the future if we ever support
SOCKS 5 authentication).

In most places however I was able to just call a handy function I added to
nsNetUtil.h called NS_QueryNotificationCallbacks.  That function takes either a
nsIChannel (or a nsIInterfaceRequestor and a nsILoadGroup) along with an nsIID
reference and returns the requested interface using the canonical logic.

I also took a crowbar to FTP in the process since it was doing some really
wacking things with its notification callbacks.

Besides the FTP changes, most everything in this patch was fairly
straightforward.

Comment 3

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

ok... so in modules/libjar/nsJARChannel.cpp (at least), it is now impossible to
reset the notificationCallbacks after the channel is opened.

ExternalHelperAppService would like to do that (used to set them from another
load cookie, now sets them to null), though, in order not to send progress and
status notification to the browser window, which is what I think happens with
the patch.

Comment 4

[Darin Fisher]

> ok... so in modules/libjar/nsJARChannel.cpp (at least), it is now impossible to
> reset the notificationCallbacks after the channel is opened.

hmm... interesting.  perhaps we should make SetNotificationCallbacks simply
clear any values that were queried when the callbacks are cleared.  that would
probably do the trick.

Comment 5

[Darin Fisher]

> hmm... interesting.  perhaps we should make SetNotificationCallbacks simply
> clear any values that were queried when the callbacks are cleared.  that would
> probably do the trick.

er.. maybe not.  if the values came from the loadgroup then clearing them would
be wrong.  ugh... so... if we were to clear them and then require that they be
queried each place they are needed instead of up front in AsyncOpen, then that
would work.

Comment 6

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

hm... what would also work is querying them in both setloadgroup and
setnotificationcallbacks. would that be better? it'd certainly mean less
getinterface for progresseventsink

Comment 7

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

Comment on attachment 159807 [details] [diff] [review]
v1 uber patch

r- per above comments

Comment 8

[Darin Fisher]

> hm... what would also work is querying them in both setloadgroup and
> setnotificationcallbacks. would that be better? it'd certainly mean less
> getinterface for progresseventsink

yeah, but each time we create a channel, we'd basically go down the query your
notification callbacks path twice.


> ExternalHelperAppService would like to do that (used to set them from another
> load cookie, now sets them to null), though, in order not to send progress and
> status notification to the browser window, which is what I think happens with
> the patch.

so, do you think it is sufficient to say that the notification callbacks can
only be cleared after AsyncOpen has been called?  that is, we could simply null
out any derived interface pointers whenever SetNotificationCallbacks(null)
and/or SetLoadGroup(null) is done.

Or, we could just put the burden on the consumer, and make docshell/uriloader
implement a nsIProgressEventSink proxy object that can be told to drop any
incoming notifications.  That has the advantage of being more robust in the face
of incorrectly implemented third-party protocol handlers.

Comment 9

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

http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIChannel.idl#90
doesn't document any restrictions on the notificationCallbacks... let's hope no
extension/embeddor relies on setting them after asyncOpen. since changing the
callbacks is rarely needed, that's probably reasonable.

>Or, we could just put the burden on the consumer, and make docshell/uriloader
>implement a nsIProgressEventSink proxy object that can be told to drop any
>incoming notifications.

hmm, yeah. isn't it actually docloader which is the progresseventsink? so, maybe
it would be sufficient to make it not send any notifications once the load has
been handed off to the externalhelperappservice.

Comment 10

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

OK. The easiest solution for this seems to be: docloader should ignore
OnProgress and OnStatus for a channel that's no longer part of the loadgroup
(and thus completed, as far as the docloader is concerned).

maybe it does that already. needs checking.

Comment 11

[Darin Fisher]

> maybe it does that already. needs checking.

Indeed it does.  If the request has no associated nsRequestInfo object, then
it's OnProgress calls are ignored.

So, I think this is the patch we want :)

Comment 12

[Darin Fisher]

Comment on attachment 159807 [details] [diff] [review]
v1 uber patch

re-requesting review.  see comment #11.  thanks!

Comment 13

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

excellent!

please document the new restriction that notificationCallbacks can only be set
before asyncOpen now.

Comment 14

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

Comment on attachment 159807 [details] [diff] [review]
v1 uber patch

mailnews/base/util/nsMsgProtocol.cpp
+  if (status == nsISocketTransport::STATUS_RECEIVING_FROM ||
+      status == nsISocketTransport::STATUS_SENDING_TO)
+  {
+    // do nothing....do NOT report socket transport progress bytes either

maybe it would be better to early return here - less indentation to keep track
of ;)

+      mProgressEventSink->OnStatus(this, nsnull, status,
NS_ConvertUTF8toUCS2(host).get());

wanna s/UCS2/UTF16/ while you're here?

mailnews/imap/src/nsImapProtocol.cpp

same comment about early return and UCS2

mailnews/local/src/nsPop3Protocol.cpp
+    // When we are making a secure connection

hmm, you are sure that's needed only then? (asking because I'm thinking
mailnews might be expecting an nsIPrompt there in other cases too... although
probably not)

(same in mailnews/news/src/nsNNTPProtocol.cpp)

well, I suppose we can make the change and wait if we get bug reports ;)


I'm noticing that pop3 and nntp set an interface normally, but you made imap
use a callbackaggregator; is that needed?

netwerk/base/public/nsNetUtil.h
hmm, why not implement the nsIChannel variant using the other one? i.e.:
  channel->GetNotificationCallbacks(cb);
  channel->GetLoadGroup(lg);
  return NS_QueryNotificationCallbacks(cb, lg, aIID, result);

(pseudocode)

since this is all inline, the generated code shouldn't be any different

netwerk/protocol/ftp/src/nsFTPChannel.cpp

+    // to worry about some wierd re-entrancy scenario.

dictionary.com claims this is spelled "weird" :-)


+	 if (NS_SUCCEEDED(rv)) {
+	     //
+	     // XXX this leaves us in a bad state !!
+	     //
+	     //     since we have already called AddRequest, we must not return
+	     //     a failure code.

but this is a success code, as ensured by the if? this code looks correct to me
- cache open succeeded; it will notify us when it's opened, and we will then
call the streamlistener methods (presumably)

although I think this should be NS_OK - an nsIChannel caller will not care
about specific cache success codes, I suspect

+nsFTPChannel::GetInterface(const nsIID &anIID, void **aResult )

wanna make this aIID while you're here?


netwerk/protocol/ftp/src/nsFtpControlConnection.cpp
-    // we do not care about notifications from the write channel.
-    // a non null context indicates that this is a write notification.
-    if (aContext != nsnull) 
-	 return NS_OK;

hmm, why is this no longer needed? or was it never needed?
ah... I see... it seems to be always null, since AsyncRead is only used once w/
this class...

netwerk/protocol/ftp/src/nsFtpControlConnection.h
+    void GetReadRequest(nsIRequest** r)	   { NS_IF_ADDREF(*r =
mReadRequest); }
+    void GetTransport(nsITransport** t)	   { NS_IF_ADDREF(*t = mCPipe);
}

hm, maybe these would be more pleasant to use if they returned a non-addrefed
nsIRequest*/nsITransport*, instead of using an out param?

netwerk/protocol/http/src/nsHttpChannel.h
+    // Helper function to simplify getting notification callbacks.
+    template <class T>
+    void GetCallback(nsCOMPtr<T> &aResult)

ohh, this is nice. I wonder if it may be useful generally, as another signature
for NS_QueryNotificationCallbacks? or maybe even as the only one?


should
http://lxr.mozilla.org/seamonkey/source/netwerk/base/public/nsIChannel.idl#92
make it more clear that the notificationCallbacks from the loadgroup should or
even must be used? we probably can't say "must", this being frozen...

thanks a lot for making this patch!

Comment 15

[Darin Fisher]

Attachment: v1.1 patch

with review comments applied.

Comment 16

[Darin Fisher]

Attachment: v1.2 patch

oops, the previous patch lacked my comment additions to nsIChannel.idl

Comment 17

[Darin Fisher]

> mailnews/local/src/nsPop3Protocol.cpp
> +    // When we are making a secure connection
> 
> hmm, you are sure that's needed only then? (asking because I'm thinking
> mailnews might be expecting an nsIPrompt there in other cases too... although
> probably not)

the prompt is passed as securityCallbacks to the socket transport.  that is only
passed to PSM.


> I'm noticing that pop3 and nntp set an interface normally, but you made imap
> use a callbackaggregator; is that needed?

As mentioned on #developers, it's unclear, but IMAP unlike the other protocols
has a mock channel.  So, there is a loadgroup, and so this code is possible.  It
was not clear to me how to do the same for the other mailnews protocols.


> since this is all inline, the generated code shouldn't be any different

Well, in cases where we are given a channel, it seems advantageous to delay
calling GetLoadGroup until we know that we need it.  Since all of this is
inlined, there's no codesize reason to justify reusing the code.


> but this is a success code, as ensured by the if?

ACK!  yes, not sure how i managed to read NS_FAILED(rv)!


> hmm, why is this no longer needed? or was it never needed?

left-over cruft from the nsIStreamProvider days.


> hm, maybe these would be more pleasant to use if they returned a non-addrefed
> nsIRequest*/nsITransport*, instead of using an out param?

great idea.


> ohh, this is nice. I wonder if it may be useful generally, as another 
> signature for NS_QueryNotificationCallbacks? or maybe even as the only one?

done.

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

So first of all, the nsIChannel docs say that channels _may_ query the
notification callbacks of the loadgroup if they don't have any.  Should that be
a "must"?

>Index: mailnews/base/util/nsMsgProtocol.cpp

> nsMsgProtocol::OnTransportStatus(nsITransport *transport, nsresult 
>+  if (mLoadFlags & LOAD_BACKGROUND || !m_url)
>+    return NS_OK;

You need parens around the loadflags check (|| has higher precedence than &).

>Index: mailnews/imap/src/nsImapProtocol.cpp

> nsImapMockChannel::OnTransportStatus(nsITransport *transport, nsresult >+  if (NS_FAILED(m_cancelStatus) || mLoadFlags & LOAD_BACKGROUND || !m_url)
>+    return NS_OK;

Same here.

>Index: mailnews/local/src/nsPop3Protocol.cpp
>+    // When we are making a secure connection, we need to make sure that we
>+    // pass an interface requestor down to the socket transport so that PSM can
>+    // retrieve a nsIPrompt instance if needed.

Is this really not needed for insecure connections?  I'd want a mailnews peer
to sign off on a functionality change like that...

>Index: mailnews/news/src/nsNNTPProtocol.cpp

>+    // When we are making a secure connection, we need to make sure that we
>+    // pass an interface requestor down to the socket transport so that PSM can
>+    // retrieve a nsIPrompt instance if needed.

Again.

>Index: extensions/datetime/nsDateTimeChannel.cpp

>-    // not fatal if these fail
>-    mTransport->SetSecurityCallbacks(mCallbacks);

Is there a reason this is not needed anymore?

>Index: extensions/finger/nsFingerChannel.cpp

>-    // not fatal if these fail
>-    mTransport->SetSecurityCallbacks(mCallbacks);

And here.

>Index: extensions/wallet/src/nsWalletService.cpp
>-                                  channel->GetNotificationCallbacks(getter_AddRefs(interfaces));

Does this make |interfaces| unused?  If so, remove the nsCOMPtr declaration? 
If not, where is it now set?

>+NS_QueryNotificationCallbacks(nsIChannel   *aChannel,
>+                              const nsIID  &aIID,
>+                              void        **aResult)

Is there a reason not to implement this in terms of the
NS_QueryNotificationCallbacks that takes an interface requestor and a
loadgroup?  I suppose that would cause us to get the loadgroup unnecessarily in
some cases, but would save some code duplication...  If it's done, the other
form could also probably be made out-of-line (since I doubt these functions are
called that much, and that would save codesize).

In any case, this should NS_PRECONDITION(aChannel) so that people who just
glance at the method can tell that aChannel can't be null.

>+/* template helper */
>+template <class T> inline void
>+NS_QueryNotificationCallbacks(nsIChannel  *aChannel,
>+                              nsCOMPtr<T> &aResult)

I think I'd actually prefer this to take a T** and have callers call
getter_AddRefs on their nsCOMPtrs.  That gives callers more flexibility (eg
they can NS_QueryNotificationCallbacks into an out param easily if they want).

If you have strong feelings about leaving this the way it is, I guess that's
fine too, though.

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

>-      mLock(nsnull),

I assume the mLock stuff is just unneeded cruft and all that?  That is, this is
general cleanup unrelated to the change at hand?

> nsFTPChannel::SetupState(PRUint32 startPos, const nsACString& entityID)

Is it ok that we no longer proxy the nsIAuthPrompt, nsIPrompt?	We used to.... 
I guess you proxy the nsIFTPEventSink in the nsFtpState code itself, so that
one's ok.

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

> nsFtpState::S_pasv() {
>+        nsITransport *controlSocket = mControlConnection->Transport();
>         if (!controlSocket) return FTP_ERROR;

If it can return null, GetTransport() is probably a better name... (I think the
decom interface naming convention people have mostly settled on is that lack of
a Get prefix means that null is never returned.)

> nsFtpState::IsPending(PRBool *result)

>+    nsIRequest *request = mControlConnection->ReadRequest();
>+    if (request)

Same issue.

> nsFtpState::DataConnectionComplete()

>-        mDPipe->SetSecurityCallbacks(nsnull);

Why?  I assume that call was breaking cycles or something, no?

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

Again, why is mLock no longer needed?  It looks like it was guarding against
the listener being set from a different thread; can that never happen?	If so,
why not?

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

> nsGopherChannel::AsyncOpen(nsIStreamListener *aListener, nsISupports 

>-    mTransport->SetSecurityCallbacks(mCallbacks);

Why?

The rest looks ok, but I'd like to see some of the above questions answered
before marking sr...

Comment 19

[Darin Fisher]

> So first of all, the nsIChannel docs say that channels _may_ query the
> notification callbacks of the loadgroup if they don't have any.  Should that >
be a "must"?

That would be an interface change, wouldn't it?  See bottom of comment #14 and
my response to biesi on this same issue in comment #17 ;-)


> You need parens around the loadflags check (|| has higher precedence than &).

Ack.. thanks!


> Is this really not needed for insecure connections?  I'd want a mailnews peer
> to sign off on a functionality change like that...

And, what would an insecure socket transport do with _security_ callbacks?  The
interface requestor is unused otherwise.  Biesi asked the same question in his
review, and my response is in comment #17.


> Is there a reason this is not needed anymore?
> And here.

Yup, not needed since neither datetime nor finger use secure sockets.


> Does this make |interfaces| unused?  If so, remove the nsCOMPtr declaration? 
> If not, where is it now set?

Good catch.


> Is there a reason not to implement this in terms of the
> NS_QueryNotificationCallbacks that takes an interface requestor and a
> loadgroup?

Again, this is something biesi asked, and my response is also in comment #17.
Remember, that this code is inlined at the callsite, so there is no codesize
savings by implementing one in terms of the other.


> In any case, this should NS_PRECONDITION(aChannel) so that people who just
> glance at the method can tell that aChannel can't be null.

Hmm, sure.


> I think I'd actually prefer this to take a T** and have callers call
> getter_AddRefs on their nsCOMPtrs.  That gives callers more flexibility (eg
> they can NS_QueryNotificationCallbacks into an out param easily if they 
> want).

Well, there's also the non-template version of the function.  I created this
version for two reasons: (1) nearly all callers can use it, and (2) it made the
callsites less verbose.  That said, perhaps getter_AddRefs is nice because in
addition to the reason you give it also helps indicate the out param.  It also
means that NS_QueryNotificationCallbacks callsites won't fit on a single line
anymore, but oh well ;-)


> I assume the mLock stuff is just unneeded cruft and all that?  That is, this 
> is general cleanup unrelated to the change at hand?

yes, useless locking.


> > nsFTPChannel::SetupState(PRUint32 startPos, const nsACString& entityID)
> 
> Is it ok that we no longer proxy the nsIAuthPrompt, nsIPrompt?	We used to.... 
> I guess you proxy the nsIFTPEventSink in the nsFtpState code itself, so that
> one's ok.

The proxies that I removed were "sync" proxies.  When a sync proxy is called on
the same thread as the destination thread, the proxy junk is bypassed.  Since
nsFTPChannel is anyways not threadsafe, there didn't seem to be much point to
creating those sync proxies.  The nsIFTPEventSink proxy is asynchronous, and is
proxied "perhaps" (as my comment in the patch indicates) to avoid weird
re-entrant possibilities (but, I'd be willing to be that the async proxy is
actually just for fun).


> If it can return null, GetTransport() is probably a better name... (I think 
> the decom interface naming convention people have mostly settled on is that 
> lack of a Get prefix means that null is never returned.)

Hmm... in Necko I leave off the "Get" prefix when I am returning a single-value
member (regardless of whether or not it may have a null value).  This is just a
matter of existing Necko conventions.  That said, I'm not sure if null is ever
really possible here.  Since the old code assumed null was possible, I just
preserved that assumption.


> Why?  I assume that call was breaking cycles or something, no?

Because there is no code that actually sets the security callbacks.  Moreover,
we don't support FTP over SSL, so again the security callbacks wouldn't be of
much use.


> Again, why is mLock no longer needed?  It looks like it was guarding against
> the listener being set from a different thread; can that never happen?	
> If so, why not?

Because this code is only ever used on the main thread.  The locking is just cruft.


> > nsGopherChannel::AsyncOpen(nsIStreamListener *aListener, nsISupports 
> 
> >-    mTransport->SetSecurityCallbacks(mCallbacks);
> 
> Why?

Same reason: no Gopher over SSL.

Comment 20

[Darin Fisher]

> That would be an interface change, wouldn't it?  See bottom of comment #14 and
> my response to biesi on this same issue in comment #17 ;-)

Actually, biesi and I only spoke about this on IRC.  We agreed that "must" is
not appropriate.  He favored "should", but I still am not so sure about that.

Comment 21

[Boris Zbarsky [:bzbarsky]]

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

(In reply to comment #19)
> That would be an interface change, wouldn't it?

True... ok, this is fine as-is, I guess.  ;)

> Remember, that this code is inlined at the callsite, so there is no codesize
> savings by implementing one in terms of the other.

Right, but I also suggested it not be inlined... I don't see this as being in
any performance-critical codepaths where the inlining would be worth it.  Or is
it a matter of all things in nsNetUtil being inlined?

Also, there's the brainprint savings from not having two pretty similar
functions.  But in the end, this is your call.

> Hmm... in Necko I leave off the "Get" prefix when I am returning a
> single-value member (regardless of whether or not it may have a null value).

Ah, ok.  If there's existing module convention, stick with it, by all means.

sr=bzbarsky with the code-level issues resolved.

Comment 22

[Darin Fisher]

> Right, but I also suggested it not be inlined... I don't see this as being in
> any performance-critical codepaths where the inlining would be worth it.  Or 
> is it a matter of all things in nsNetUtil being inlined?

Ah, yes.. there's no way to do non-inline necko utilities.  We only have a
component lib :(


> sr=bzbarsky with the code-level issues resolved.

Thanks!

Comment 23

[Darin Fisher]

> You need parens around the loadflags check (|| has higher precedence than &).

Actually, this is not correct.  bitwise AND (&) has higher precidence over
relational OR (||).

I'll add the parens anyways for clarity.

Comment 24

[Darin Fisher]

fixed-on-trunk

Comment 25

[David G King]

This checkin caused the following problem on Win32/MinGW/cygwin :-

In file included from e:/mozilla/source/mozilla/js/src/xpconnect/loader/mozJSCom
ponentLoader.cpp:70:
../../../../dist/include/necko/nsNetUtil.h: In function `void NS_QueryNotificati
onCallbacks(nsIInterfaceRequestor*, nsILoadGroup*, const nsIID&, void**)':
../../../../dist/include/necko/nsNetUtil.h:957: error: `aChannel' undeclared (fi
rst use this function)
../../../../dist/include/necko/nsNetUtil.h:957: error: (Each undeclared identifi
er is reported only once for each function it appears in.)
make[4]: *** [mozJSComponentLoader.o] Error 1
make[4]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/js/src/xpconnect/
loader'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/e/mozilla/source/mozilla/js/src/xpconnect'

make[2]: *** [libs] Error 2
make[2]: Leaving directory `/cygdrive/e/mozilla/source/mozilla'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/e/mozilla/source/mozilla'
make: *** [alldep] Error 2

Comment 26

[Darin Fisher]

david: update netwerk/base/public/nsNetUtils.h

that bustage was not particular to MingW.

Comment 27

[David G King]

Hmmm, interesting. I would have thought that CVS and a Depend build would have
updated that file? (including the variation in mozilla/dist)

Comment 28

[Darin Fisher]

> Hmmm, interesting. I would have thought that CVS and a Depend build would have
> updated that file? (including the variation in mozilla/dist)

I probably checked in the fix after you updated your tree.

Comment 29

[David G King]

OK, my compile is working now. I did notice that nsNetUtil.h seems to have been
modified twice today (16/11/2004 NZDT) (v1.85 and v1.86).

Comment 30

[timeless]

*** Bug 274981 has been marked as a duplicate of this bug. ***

Comment 31

[Ian Neal]

At least the nsBookmarksService.cpp part of this patch got lost during the
aviary landing. Is it still needed?

Comment 32

[Darin Fisher]

If it was lost, then yes it should be added back.

Comment 33

[Boris Zbarsky [:bzbarsky]]

And please do not reopen bugs due to the aviary-landing regressions...

Comment 34

[Ian Neal]

Relanded nsBookmarksService.cpp part of patch v1.2
Checking in nsBookmarksService.cpp;
/cvsroot/mozilla/browser/components/bookmarks/src/nsBookmarksService.cpp,v  <--
 nsBookmarksService.cpp
new revision: 1.54; previous revision: 1.53
done

I'll try to remember not to reopen bug if/when relanding in future

Comment 35

[Alex Fritze]

This patch is causing the regression described in bug#280808.