265289 - nsDocLoader comparing weakrefs

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect)

Description

[jhp (no longer active)]

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

[Benjamin Smedberg]

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

[Darin Fisher]

> 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

[Darin Fisher]

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

Comment 4

[jhp (no longer active)]

Attachment: patch

Comment 5

[Darin Fisher]

Comment on attachment 162745 [details] [diff] [review]
patch

sr=darin

Comment 6

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

(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

[Darin Fisher]

> 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!

Comment 8

[jhp (no longer active)]

Checked into trunk.