Last Comment Bug 666414 - ns{Ref,COM}Ptr operator-> shouldn't allow AddRef, Release
: ns{Ref,COM}Ptr operator-> shouldn't allow AddRef, Release
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla9
Assigned To: :Ehsan Akhgari
:
:
Mentors:
Depends on: 689301 689397 689617 690235 710473 1114999
Blocks:
  Show dependency treegraph
 
Reported: 2011-06-22 15:24 PDT by Joe Drew (not getting mail)
Modified: 2014-12-23 07:22 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (3.95 KB, patch)
2011-09-16 13:44 PDT, :Ehsan Akhgari
no flags Details | Diff | Splinter Review
Patch (v2) (4.91 KB, patch)
2011-09-19 09:26 PDT, :Ehsan Akhgari
benjamin: review+
Details | Diff | Splinter Review
Patch (v3) (19.52 KB, patch)
2011-09-22 08:02 PDT, :Ehsan Akhgari
benjamin: review+
Details | Diff | Splinter Review
Part 2 - Fix the android usage (3.17 KB, patch)
2011-09-22 08:05 PDT, :Ehsan Akhgari
doug.turner: review+
Details | Diff | Splinter Review

Description Joe Drew (not getting mail) 2011-06-22 15:24:02 PDT
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!) 2011-06-22 15:25:20 PDT
We used to have this and removed it.
Comment 2 :Ehsan Akhgari 2011-06-22 15:26:36 PDT
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 :Ehsan Akhgari 2011-06-22 15:26:49 PDT
(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!) 2011-06-22 15:43:17 PDT
Bug 279210 removed nsDerivedSafe.
Comment 5 Joe Drew (not getting mail) 2011-06-22 15:57:55 PDT
David Baron probably has opinions on this!
Comment 6 David Baron :dbaron: ⌚️UTC-7 2011-06-22 16:53:46 PDT
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-7 2011-06-22 16:54:49 PDT
(And if we do add it back, please call it nsDerivedSafe for the benefit of those of us with long memories.)
Comment 8 :Ehsan Akhgari 2011-06-23 09:08:51 PDT
(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 [:bsmedberg] 2011-06-23 09:47:57 PDT
Yes, it's acceptable to apply it only to -> and not the other cases.
Comment 10 :Ehsan Akhgari 2011-09-16 13:44:14 PDT
Created attachment 560615 [details] [diff] [review]
Patch (v1)
Comment 11 Mozilla RelEng Bot 2011-09-16 17:10:55 PDT
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
Comment 12 :Ehsan Akhgari 2011-09-19 09:26:15 PDT
Created attachment 560936 [details] [diff] [review]
Patch (v2)

Fixed one such problematic case in XPCOM itself.
Comment 13 Mozilla RelEng Bot 2011-09-19 13:02:02 PDT
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
Comment 14 :Ehsan Akhgari 2011-09-19 14:21:58 PDT
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 2011-09-19 20:50:54 PDT
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 Mozilla RelEng Bot 2011-09-21 20:20:46 PDT
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 Mozilla RelEng Bot 2011-09-21 23:31:18 PDT
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
Comment 18 :Ehsan Akhgari 2011-09-22 08:02:44 PDT
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.
Comment 19 :Ehsan Akhgari 2011-09-22 08:05:18 PDT
Created attachment 561741 [details] [diff] [review]
Part 2 - Fix the android usage
Comment 20 Mozilla RelEng Bot 2011-09-22 13:21:26 PDT
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
Comment 21 :Ehsan Akhgari 2011-09-23 10:22:13 PDT
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 2011-09-23 14:31:25 PDT
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 23 Benjamin Smedberg [:bsmedberg] 2011-09-26 12:57:07 PDT
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 :Ehsan Akhgari 2011-09-26 13:13:51 PDT
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 :Ehsan Akhgari 2011-09-26 13:42:58 PDT
Follow-up: https://hg.mozilla.org/mozilla-central/rev/ebccffe6d7e6
Comment 26 Jonas Sicking (:sicking) No longer reading bugmail consistently 2011-09-26 17:50:03 PDT
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-7 2011-09-26 18:07:08 PDT
Jonas:  See comment 6?
Comment 28 Brad Jackson 2011-09-26 18:26:02 PDT
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 2011-09-27 17:42:46 PDT
I'm not suggesting that we make .get() return nsDerivedSafe, just operator T*.
Comment 30 :Ehsan Akhgari 2011-09-28 17:36:16 PDT
(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 2011-09-28 19:34:35 PDT
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 :Ehsan Akhgari 2011-09-29 12:33:32 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.