169257 - Not getting nsIUriContentListener callbacks using nsIWebBrowser registration

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Embedding: APIs, defect)

Description

[David Epstein]

Mozilla 1.2a Gecko 2002910. TestEmbed
nsIUriContentListener methods are implemented in BrowserImpl.cpp
1. Register nsIUriContentListener with AddWebBrowserListener() & iid. (In
TestEmbed, select Tests > Add urIContentListener > Add from nsIWebBrowser).
Listener is successfully registered. 
2. Launch embedding app and enter a url, load content, download, etc..
3. Check logfile (In TestEmbed, C:/temp/TestOutput.txt).
Result: Callbacks for the iface aren't kicking in, don't get notifications from
any of the implemented methods.

Comment 1

[Judson Valeski]

.

Comment 2

[Adam Lock]

Reassign to me while I investigate

Comment 3

[Adam Lock]

AddWebBrowserListener and RemoveWebBrowserListener have no support at all for
nsIURIContentListener. The only listeners currently supported are
nsIWebProgressListener and nsISHistoryListener. This shouldn't matter so much as
there is an explicit parentURIContentListener attribute on nsIWebBrowser to
register a URI content listener:

http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsIWebBrowser.idl#142

I could add code to internally map nsIURIContentListener onto this attribute if
called through the wrong method but I'm not sure if its worth it. David, can you
try setting the listenver via parentURIContentListener and see if that works for
you?

I have a related bug 81835 to add support for event listeners via
addWebBrowserListener.

Comment 4

[David Epstein]

Also see bug 162044 and bug 169254. I haven't been on the project for awhile so
I have an older build. Using it I was able to set the parent uriContentListener
via the web browser (also able to get the object and make an explicit call).
However, the only nsIURIContentListener notification asyncronously received is
for nsIURIContentListener->OnStartURIOpen(). No notifications are received for
content handling. They used to work fine through nsIURILoader->OpenURI() but
don't know now. Will try out on more recent build when I get a chance.

Comment 5

[Juan José Mata]

5/5 EDT triage: minusing topembed+ status.  Dropping this from the radar to
better focus on existing working set.

Comment 6

[Bill Filler]

Attachment: nsURILoader.cpp bug fix

Comment 7

[Bill Filler]

I've been working on the JRex project and have encountered this bug as well - 
that is - registering via nsIWebBrowser->SetParentContentListener() results in 
the listener not being called back correctly. I believe I have isolated and 
fixed the problem in two files uriloader/base/nsURILoader.cpp and 
docshell/base/nsDSURIContentListener.cpp.

I found that the DispatchContent() method in nsURILoader.cpp was never trying 
m_contentListener->GetParentContentListener() to see if it was interested in 
handling the request. I added the check on line 462. Also, 
nsDSURIContentListener.cpp IsPreferred() method was calling the 
GetParentContentListener().IsPreferred() method but this was incorrect (result 
of the call was never being used) and is now unecessary because the caller, 
DispatchContent(), is now correctly checking the parent listener.

I've attached both files and commented my changes. I'm not part of this 
project and don't claim to be an expert in this code, but I did step through 
with a debugger and believe my fix should be correct. I also modified 
Testembed.Tests.cpp to register via nsIWebBrowser->SetParentContentListener() 
and the test now passes (listener methods are correctly called). Can someone 
who is familiar with the code please review my changes and submit into CVS if 
they are correct - or let me know how to proceed. Thanks

Comment 8

[Bill Filler]

Attachment: nsDSURIContentListener.cpp bug fix

Comment 9

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

