Closed Bug 65925 Opened 24 years ago Closed 24 years ago

nsIWebProgressListener registration is ambiguous.

Categories

(Core :: DOM: Navigation, defect, P1)

x86
Windows 2000
defect

Tracking

()

RESOLVED FIXED
mozilla0.8

People

(Reporter: jud, Assigned: jud)

References

Details

(Keywords: embed)

Attachments

(5 files)

Currently an embeddor's nsIWebProgressListener get's implicitly registered when it sets itself as the webbrowser chrome; see http://lxr.mozilla.org/mozilla/source/embedding/browser/webBrowser/nsDocShellTre eOwner.cpp#602 This is out of sync w/ nsIWebBrowser::AddWebBrowserListener() semantics which call for nsIWebProgressListener registration to occur using AddWebBrowserListener(). The implicit registration happening in SetWebBrowserChrome is wrong. Embeddor's need to register via AddWebBrowserListener.
ugh. I just gutted (#if 0'd) the new AddWebBrowserListener() routines because it was causing double registration of listeners.
Blocks: 64833
The first patch removes the nsIWebProgressListener impl from the nsDocShellTreeOwner. It breaks Adam's mouse listener registration as he was piggy backing that binding to one of the listener callbacks which I removed. I'm not sure how to add the mouse listener connection back in. I removed this impl because the docshelltreeowner no longer acts as an intermediary between the embeddor's listener impl and the uri loader which fires the actual callbacks. Instead, the embeddor needs to register explicitly to receive the callbacks directly. I see three problems w/ my patch: 1. It breaks the mouse listener stuff (this seems resolvable). 2. It requires the embeddor to register *after* window creation (nsIBaseWindow->Create()). Trying to register before window creation will fail because we haven't constructed the underlying docshell which provides the nsIWebProgress interface for nsIWebProgressListener registration. 3. Removing a listener is not possible after you destroy the window, thus it forces the embeddor to un-register (RemoveWebBrowserListener()) *before* calling destroy. That seems anal. Maybe #2 and #3 above are reasonable?
Keywords: embed
the AddWebBrowserListener impls are back in. I shouldn't have if def'd them out to begin w/. there was no double registration as no-one's currently using these methods. There is the double reg. problem if someone starts using them though.
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → mozilla0.8
After talking this through w/ Conrad, we came up w/ the following. 1. nsDocShellTreeOwner will become a nsIWebProgressListener again so it can continue to leverage those callbacks for mouselistener registration. 2. Add|RemoveWebBrowserListener() will maintain a list of registration objects (the nsISupports*, and IID registered) and cache them until the window is Create()ed (at which point we can access a nsIWebProgress to do the real registration). Once the list has been "registered" it will be cleaned up. 3. Embeddor's registration objects will need to support nsSupportsWeakRef so when they go away, we don't try to send them listener callbacks.
disregard the patches on 01/18/01. I'm attatching two patches: one which modifies the docloader (the current implementor of nsIWebProgress) per the # suggestion above. The 2nd patch modifies the webbrowser listener registration mechanism to do the #2 suggestion above. I have not converted all of the "listeners" themselves (usually the chrome implementors in the embeddor code) as I want to run this code by everyone first. It forces the embeddors to do #1 above (support weak references). You'll note that the weak reference support in the docloader doesn't come for free, there are several more function calls to accomplish the weak reference usage. The alternative to the weak reference model is to maintain the list of listeners in the docshell throughout the lifetime of the webshell. Pros to weak ref model: 1. The embeddor can remove itself as a listener anytime it wants, explicitly, using nsIWebBrowser::RemoveWebBrowserListener(), or by destroying it's listener object. Cons to weak ref model: 1. extra function calls, little more heavyweight. 2. requires embeddor listener objects to support nsWeakReference (no impl involved, just an inheritance addition). thoughts?
For reference, here's how easy it is to make a listener object support weak reference. It's obviously trivial. Index: WebBrowserChrome.h =================================================================== RCS file: /cvsroot/mozilla/embedding/tests/winEmbed/WebBrowserChrome.h,v retrieving revision 1.3 diff -u -4 -r1.3 WebBrowserChrome.h --- WebBrowserChrome.h 2000/12/13 13:47:16 1.3 +++ WebBrowserChrome.h 2001/01/20 00:47:34 @@ -36,9 +36,11 @@ #include "nsIInterfaceRequestor.h" #include "nsIPrompt.h" #include "nsIWebBrowser.h" #include "nsVoidArray.h" +#include "nsWeakReference.h" + class WebBrowserChromeUI { public: virtual nativeWindow CreateNativeWindow(nsIWebBrowserChrome* chrome) = 0; @@ -59,9 +61,10 @@ class WebBrowserChrome : public nsIWebBrowserChrome, public nsIWebProgressListener, public nsIBaseWindow, // public nsIPrompt, - public nsIInterfaceRequestor + public nsIInterfaceRequestor, + public nsSupportsWeakReference { public: WebBrowserChrome(); virtual ~WebBrowserChrome();
Jud, in nsDocLoaderImpl::AddProgressListener: + nsWeakPtr listener = getter_AddRefs(NS_GetWeakReference(aListener)); ... + rv = mListenerList->AppendElement(listener) ? NS_OK : NS_ERROR_FAILURE; After AppendElement(), mListenerList doesn't end up with a strong ref to aListener, does it?
Applied the patches, built and tested with my embedding app. My strong ref question was answered (It does the right thing - no strong ref). Another thing to note - making the object support weak reference requires no only mixing in nsWeakReference, but also having nsISupportsWeakReference in the IMPL_ISUPPORTS stuff. r=ccarlen
Hmmm... So I would like to register a nsIWebProgressListener JS object, but to do that the listener list needs to hold strong references, otherwise the native wrapper object around the JS object goes away right after AddProgressListener (leaving you with a dangling pointer and a crash soon to follow)...
Actually, I want weak references, thanks to jband for pointing that out in bug 59246, and as soon as JS supports that (bug 66950) I'll be all set.
Attached patch complete tree diffs. (deleted) — Splinter Review
The 1/30/01 09:53 patch is a diff -u of the following changes: - webbrowser/webbrowserchrome changes to accommodate listener registration (r=ccarlen) - nsDocLoader changes to store weakpointer refs to listeners (r=ccarlen) - changes to make everyone who registers as a webProgressListener to support weak references. (needs review).
Keywords: review
Blocks: 59246
> - changes to make everyone who registers as a webProgressListener to support > weak references. (needs review). The change to /mozilla/extensions/psm-glue/src/nsSecureBrowserUIImpl.cpp has the additional bonus of fixing bug 63552. WRT that, look at http://lxr.mozilla.org/seamonkey/source/xpfe/browser/src/nsBrowserInstance.cpp#519 That code (wrongly) depends on the fact the a strong ref is going to get attached to the nsSecureBrowserUI during its Init() and now it won't. That code (nsBrowserinstance) needs to to hold the owning ref now. Not even sure if it's used but deserves a look.
r=ccarlen, and sr=rpotts.
Keywords: review
fixes are in. thanks all!
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
shucks, I wish I knew about this change. There are several web progress listeners that are implemented purely in JS. None of these were changed to support weak references. This is causing my progress download dialog to break for large transfers. Please see Bug #67481! Thanks to Blake for making the connection. I've been banging my head on this like crazy. My head hurts.
No longer blocks: 64833
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: