355463 - nsCOMPtr::get() should not return a nsDerivedSafe

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Currently mycomptr.get() returns an nsDerivedSafe. This makes it very clunky to upcast such a variable. The result is something like

nsCOMPtr<nsINode> node;
....
nsIContent* cont = NS_STATIC_CAST(nsIContent*, NS_STATIC_CAST(nsINode*, node));

It would be much cleaner if it was possible to do

nsIContent* cont = NS_STATIC_CAST(nsIContent*, node.get());

Additionally, it's safer in situations where someone uses c-style casts.

nsCOMPtr<nsIFormControl> fc = ...;
nsHTMLInputElement* input = (nsHTMLInputElement*)fc.get();

compiles but fails (it will be a reinterpret cast, whereas what you want is a static cast). Granted, noone should be doing c-style casts.

Comment 1

[Boris Zbarsky [:bzbarsky]]

We don't really want people doing foo.get()->AddRef(), though...

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Personally, I don't think it's likely that anyone will do that accidentally, nor NS_ADDREF(foo.get()). I've never seen anyone do
NS_ADDREF(NS_STATIC_CAST(nsINode*, foo)) for example so I don't think they'll start using .get() get around nsDerivedSafe.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch to fix

I searched content/ and layout/ for ".get()))" and simplified the places i found.

Comment 4

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

> upcast such a variable

you mean downcast?

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

No, downcast is easy and works implicitly. I.e. the following compiles:

nsCOMPtr<nsIContent> cont = ...
nsINode* node = cont;

Upcasting is very clunky though. The following does _not_ compile

nsCOMPtr<nsINode> node = ...
nsIContent* cont = NS_STATIC_CAST(nsIContent*, node);

Nor does

nsCOMPtr<nsINode> node = ...
nsIContent* cont = NS_STATIC_CAST(nsIContent*, node.get());

You currently have to do

nsCOMPtr<nsINode> node = ...
nsIContent* cont = NS_STATIC_CAST(nsIContent*,
                                  NS_STATIC_CAST(nsINode*, node.get()));

(Not sure if the last .get() is needed or not, but people seem to use it)

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Compile tests

There was a compile error in xpcom/tests with the previous patch.

Comment 7

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

Comment on attachment 241794 [details] [diff] [review]
Compile tests

>Index: xpcom/glue/nsCOMPtr.h
>+      nsDerivedSafe<T>*
>+      get_DerivedSafe() const
>+        {
>+          return NS_REINTERPRET_CAST(nsDerivedSafe<T>*, mRawPtr);
>+        }

Do you want this to be private?  With that, r+sr=dbaron.


I think there are some unneeded NS_CONST_CASTs floating around, but probably better to only try one source of build bustage at a time.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Patch that went in

This is the patch I checked in. It addresses dbarons review comment and also fixes nsCOMPtr<nsISupports> to get the same changes as nsCOMPtr<T>