(In reply to comment #7)
> I've attached both files and commented my changes.

could you attach the changes in patch form, i.e.:
cvs diff -u6p uriloader docshell > patch

then attach the resulting file "patch" here.

Comment 10

[Bill Filler]

Attachment: patch for nsURILoader.cpp

here is the change in diff format

Comment 11

[Bill Filler]

Attachment: patch for nsDSURIContentListener.cpp

patch in diff format

Comment 12

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

Attachment: full patch

this attachment contains the two patches in a single file

Comment 13

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

ok... I don't know this code all that well, but it seems to be "the right
thing", generally.

however, why just look at one parent listener? why not the entire chain of
parent listeners, until there is none?

Comment 14

[Boris Zbarsky [:bzbarsky]]

This patch is effectively a pretty major api change to nsIURIContentListener
(which is frozen).  It totally changes the meaning of the IsPreferred() method...

Comment 15

[David Epstein]

Is there an alternative fix that doesn't change the API?

Comment 16

[Boris Zbarsky [:bzbarsky]]

It's not clear to me what problem we are trying to fix.  The current behavior is
exactly what the nsIURIContentListener API specifies it should be, as far as I
can tell.

Comment 17

[Bill Filler]

The problem we are trying to fix is that if you register a listener via 
nsIWebBrowser->SetParentContentListener(myListener), the myListener is not 
called back correctly. That is, myListener.onStartURIOpen() is called but 
myListener.isPreferred() myListener.canHandleContent() and myListener.doContent
() are never called. This is a bug as it prevents embedders from overriding 
how certain content types are loaded. The documentation for 
nsIWebBrowser::parentURIContentListener states: 
"Sets the URI content listener parent. Embedders may set this property to 
their own implementation if they intend to override or prevent how certain 
kinds of content are loaded."
If this is not a bug, maybe I'm completely misinterpreting the API. How then 
would an embedder go about registering a contentListener to override how 
certain content types are loaded?
Also, you state that my change is a major API change. How so? I did not change 
the API at all. And can you elaborate on how the change to IsPreferred
() "totally changes the meaning" of the method? I don't understand..

Comment 18

[Boris Zbarsky [:bzbarsky]]

First, the API change issue.  The current API, for better or for worse, is
designed as follows (this is not-too-clearly documented in
nsIURIContentListener.idl; we have some wiggle room as long as we don't break
existing users, and I would be happy to improve the documentation to make things
clearer if people can suggest better ways of saying what it's trying to say):

1)  The parent listener is notified of OnStartURIOpen
2)  The parent listener is allowed to override IsPreferred() to prevent loading
    of content of a particular type or request loading of content the docshell
    thinks it cannot handle.
3)  The parent listener cannot override CanHandleContent()
4)  The parent listener cannot override DoContent()

With your patch, the API becomes:

1)  The parent listener is notified of OnStartURIOpen
2)  The parent listener cannot override IsPreferred, hence cannot prevent
    loading of a type, but can request loading of content the docshell thinks it
    cannot handle.
3)  Same thing for CanHandleContent
4)  The parent listener cannot override DoContent()

Note the major difference in item 2 (and item 3, but item 2 is the big deal for
me).  That change actually _removes_ existing functionality and would cause
severe problems for embeddors that want to prevent loading of certain types of
content in their docshells (mailnews/thunderbird come to mind).  The patch is
not changing the IDL, sure, but we're changing the API all the same.

Now to the problem at hand.  Reading through the code, when a listener is
registered via nsIWebBrowser::SetParentURIContentListener (which is what I
assume you meant), it should get the OnStartURIOpen callback and it should get
the IsPreferred callback for "preferred" loads.  It should get no other callbacks.

The remaining question is what a "preferred" load is.  This is controlled by the
boolean passed to nsIURILoader::OpenURI; at the moment it means the load is of
type "LOAD_LINK" (so triggered by a link click, form submit, etc).

I know for a fact that IsPreferred _is_ called on the parent listener because,
as I said, thunderbird overrides it to prevent loading of various types of
content in the message pane docshell.

I do agree that not being able to override CanHandleContent may be a problem for
embedders, since some loads (eg JS setting document.location) are not of type
LOAD_LINK.  I would be fine with changing nsDSURIContentListener in such a way
that the parent listener gets a crack at overriding CanHandleContent.  I would
also be fine with changing both IsPreferred and CanHandleContent such that the
parent can subtract types but not add them (since adding will just break when we
hit DoContent anyway).  That would allow parents to stop hardcoding the "what
can docshell load?" algorithm the way they have to right now.

Thoughts?

Comment 19

[David Epstein]

Thanks for all your info on this Boris. Except for OnStartURIOpen(), I wasn't
able to get callbacks from any of the methods including IsPreferred() when I
tested this some time ago, but if it's working in Thunderbird, great. From the
documentation of nsIURIContentListener, it states that IsPreferred() should be
called for a preferred content provider, and if there isn't one, then
CanHandleContent() is called. Also DoContent() is called to activate a stream
listener to consume the data. None of this was working for me, hence this bug.

