489705 - nsRefPtr should be able to forget a base class to a pure virtual interface it inherits from

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Shawn Wilsher :sdwilsh]

say you have an outparm, nsISupports **_retval, and you have some base object that inherits from nsISupports, nsFoo.  Right now, you can't do this:

nsRefPtr<nsFoo> foo;
foo.forget(_retval);

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

This does the trick for me.  I'm pushing this to the try server now.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Seems like you should probably get rid of the forget(T**) since this should cover it.  (Also, if you're going to ask both benjamin and me for reviews, probably better to swap r/sr.)

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And a random thought, in case anybody ever sees it here:

If we ever do the same thing for nsCOMPtr (which might be sensible given that interfaces can inherit from other interfaces), we should have the equivalent of an NSCAP_ASSERT_NO_QUERY_NEEDED on the result.

Comment 4

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

(In reply to comment #2)
> Seems like you should probably get rid of the forget(T**) since this should
> cover it.  (Also, if you're going to ask both benjamin and me for reviews,
> probably better to swap r/sr.)
I didn't want to change too much, even though I was pretty sure it'd be alright to do that.  My test compile is doing just fine, and the diff is simpler now :)

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 374189 [details] [diff] [review]
v1.1

sr=dbaron assuming this passes the try server

You might want to extend the comment to note that |rhs| may be T** or I** where I is a base class of T.

Comment 6

[David Bradley]

(In reply to comment #3)
> If we ever do the same thing for nsCOMPtr (which might be sensible given that
> interfaces can inherit from other interfaces), we should have the equivalent of
> an NSCAP_ASSERT_NO_QUERY_NEEDED on the result.

Yeah, I've been hitting this case with nsCOMPtr more and more. Working with nsIWritablePropertyBag and returning it as an nsIPropertyBag is one example. Songbird also has a fair number of interface inheritance so we hit it a fair bit.

Comment 7

[Shawn Wilsher :sdwilsh]

Updated the comment per comment 5 before pushing.
http://hg.mozilla.org/mozilla-central/rev/ddb8d3efc243

Comment 8

[[e-mail]]

Note that nsCOMPtr is much harder because of refcnt balancing.