Closed Bug 666414 Opened 8 years ago Closed 8 years ago

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

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: joe, Assigned: ehsan)

References

Details

Attachments

(2 files, 2 obsolete files)

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?
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.
Attached patch Patch (v1) (obsolete) — Splinter Review
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #560615 - Flags: review?(benjamin)
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
Attached patch Patch (v2) (obsolete) — Splinter Review
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+
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.
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
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
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
Attached patch Patch (v3)Splinter Review
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)
Attachment #561741 - Flags: review?(doug.turner)
Attachment #561741 - Flags: review?(doug.turner) → review+
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.
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
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
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).
Changeset 489f9e746213 caused my build to fail. See https://bugzilla.mozilla.org/show_bug.cgi?id=689390
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);
(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.