I'm not sure why a parent listener can't override either CanHandleContent() or
DoContent(). Under what conditions will these methods be called? When handled by
a content listener lower in the hierarchy, when not handled by the docshell?
While I'm not too familiar with the code, your suggestions seem reasonable.

By the way how is loadCookie attribute used?

Comment 20

[Boris Zbarsky [:bzbarsky]]

There are two separate uses of nsIURIContentListener:

1)  Set it as a parent listener of a docshell
2)  Register it with the URILoader (see nsIURILoader::registerContentListener).

In the second case, the listener will get a CanHandleContent or IsPreferred and
it it accepts the content will get a DoContent.  In the first case you get the
behavior I described in comment 18. It's unfortunate that the same interface is
used for both use cases; I would have split the setup up into two separate APIs.
 But again, the interfaces are long since frozen.

> I'm not sure why a parent listener can't override either CanHandleContent() or
> DoContent().

For the former, no reason.  For the latter, it's a question of why parent
listeners exist.  If you just want to handle content yourself instead of having
a docshell handle it, make sure that a docshell doesn't accept the content (by
setting a parent listener on it) and then register your whatever-it-is with the
URILoader.  This is why I wish there were two separate interfaces -- these are
two distinct tasks, and there's no good reason to lump them into a single place.

> Under what conditions will these methods be called?

CanHandleContent is called when looking for a listener to handle a load (modulo
the random IsPreferred complication).  DoContent is called once a listener has
been found that accepts the load.  ONLY the listener that ends up with the load
will have DoContent called.  This listener will be either the listener
associated with a docshell (_not_ its parent), a listener registered with the
URILoader, or a listener instantiated via contractid by the URILoader.

> By the way how is loadCookie attribute used?

Nastily.  ;)  Seriously, it's just a way of passing about a docloader without
calling it a docloader.  biesi has some patches in the pipe to simplify that
stuff a bit, and I suspect that once I figure out what exactly docloader is
doing and why I'll be reworking that neck of the woods...

Comment 21

[Bill Filler]

Boris,
Thanks for your explanation of what's going on. It was not apparent to me that 
there was a difference between registering a listener via 
nsIWebBrowser::SetParentContentListener() and 
nsIURILoader::registerContentListener. The code changes I made were a 
deliberate attempt to make the two work identical, that is, a listener 
registered either way would cause the listener to get a CanHandleContent or 
IsPreferred and
if it accepts the content it will get a DoContent. I believe I accomplished 
that as that's the behavior I'm seeing.

