293350 - Allow dynamic changes of notification callbacks (was: Don't show download status in status bar (since it shows in the DL manager))

Status: VERIFIED FIXED

(Core :: Networking, defect)

Description

[timeless]

build id: 2005021808 (v0.8+)
steps:
0. load tabs for gmail.com, bugzilla.mozilla.org, ubuntu.com
1. ask camino to download two isos from us.releases.ubuntu.com
1' camino's downloads window merrily displays progress for the two downloads
2. read gmail and poke bugzilla.mozilla.org in tabs in my main camino window
(from which i started the iso downloads)

actual (unexpected results): the status bar in the tabbed camino browser window
says "Transferring data from us.releases.ubuntu.com"

I asked Wevah#camino, and wevah said: yes, that does happen :/

Comment 1

[Jasper]

Yeah I saw this aswell. I think this mightbe a regression of some sort.

Comment 2

[Echidnae]

This only happens for me if I'm browsing in the tab that I started the download
in.  All other tabs display normally for me.

Comment 3

[Mike Pinkerton (not reading bugmail)]

if this really is a regression, it might be worth looking at, but personally i'd
say this is minor enough that it can be pushed to 1.0 in a pinch.

Comment 4

[Mike Pinkerton (not reading bugmail)]

i've seen this a lot this weekend, boy is it annoying.

Comment 5

[Josh Aas]

marking 0.9 for now

Comment 6

[Samuel Sidler (old account; do not CC)]

For the record, using Firefox 1.0.4 on Windows 2003, I can see this same issue.
I'm not sure if it's related, but it seems to be exactly the same.

Comment 7

[Simon Fraser [no longer active]]

Is that a more comprehensible summary?

Comment 8

[timeless]

smfr: except it doesn't accurately summarize the problem. the download status
*is* shown *only* in the tab that started the download. it shouldn't show in
*any* tabs, since the download manager window owns the status.

Comment 9

[Josh Aas]

Removing regression keyword as we have no evidence of that in the bug. Feel free
to put it back if there is evidence found.

This is an annoying bug but I don't think it should block a release. Upping its
severity since we should fix it soon, but bumping it to 1.0.

Comment 10

[Nick Kreeger]

Will take a look at while working on download manager tweaks

Comment 11

[Nick Kreeger]

Same problem occurs in deer park alpha 2, If you download something in another
tab than select some other tabs, it still says something like "transering data
from XX.XX.XXX". Maybe regression isn't key here, and it is a gecko issue?

Comment 12

[Simon Fraser [no longer active]]

Yeah, this certainly isn't an easy camino fix, I think.

Comment 13

[Mike Pinkerton (not reading bugmail)]

if it's a core bug, over to core....

Comment 14

[Mike Pinkerton (not reading bugmail)]

hrm, docshell probably isn't the right component, but i'm not sure what is. it's
not an app-level bug (since it's in both camino and firefox), but it's not
really a necko bug either.

ideas on component?

regardless, this is a regression from the 1.7branch. 

Comment 15

[Stephen Donner [:stephend] Not actively reading bugmail]

wheeee...

*** This bug has been marked as a duplicate of 250295 ***

Comment 16

[Samuel Sidler (old account; do not CC)]

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

Comment 17

[Samuel Sidler (old account; do not CC)]

-> Camino

Comments to follow.

Comment 18

[Samuel Sidler (old account; do not CC)]

Alright, here's the deal. I tracked this down and this regressed in Camino only
on 11/16/04.

There are no Camino-specific checkins that would have caused this, however,
talking with stephend, we found a checkin for bug 261083 which "without a doubt"
caused this. The query we used is below.

So, this is *not* the core bug and can be fixed on our end (at least I hope) but
will not fix the core problems.

CCing darin since it was his checkin. Also, does this need to be in for 1.8b4?


http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=%2Fmozilla&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2004-11-15&maxdate=2004-11-17&cvsroot=%2Fcvsroot

Comment 19

[Mike Pinkerton (not reading bugmail)]

my gut feeling is to keep this in Core so it can get a 18b blocking flag and
stays on their radar. If we can't get permission for the branch, it matters
little if it blocks us.

Comment 20

[Asa Dotzler [:asa]]

Darin, can you have a look at this?

Comment 21

[Mike Schroepfer]

Can you fix it on the camino side and file a new bug for any core issues?

Comment 22

[Samuel Sidler (old account; do not CC)]

(In reply to comment #21)
> Can you fix it on the camino side and file a new bug for any core issues?
> 
> 

No, because a core checkin caused the problems. We can't fix this without fixing
the core bug (that I know of).

Please re-evaluate blocking1.8b4.

Comment 23

[Josh Aas]

Note: You do not need to have a blocking flag set in order to get a patch
approved for the branch.

Comment 24

[Samuel Sidler (old account; do not CC)]

-> Core. Re-requesting nomination per IRC conversation with schrep.

Comment 25

[Boris Zbarsky [:bzbarsky]]

The problem is the immutability of notification callbacks that bug 261083
introduced.  It means that the code in
nsExternalAppHandler::RetargetLoadNotifications becomes a no-op, and as a result
the load delivers progress and status notifications to the window that started
it, since that's the callback it has for progress and status events.

I do think this should block 1.8b4, since fixing this may require yet another
effective nsIChannel API change of the sort that bug 261083 made.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Further note: bug 261083 comment 11 is wrong -- since the channel used to be in
the loadgroup it DOES have a request info in the docloader.  We don't remove
request infos till the next pageload starts or all loads stop as far as I can
see.  biesi suggested removing them in OnStopRequest.

Comment 27

[Darin Fisher]

I think the best solution for this bug is to make all of the code lazily query
notification callbacks, and then clear out any cached interfaces when the
notification callbacks are set.  I'll develop a patch for this shortly...

Comment 28

[Darin Fisher]

Attachment: prototype patch for http channel

Comment 29

[Darin Fisher]

Comment on attachment 194434 [details] [diff] [review]
prototype patch for http channel

biesi: can you look this over to see if you agree with this approach?  if so,
i'll do the same for the rest of the channel implementations.  thx!

Comment 30

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

Comment on attachment 194434 [details] [diff] [review]
prototype patch for http channel

hm, yeah, this does make sense.

Now, nsIChannel still documents that channels may ignore changing the
callbacks. Should nsDocLoader deal with that? Maybe be setting a flag on the
RequestInfo object in its OnStopRequest implementation and ignoring further
notifications for that object?

Comment 31

[Darin Fisher]

biesi:

I believe I added that comment last November.  So, we haven't exactly shipped a
release with that comment made explicit.  I think we could massage that
statement into something like this:

  NOTE: A channel implementation should take care when caching an interface
  queried from the notification callbacks.  If the notification callbacks
  are changed, then a cached interface may become invalid and need to be
  re-queried.

I think I'd be happier adding this requirement even though it was not
implemented in Gecko 1.7 or earlier.  What do you think?

The RequestInfo idea sounds pretty good too in case we want to be absolutely
sure that no "rogue" channel can re-introduce this bug.  But, maybe that is not
a huge concern.  IIRC, nsIProgressEventSink changed since Gecko 1.7 anyways.

Comment 32

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

yeah, the nsIProgressEventSink change is a good point. I guess other channel
impls don't often need other interfaces (nsIAuthPrompt maybe? but then, that
doesn't really need dynamic changes). ok, just changing the nsIChannel docs
seems ok.

Comment 33

[Darin Fisher]

Alright.. sounds good.  I'll post a final patch shortly.

Comment 34

[Darin Fisher]

Attachment: v1 patch

Fixes most channels.  I left out changes to nsMsgProtocol and nsLDAPChannel
because 1) they don't apply to this bug and 2) it isn't obvious how to fix them
or if it is even necessary to fix them.  nsMsgProtocol for instance may have
mProgressEventSink initialized in its constructor, and I'm not sure if it is
okay to clear that value when SetNotificationCallbacks or SetLoadGroup is
called.  nsLDAPChannel uses multiple threads, and needs to query the
notification callbacks up front so that it can be build a proxy object that its
background thread will use.  Seems like it would be pretty complicated to teach
it how to query its notification callbacks lazily.

Comment 35

[Boris Zbarsky [:bzbarsky]]

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

>Index: netwerk/base/public/nsIChannel.idl
>+     * listener as it becomes available.  The stream listener's methods are
>+     * called via an event dispatched through the event queue service on the
>+     * same thread that called asyncOpen.

How about just:

  The stream listener's methods are called on the thread that
  asyncOpen was called on.

because if a channel runs entirely on one thread there is no pressing need for
it to call these methods via an event, really...

Unless you're trying to emphasize that asyncOpen will return before
OnStartRequest is called, in which case we should perhaps just state that
explicitly.

The rest looks great.  sr=bzbarsky.

Comment 36

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

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

Hrm, as I now realize, this means that if notification callbacks do not give
out a progress event sink, it gets queried on each ODA call...

Maybe the LSB of the pointer could store a flag whether it should be
regotten. Or am I worrying too much about perf here?

nsFTPChannel.cpp:
hrm... is OnStatus here always called on the thread asyncOpen was called on?
(I am especially concerned about the caller in StopProcessing via Process)
And what about FtpState's mPrompter/mAuthPrompter, that doesn't seeem to get
reset?

nsGopherChannel.cpp:
 nsGopherChannel::SetLoadFlags(PRUint32 aLoadFlags)
 {
     mLoadFlags = aLoadFlags;
+    mProgressSink = nsnull;

SetLoadFlags? Wrong function? ;)


I'm not sure that ignoring mailnews is great here, because it does trigger
downloads (mail attachments). But then, I guess status from them rarely ends
up in a status bar...

Comment 37

[Darin Fisher]

>   The stream listener's methods are called on the thread that
>   asyncOpen was called on.

Yeah, I like that wording combined with your suggested phrase "that asyncOpen
will return before OnStartRequest is called."


> because if a channel runs entirely on one thread there is no pressing need for
> it to call these methods via an event, really...

No, the whole event queue business is important.  It's important that we make
some promise of actually calling OnStartRequest asynchronously after asyncOpen
returns.  But, perhaps actual mention of "event queues" is best avoided.  That's
why I like your suggested wording.

Comment 38

[Darin Fisher]

> Hrm, as I now realize, this means that if notification callbacks do not give
> out a progress event sink, it gets queried on each ODA call...
> 
> Maybe the LSB of the pointer could store a flag whether it should be
> regotten. Or am I worrying too much about perf here?

Hmm... yeah, I think we can punt on this for now.  I doubt there is much
overhead to this, and most of the critical paths (like webpage loading) will
have a progress sink.  At some point, it might be nice to create a helper class
that encapsulates this optimization though... or encapsulate it in a
nsBaseChannel implementation ;-)



> nsFTPChannel.cpp:
> hrm... is OnStatus here always called on the thread asyncOpen was called on?

Yes, it is, and it had better be :-)


