Closed
Bug 65925
Opened 24 years ago
Closed 24 years ago
nsIWebProgressListener registration is ambiguous.
Categories
(Core :: DOM: Navigation, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla0.8
People
(Reporter: jud, Assigned: jud)
References
Details
(Keywords: embed)
Attachments
(5 files)
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
patch
|
Details | Diff | Splinter Review | |
changes to our nsIWebProgress listener implementation (nsDocLoader) to support weak reference usage.
(deleted),
patch
|
Details | Diff | Splinter Review | |
(deleted),
text/plain
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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•24 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•24 years ago
|
||
Assignee | ||
Comment 3•24 years ago
|
||
Assignee | ||
Comment 4•24 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 | ||
Comment 5•24 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•24 years ago
|
Severity: normal → critical
Priority: -- → P1
Target Milestone: --- → mozilla0.8
Assignee | ||
Comment 6•24 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•24 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•24 years ago
|
||
Assignee | ||
Comment 9•24 years ago
|
||
Assignee | ||
Comment 10•24 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();
Comment 11•24 years ago
|
||
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?
Comment 12•24 years ago
|
||
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•24 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•24 years ago
|
||
See bug 59246.
Comment 15•24 years ago
|
||
Assignee | ||
Comment 16•24 years ago
|
||
Assignee | ||
Comment 17•24 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
Comment 18•24 years ago
|
||
> - 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 20•24 years ago
|
||
fixes are in. thanks all!
Status: NEW → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Comment 21•24 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.
You need to log in
before you can comment on or make changes to this bug.
Description
•