Now I understand that a ParentListener is in fact different than a a listener 
directly registered on nsIURILoader, and I agree the patch would be the wrong 
thing to do as it would change the behavior. As a user of the Embedding API, 
the bottom line to me that I need a way to handle certain content types 
myself. The only public Embedding API that I see documented for adding content 
listeners is nsIWebBrowser::SetParentContentListener()  
(http://www.mozilla.org/projects/embedding/embedapiref/embedapiTOC.html), and 
that led to my confusion. Your suggestion about needing to register both a 
parent listener and and a listener directly on the nsIURILoader in order to 
accomplish the behavior that I'm looking for is fine with me if it will work. 
But how come nsIURILoader is not documented as part of the API doc? Is it 
legit for me to use it as an embedder? Also the documentation for 
nsIWebBrowser::SetParentContentListener() should explain the purpose of 
a "Parent Listener" better and describe how to handle content types via 
nsIURILoader because I think it's really a cause for confusion.

I know the API's are frozen for now. What would seem intuitive to me would be 
adding a  nsIWebBrowser::RegisterContentListener() method that would simply 
give the registered listener the ability to handle specified content types 
(i.e. the equivalent of adding a parentListener and nsIURILoader 
contentListener).

Comment 22

[Boris Zbarsky [:bzbarsky]]

> But how come nsIURILoader is not documented as part of the API doc?

Largely because it's not frozen yet... I think we should move toward freezing
it, and I'm fairly certain the content listener registration will not change
from this point forward.

The interface as a whole may change a bit, so using it if you want binary compat
across multiple Mozilla versions is tough.  If that is the case, it may be
possible to just register the contractid for your listener with the category
manager under the "external-uricontentlisteners" category for the type you want
to handle with that listener.

I agree that it would be great if you could register a listener on a web browser
and just have stuff delegated to it if the docshell cannot handle it.  It's
unfortunate that this API was not designed that way to start with.

Comment 23

[David Epstein]

> Register it with the URILoader (see nsIURILoader::registerContentListener).
I submitted bug 162044 about this. Initially wasn't receiving notifications, but
with RegisterContentListener() registration, and downloading docs, I was getting
notifications for IsPreferred() and then DoContent(). 

Another approach is to register with RegisterContentListener(), and as I noted
in Comment #4, you could use nsIURILoader->OpenURI(). That's what I used as a
testing baseline, and callbacks should work fine, but you'll be loading urls
under the hood (with a channel input) and content listening will be associated
with the loader not the parent listener ... The main concern is that whatever
method is invoked, we get a callback for IsPreferred().

Comment 24

[David Epstein]

Boris, can we assign this bug to you? What's happening with it?

Comment 25

[Boris Zbarsky [:bzbarsky]]

David, once we come to a decision on what we want to do, exactly, I'll be happy
to implement it.  We're just talking about making life easier for embeddors
somehow at this point, right?

Comment 26

[David Epstein]

Yes, implementations that will help the embeddor using the parent listener
approach, so long as notifications are received. Your suggestions in the last
paragraph of Comment #18 appear to do that. Thanks.

Comment 27

[Boris Zbarsky [:bzbarsky]]

darin?  biesi?  Thoughts on that suggestion?

Comment 28

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

(In reply to comment #18)
> I would be fine with changing nsDSURIContentListener in such a way
> that the parent listener gets a crack at overriding CanHandleContent.  I would
> also be fine with changing both IsPreferred and CanHandleContent such that the
> parent can subtract types but not add them (since adding will just break when we
> hit DoContent anyway).  That would allow parents to stop hardcoding the "what
> can docshell load?" algorithm the way they have to right now.

That seems like a good suggestion. But how does your last sentence follow? Oh,
would IsPreferred/CanHandleContent just return always true, and docshell uses it
only if it can handle it?


you have to be careful w/ CanHandleContent probably if embeddors now get it but
previously didn't... they may just have it always fail, or always return success
w/o changing out params...

Comment 29

[Boris Zbarsky [:bzbarsky]]

> But how does your last sentence follow? Oh, would IsPreferred/CanHandleContent
> just return always true

Exactly.

Comment 30

[Boris Zbarsky [:bzbarsky]]

Darin?  Thoughts?  I'm worried that we're proposing de-facto API changes for
nsIURIContentListener....

Comment 31

[Christophe Cornu]

To capture a discussion with Darin and Boris:
The final solution for this bug report would also ideally not require 
embedders to use unfrozen API to implement nsIURIContentListener.
At the moment, Mozilla embedded samples overriding IsPreferred (e.g. 
GtkMozembed ...) rely on the unfrozen nsICategoryManager.GetCategoryEntry
("geck-content-viewer") to determine what content is supported or not.

Comment 32

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

one problem with the suggested solution:
if an embeddor claims being able to handle a certain type, and handles it by
aborting the load in DoContent and calling LoadStream with some data, then that
use case would be broken...

Comment 33

[Boris Zbarsky [:bzbarsky]]

OK, per discussion with biesi, there's really nothing more we can do with
nsIURIContentListener without breaking api compat.  So the current plan is:

1)  Don't touch anything that involves nsIURIContentListener
2)  Create a service (Frozen) that will scan plugins as needed and check for a
    type in the gecko category.  This can be used by embeddors instead of
    manually enumerating the category.
3)  Create nsIURIContentListener2 to address this and the other issues we've run
    into with nsIURIContentListener
4)  Freeze nsIURIContentListener2 for 1.9.

Darin, does that seem ok to you?

(Note that if people agree I will mark this bug wontfix accordingly).

Comment 34

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

random note: That service should keep stream converters in mind, imo. i.e. if we
have a streamconverter that converts from the given mime type to anything, we
should claim to support that type, probably.

Comment 35

[Benjamin Smedberg]

Marking a bunch of bugs in the "Embedding: APIs" component INCOMPLETE in preparation to archive that component. If I have done this incorrectly, please reopen the bugs and move them to a more correct component as we don't have "embedding" APIs any more.