> (I am especially concerned about the caller in StopProcessing via Process)
> And what about FtpState's mPrompter/mAuthPrompter, that doesn't seeem to get
> reset?

Yeah, the same problem exists with the nsIFTPEventSink object.  I should
probably clean that up too.



> nsGopherChannel.cpp:
>  nsGopherChannel::SetLoadFlags(PRUint32 aLoadFlags)
>  {
>      mLoadFlags = aLoadFlags;
> +    mProgressSink = nsnull;
> 
> SetLoadFlags? Wrong function? ;)

Crap, I meant SetLoadGroup :(  Thanks!


> I'm not sure that ignoring mailnews is great here, because it does trigger
> downloads (mail attachments). But then, I guess status from them rarely ends
> up in a status bar...

I'd rather do the mailnews changes in a separate patch at least because I have
less confidence in that area of the code.

Thanks for the review comments.  I'll post a revised patch shortly.

Comment 39

[Darin Fisher]

Attachment: v1.1 patch

This patch includes the following changes:

 * revised text for asyncOpen documentation in nsIChannel.idl
 * revised nsGopherChannel.cpp to fix SetLoadFlags vs SetLoadGroup thinko
 * revised nsFTPChannel.cpp & nsFtpConnectionThread.cpp to resolve outstanding
   issues there with the prompters and FTP eventsink

Biesi, can you please look this over.  Thanks!

Comment 40

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

netwerk/protocol/ftp/src/nsFTPChannel.cpp
-    return mEventSink->OnStatus(this, mUserContext, aStatus,
+    return mProgressSink->OnStatus(this, mUserContext, aStatus,
                                 NS_ConvertUTF8toUTF16(host).get());

wrong indentation here

-    return mEventSink->OnProgress(this, mUserContext, 
+    return mProgressSink->OnProgress(this, mUserContext, 
                                   aProgress, aProgressMax);

and here...

nsFtpConnectionThread.cpp:
+            mChannel->GetCallback(prompter);
+            if (!prompter)
+                return NS_ERROR_NOT_INITIALIZED;

That error code is kinda not ideal...

-        NS_ASSERTION(mPrompter, "no prompter!");

nice :)

Comment 41

[Darin Fisher]

fixed-on-trunk

biesi: I kept NS_ERROR_NOT_INITIALIZED for lack of a better error code.

Comment 42

[Vidar Haarr (not reading bugmail)]

(In reply to comment #41)
> biesi: I kept NS_ERROR_NOT_INITIALIZED for lack of a better error code.

Wouldn't NS_ERROR_NOT_AVAILABLE be better?

Comment 43

[Darin Fisher]

> > biesi: I kept NS_ERROR_NOT_INITIALIZED for lack of a better error code.
> 
> Wouldn't NS_ERROR_NOT_AVAILABLE be better?

Would it?  I considered that error code too, but I figured that neither really
seemed better than the other.  I also considered NS_ERROR_NO_INTERFACE.  But,
finally I thought that if someone really cared about the error code, then they
might care that it not change.  Remember that this error code will be seen by
the Stream Listener's OnStopRequest method, so from that point of view
NS_ERROR_NOT_INITIALIZED seems to make some sense (i.e., the data could not be
fetched because something wasn't initialized properly).

Comment 44

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

looks like this caused bug 308018

Comment 45

[Darin Fisher]

Comment on attachment 195204 [details] [diff] [review]
v1.1 patch

Requesting approval to land this patch on the 1.8 branch w/ biesi's small
addition in bug 308018.

Comment 46

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED on trunk using Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US;
rv:1.9a1) Gecko/20050913 Firefox/1.6a1

Comment 47

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Sorry for the noise, but is there a timetable for landing this on the branch?

Comment 48

[Darin Fisher]

I'll be landing this on the 1.8 branch shortly.  It unfortunately slipped off my
radar until now :-(

Comment 49

[Darin Fisher]

fixed1.8

Comment 50

[Samuel Sidler (old account; do not CC)]

Verified fixed on the branch in Camino: Version 2005092804 (1.0a1+). Adding keyword.