Closed Bug 234858 Opened 21 years ago Closed 20 years ago

do_GetWeakReference isn't typesafe

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: sicking, Assigned: sicking)

Details

Attachments

(1 file)

Patch to fix
11.08 KB, patch
dbaron
: review+
bryner
: superreview+
Details | Diff | Splinter Review 
Currently doing:

nsCOMPtr<nsIFoo> foo = do_GetWeakReference(expr);

will compile just fine but crash severly when executed. The reason is that
do_GetWeakReference return an nsCOMPtr_helper which ignores the IID argument
passed to it and assumes that the resulting pointer is an nsIWeakReference.

The fix for this is to not return an nsCOMPtr_helper. This is compleatly
unneccesary since the result of do_GetWeakReference doesn't depend on what we're
assigning into anyway. Returning an already_AddRefed<nsIWeakReference> is
faster, smaller and typesafe.

Unfortunatly this means that doing:
nsCOMPtr<nsISupports> sup = do_GetWeakReference(expr);

won't compile. However I think the benefit of typesafty outweighs this. And we
currently only do this at 4 places in the tree.

Ideally it would be possible to assign an already_AddRefed<nsIBar> to an
nsCOMPtr<nsIFoo> of nsIBar extens nsIFoo. I'm not sure if it's possible to write
up the template-magic to get that to work though. I'll investigate.

Patch to make do_GetWeakReference return an already_AddRefed comming up.
Comment on attachment 141721 [details] [diff] [review]
Patch to fix

oh, i also made nsWeakReference use appropriate macros to implement nsISupports
rather then having them 'manually' implemented.
Attachment #141721 - Flags: superreview?(dougt)
Attachment #141721 - Flags: review?(dbaron)
It looks somewhat like the ability to do nsCOMPtr<nsISupports> =
do_GetWeakReference(...) was a feature rather than a bug.  Why not leave support
for that, but just add an assertion that QI to the given IID yields the same
pointer?

Do you expect any binary compatibility issues?
  
Hrm, i guess there will be binary compatibility issues since libaries linking
against XPCOM will expect the nsGetWeakReference symbols there. I guess I could
always leave that class there (and move it to xpcom/obsolete) and just not use it.

Sounds ok?

It might well have been that support for
nsCOMPtr<nsISupports> sup = do_GetWeakReference(...)
was a feature rather then a bug, but since it's used at exactly 4 places in our
codebase it would IMHO be more imporatant with typesafty.

If we really want that feature we should make it possible to do.
nsCOMPtr<nsIFoo> foo = already_AddRefed<nsIBar>(...)
when nsIBar extends nsIFoo. This shouldn't be all that hard to implement though
the template magic might be more then some compilers can handle just yet.
(In reply to comment #4)
> It might well have been that support for
> nsCOMPtr<nsISupports> sup = do_GetWeakReference(...)
> was a feature rather then a bug, but since it's used at exactly 4 places in our
> codebase it would IMHO be more imporatant with typesafty.
> 
> If we really want that feature we should make it possible to do.
> nsCOMPtr<nsIFoo> foo = already_AddRefed<nsIBar>(...)
> when nsIBar extends nsIFoo. This shouldn't be all that hard to implement though
> the template magic might be more then some compilers can handle just yet.

There's always been a difference between nsCOMPtr_helper implementations, which
are (in general, at least), intended to be able to used with an nsCOMPtr of any
appropriate type, and functions returning already_AddRefed<T>, which work only
with an nsCOMPtr of a specific type.  I guess the question is which we want this
to be.  The reason to prefer the former is that it makes the weak reference
implementation a little bit less clunky to use...
I agree that there's a difference between functions returning nsCOMPtr_helpers
and functions returning already_addrefed. And IMHO do_GetWeakreference should be
the latter since that's what we're really doing (since we ignore the IID argument).

I don't see what's more cludgy about that implemenation?
Comment on attachment 141721 [details] [diff] [review]
Patch to fix

r=dbaron if you add a version of do_GetWeakReference returning
already_AddRefed<nsISupports> instead of making the last two changes in the
patch.
Attachment #141721 - Flags: review?(dbaron) → review+
can you really do that? won't that create two signatures differing only in
return type, which won't compile? Or am I making this up?
Oh, right.  So can you fix nsCOMPtr<nsISupports> so it accepts assignment from
already_AddRefed<foo>?
Oh, never mind.  That's probably hard.  So it's ok as-is.
Attachment #141721 - Flags: superreview?(dougt) → superreview?(bryner)
Attachment #141721 - Flags: superreview?(bryner) → superreview+
I'm not going to be able to land this within a few weeks since I don't have an
environment set up. I'll happily land this afterwards though, and there's no
hurry landing it since it's currently not causing any bugs. But if someone wants
to beat me to it feel free.
This caused two assertions on shutdown about nsWeakReference not being
threadsafe, which it of course never was, it's just that we didn't use to check
for it. I'll look into this and file bugs tomorrow.
Maybe this checkin also caused the (very) small jump in brad leak stats?
(284 -> 296 bytes)
Just a heads up, as a reminder! :)
The leaks probably exited before, it's just that now we can detect them since
AddRef and Release are now implemented using NS_IMPL_ISUPPORTS1
dbaron did the work to find the source of the assertions. Thanks David!

Marking this one fixed.
Status: NEW → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: