666414 - ns{Ref,COM}Ptr operator-> shouldn't allow AddRef, Release

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Joe Drew (not getting mail)]

I was modifying some legacy code to use nsRefPtr rather than manually AddRefing and Releasing, and I accidentally missed one NS_RELEASE(some nsRefPtr), which happily compiled and shot me in the foot.

Microsoft apparently has some magic in their smart pointers that makes operator-> return a type of pointer that disallows manually calling AddRef and Release. We should emulate that.

Comment 1

[Ben Turner (not reading bugmail, use the needinfo flag!)]

We used to have this and removed it.

Comment 2

[(no longer active)]

This is CComPtr::operator->:


	_NoAddRefReleaseOnCComPtr<T>* operator->() const throw()
	{
		ATLASSERT(p!=NULL);
		return (_NoAddRefReleaseOnCComPtr<T>*)p;
	}

And here's the definition of _NoAddRefReleaseOnCComPtr:


template <class T>
class _NoAddRefReleaseOnCComPtr : public T
{
	private:
		STDMETHOD_(ULONG, AddRef)()=0;
		STDMETHOD_(ULONG, Release)()=0;
};

I think we should do something similar for nsRefPtr and nsCOMPtr.

Comment 3

[(no longer active)]

(In reply to comment #1)
> We used to have this and removed it.

Why?

Comment 4

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Bug 279210 removed nsDerivedSafe.

Comment 5

[Joe Drew (not getting mail)]

David Baron probably has opinions on this!

Comment 6

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

So we used to have it apply for operator*, operator T*, and operator-> and for get().

Having it for operator T* (which was then operator nsDerivedSafe<T>*) meant nsCOMPtr users had to use explicit casts where they no longer do.  For example, what can now be written as:

  nsCOMPtr <nsIDocument> doci;
  nsDocument *doc = static_cast<nsDocument*>(doci.get());

then had to be written as:

  nsCOMPtr <nsIDocument> doci;
  nsDocument *doc = static_cast<nsDocument*>(static_cast<nsIDocument*>(doci));

because you had to cast to the common base class to cast between two derived classes of nsIDocument*.


If we applied it only to operator->, we'd catch most cases, though.  That might be a reasonable compromise.

Comment 7

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

(And if we do add it back, please call it nsDerivedSafe for the benefit of those of us with long memories.)

Comment 8

[(no longer active)]

(In reply to comment #6)
> If we applied it only to operator->, we'd catch most cases, though.  That might
> be a reasonable compromise.

Benjamin, would you agree with this too?

Comment 9

[Benjamin Smedberg]

Yes, it's acceptable to apply it only to -> and not the other cases.

Comment 10

[(no longer active)]

Attachment: Patch (v1)

Comment 11

[Mozilla RelEng Bot]

Try run for 9f9dd3cfe761 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=9f9dd3cfe761
Results (out of 18 total builds):
    success: 6
    failure: 12
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-9f9dd3cfe761

Comment 12

[(no longer active)]

Attachment: Patch (v2)

Fixed one such problematic case in XPCOM itself.

Comment 13

[Mozilla RelEng Bot]

Try run for 84c78355b7d4 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=84c78355b7d4
Results (out of 18 total builds):
    success: 6
    failure: 12
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-84c78355b7d4

Comment 14

[(no longer active)]

Seems like we have all sorts of broken code all over the tree which rely on being able to call AddRef/Release on our smart pointers.  I'm going through them trying to fix the compilation errors.

Comment 15

[Mozilla RelEng Bot]

Try run for bb26cf539306 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=bb26cf539306
Results (out of 35 total builds):
    success: 13
    warnings: 1
    failure: 21
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-bb26cf539306

Comment 16

[Mozilla RelEng Bot]

Try run for 5d05bdceb8d0 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=5d05bdceb8d0
Results (out of 18 total builds):
    success: 12
    failure: 6
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-5d05bdceb8d0

Comment 17

[Mozilla RelEng Bot]

Try run for 72faec14942d is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=72faec14942d
Results (out of 2 total builds):
    success: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-72faec14942d

Comment 18

[(no longer active)]

Attachment: Patch (v3)

This fixes all of the usages in our code base, except for one in the Android plugin code, which requires a bit more involved fix.  I'll attach another patch for that.

Comment 19

[(no longer active)]

Attachment: Part 2 - Fix the android usage

Comment 20

[Mozilla RelEng Bot]

Try run for cc9a8d680df8 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=cc9a8d680df8
Results (out of 18 total builds):
    success: 16
    failure: 2
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-cc9a8d680df8

Comment 21

[(no longer active)]

I verified that these patches do not break Thunderbird and SeaMonkey (at least on Windows and Linux).  The comm-central try server was useless unfortunately, so I couldn't test this on the try server.

Comment 22

[Mozilla RelEng Bot]

Try run for fa219a882ef2 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=fa219a882ef2
Results (out of 18 total builds):
    success: 18
Builds available at http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fa219a882ef2

Comment 23

[Benjamin Smedberg]

Comment on attachment 561740 [details] [diff] [review]
Patch (v3)

Ugh, I think the `copy.forget()` pattern is pretty hideous, but I don't have an alternative to suggest right now.

Comment 24

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/489f9e746213
https://hg.mozilla.org/mozilla-central/rev/35196c69cb12

I also merged to inbound immediately after pushing to make sure that no patch lands on inbound which breaks because of this before the next merge.

Comment 25

[(no longer active)]

Follow-up: https://hg.mozilla.org/mozilla-central/rev/ebccffe6d7e6

Comment 26

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

FWIW, I think that the implicit operator* should also return a nsDerivedSafe. People can still use .get() (in fact, they might anyway in order to get things to compile on all platforms).

Comment 27

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

Jonas:  See comment 6?

Comment 28

[Brad Jackson]

Changeset 489f9e746213 caused my build to fail. See https://bugzilla.mozilla.org/show_bug.cgi?id=689390

Comment 29

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

I'm not suggesting that we make .get() return nsDerivedSafe, just operator T*.

Comment 30

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #29)
> I'm not suggesting that we make .get() return nsDerivedSafe, just operator
> T*.

That would break the people who use the implicit conversion, wouldn't it?

Comment 31

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

It would break doing something like:

  nsCOMPtr <nsIDocument> doci;
  nsDocument *doc = static_cast<nsDocument*>(doci);

but does that work anyway?

It wouldn't break:

  nsCOMPtr <nsIDocument> doci;
  functionThatTakesnsIDocumentOrnsINode(doci);

Comment 32

[(no longer active)]

(In reply to Jonas Sicking (:sicking) from comment #31)
> It would break doing something like:
> 
>   nsCOMPtr <nsIDocument> doci;
>   nsDocument *doc = static_cast<nsDocument*>(doci);
> 
> but does that work anyway?

Hmm, yeah, you're right.