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

RESOLVED FIXED in mozilla9

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: Joe Drew (not getting mail), Assigned: Ehsan)

Tracking

Trunk
mozilla9
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

6 years ago
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.
We used to have this and removed it.
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.
(In reply to comment #1)
> We used to have this and removed it.

Why?
Bug 279210 removed nsDerivedSafe.
(Reporter)

Comment 5

6 years ago
David Baron probably has opinions on this!
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.
(And if we do add it back, please call it nsDerivedSafe for the benefit of those of us with long memories.)
(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?
Yes, it's acceptable to apply it only to -> and not the other cases.
Created attachment 560615 [details] [diff] [review]
Patch (v1)
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #560615 - Flags: review?(benjamin)

Comment 11

6 years ago
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/eakhgari@mozilla.com-9f9dd3cfe761
Created attachment 560936 [details] [diff] [review]
Patch (v2)

Fixed one such problematic case in XPCOM itself.
Attachment #560615 - Attachment is obsolete: true
Attachment #560936 - Flags: review?(benjamin)
Attachment #560615 - Flags: review?(benjamin)
Attachment #560936 - Flags: review?(benjamin) → review+

Comment 13

6 years ago
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/eakhgari@mozilla.com-84c78355b7d4
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

6 years ago
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/eakhgari@mozilla.com-bb26cf539306

Comment 16

6 years ago
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/eakhgari@mozilla.com-5d05bdceb8d0

Comment 17

6 years ago
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/eakhgari@mozilla.com-72faec14942d
Created attachment 561740 [details] [diff] [review]
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.
Attachment #560936 - Attachment is obsolete: true
Attachment #561740 - Flags: review?(benjamin)
Created attachment 561741 [details] [diff] [review]
Part 2 - Fix the android usage
Attachment #561741 - Flags: review?(doug.turner)

Updated

6 years ago
Attachment #561741 - Flags: review?(doug.turner) → review+

Comment 20

6 years ago
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/eakhgari@mozilla.com-cc9a8d680df8
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

6 years ago
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/eakhgari@mozilla.com-fa219a882ef2
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.
Attachment #561740 - Flags: review?(benjamin) → review+
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
Follow-up: https://hg.mozilla.org/mozilla-central/rev/ebccffe6d7e6
Depends on: 689397
Depends on: 689301
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).
Jonas:  See comment 6?

Comment 28

6 years ago
Changeset 489f9e746213 caused my build to fail. See https://bugzilla.mozilla.org/show_bug.cgi?id=689390
Depends on: 689617
I'm not suggesting that we make .get() return nsDerivedSafe, just operator T*.
(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?
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);
Depends on: 690235
(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.
Depends on: 710473
Depends on: 1114999
You need to log in before you can comment on or make changes to this bug.