nsDocLoader comparing weakrefs

RESOLVED FIXED

Status

()

Core
Document Navigation
RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: jhp (no longer active), Assigned: Adam Lock)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

3.89 KB, patch
Benjamin Smedberg
: review+
Darin Fisher
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

13 years ago
The function nsDocLoaderImpl::GetListenerInfo compares its parameter (an
nsIWeakReference*) to a saved nsWeakPtr.  But this seems wrong to me.  The two
weakref instances come from different calls to |do_GetWeakReference|, so how
does it work if we compare them to each other?  Should we not be comparing the
referent?

For me, at least, it is causing a failure, since the weakrefs never match up,
and nsWebBrowser::RemoveWebBrowserListener() returns an error (since
nsDocLoaderImpl::RemoveProgressListener() failed).  I was able to work around
this by getting the referent for each weakref and comparing those, but I'm not
sure if that is the right solution.

Comment 1

13 years ago
That is the correct solution. Be sure that you get the referent as nsISupports*,
not any other interface*, since only nsISupports* is guaranteed to have
pointer->object identity per the COM rules.

Comment 2

13 years ago
> That is the correct solution. Be sure that you get the referent as nsISupports*,
> not any other interface*, since only nsISupports* is guaranteed to have
> pointer->object identity per the COM rules.

nsIWebProgressListener should be just as good... there cannot be two different
nsIWebProgressListener pointers to the same object.  IOW, it should be enough to
take aListener passed to Add/RemoveProgressListener and compare that to the
result of QueryReferent(NS_GET_IID(nsIWebProgressListener)) on the list of
stored progress listeners weak refs.

Comment 3

13 years ago
I stand corrected.  A tearoff could be used to implement nsIWebProgressListener,
so it is necessary to compare nsISupports instances as bsmedberg said.  My bad.
(Reporter)

Comment 4

13 years ago
Created attachment 162745 [details] [diff] [review]
patch
(Reporter)

Updated

13 years ago
Attachment #162745 - Flags: review?(bsmedberg)

Updated

13 years ago
Attachment #162745 - Flags: review?(bsmedberg) → review+

Comment 5

13 years ago
Comment on attachment 162745 [details] [diff] [review]
patch

sr=darin
Attachment #162745 - Flags: superreview+
(In reply to comment #3)
> I stand corrected.  A tearoff could be used to implement nsIWebProgressListener,

or code could implement two interfaces which both derive from nsIWPL - then you
could also have different nsIWPL pointers for the same object.

Comment 7

13 years ago
> or code could implement two interfaces which both derive from nsIWPL - then you
> could also have different nsIWPL pointers for the same object.

true true!
(Reporter)

Comment 8

13 years ago
Checked into trunk.
Status: NEW → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.