If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

nsIWebProgressListener registration is ambiguous.

RESOLVED FIXED in mozilla0.8

Status

()

Core
Document Navigation
P1
critical
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Judson Valeski, Assigned: Judson Valeski)

Tracking

({embed})

Trunk
mozilla0.8
x86
Windows 2000
embed
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

17 years ago
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.
(Assignee)

Comment 1

17 years ago
ugh. I just gutted (#if 0'd) the new AddWebBrowserListener() routines because it 
was causing double registration of listeners.
Blocks: 64833
(Assignee)

Comment 2

17 years ago
Created attachment 22958 [details] [diff] [review]
these changes remove the treeOwner as an intermediary listener (between the uriLoader and the final listener).
(Assignee)

Comment 3

17 years ago
Created attachment 22959 [details] [diff] [review]
these changes have the embedding app explicitly register their listener. note that registration only works *after* the window is created.
(Assignee)

Comment 4

17 years ago
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?
(Assignee)

Updated

17 years ago
Keywords: embed
(Assignee)

Comment 5

17 years ago
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.
(Assignee)

Updated

17 years ago
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → mozilla0.8
(Assignee)

Comment 6

17 years ago
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.
(Assignee)

Comment 7

17 years ago
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?
(Assignee)

Comment 8

17 years ago
Created attachment 23027 [details] [diff] [review]
changes to our nsIWebProgress listener implementation (nsDocLoader) to support weak reference usage.
(Assignee)

Comment 9

17 years ago
Created attachment 23028 [details]
changes to the webbrowser and webbrowser chrome.
(Assignee)

Comment 10

17 years ago
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

Comment 13

17 years ago
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)...

Comment 14

17 years ago
See bug 59246.

Comment 15

17 years ago
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.
(Assignee)

Comment 16

17 years ago
Created attachment 23832 [details] [diff] [review]
complete tree diffs.
(Assignee)

Comment 17

17 years ago
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

Updated

17 years ago
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.
(Assignee)

Comment 19

17 years ago
r=ccarlen, and sr=rpotts.
Keywords: review
(Assignee)

Comment 20

17 years ago
fixes are in. thanks all!
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED

Comment 21

17 years ago
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. 

Updated

16 years ago
No longer blocks: 64833
You need to log in before you can comment on or make changes to this bug.