Last Comment Bug 661973 - Create templated reference counting for Spidermonkey
: Create templated reference counting for Spidermonkey
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: MFBT (show other bugs)
: Trunk
: x86 Windows 7
: -- normal (vote)
: mozilla7
Assigned To: Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
:
:
Mentors:
Depends on:
Blocks: 625600 637216
  Show dependency treegraph
 
Reported: 2011-06-03 16:22 PDT by David Mandelin [:dmandelin]
Modified: 2011-12-17 23:19 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutparamRef (9.94 KB, patch)
2011-06-16 03:01 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutParamRef, v2 (10.29 KB, patch)
2011-06-16 14:23 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
luke: review+
Details | Diff | Splinter Review
Bug 661973: Implement mozilla::RefCounted, RefPtr, TemporaryRef, OutParamRef, and byRef, v3 (10.56 KB, patch)
2011-06-16 15:52 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
bas: review+
roc: superreview+
Details | Diff | Splinter Review
Some changes to RefPtr code (2.48 KB, patch)
2011-06-20 08:59 PDT, Bas Schouten (:bas.schouten)
cjones.bugs: feedback-
Details | Diff | Splinter Review
Track which ptr was handed out from OutParamRef (4.12 KB, patch)
2011-06-20 20:08 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Add additional intermediate class (3.29 KB, patch)
2011-06-20 20:40 PDT, Bas Schouten (:bas.schouten)
cjones.bugs: feedback-
Details | Diff | Splinter Review
Proposed RefPtr additions (1.23 KB, patch)
2011-06-21 07:43 PDT, Bas Schouten (:bas.schouten)
no flags Details | Diff | Splinter Review
Proposed RefPtr additions v2 (3.35 KB, patch)
2011-06-21 21:44 PDT, Bas Schouten (:bas.schouten)
cjones.bugs: review-
Details | Diff | Splinter Review
Followup to bug 661973: Fix bug with COM outparams and add convenience operators (5.08 KB, patch)
2011-06-21 22:42 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
cjones.bugs: review+
Details | Diff | Splinter Review

Description David Mandelin [:dmandelin] 2011-06-03 16:22:58 PDT
JM uses fully manual reference counting to manage code buffers. It would be nicer to have template-based reference counting smart pointers.

Also, upstream Yarr uses reference counting for a few things, with reference-counting smart pointers. We could stay closer to upstream if we had our own refcounting smart pointers.

So, let's make some. Ideally, they would have the same API used in yarr/wtfbridge.h, which contains do-nothing smart pointers that are API-compatible with Yarr. Failing that, they do need to be wrappable to have that same interface. 

Bug 463260 is related, but that's an old one. If we go forward here it's time to close that one out.
Comment 1 Boris Zbarsky [:bz] (still a bit busy) 2011-06-06 12:19:39 PDT
Can we just move nsRefPtr into our shared template stuff?
Comment 2 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-06 13:34:24 PDT
What kind of refcounting pointer(s) do the Monkeys want?
Comment 3 David Mandelin [:dmandelin] 2011-06-06 13:36:51 PDT
(In reply to comment #2)
> What kind of refcounting pointer(s) do the Monkeys want?

What kinds do you have? :-) For now, the only criterion is something compatible with Yarr/ExecutablePool. They have a RefCounted<T> base class, a RefPtr<T> smart pointer that adjusts counts, and a PassRefPtr<T> that I believe is just used to make the compiler tell you if you try to drop a reference on the floor.
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-06 14:00:01 PDT
That doesn't sound too far off from what non-XPCOM gecko uses, except with macros+duck typing[1] instead of RefCounted<T> [1].  (Another kind is boost's shared_ptr, but that couldn't be used in gecko except with brand-new code.)

What kind of thread safety do the Monkeys need?

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsISupportsImpl.h#327
Comment 5 David Mandelin [:dmandelin] 2011-06-06 14:12:33 PDT
(In reply to comment #4)
> What kind of thread safety do the Monkeys need?

None. (i.e., single-threaded access only).
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-08 17:04:57 PDT
We need the same thing for Azure. And we need it now!

(In reply to comment #4)
> That doesn't sound too far off from what non-XPCOM gecko uses, except with
> macros+duck typing[1] instead of RefCounted<T> [1].

RefPtr<T> could work with both existing macro-ized refcounted types as well as a RefCounted<T> base class, I assume.

I would love to see RefPtr<T> and RefCounted<T> in mfbt, with nsRefPtr a typedef for mozilla::RefPtr.
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-08 18:12:44 PDT
Sorry, lost this bug in the shuffle of moving.

WTF's RefPtr also uses "duck typing", AFAICT.  That's generally good except it means we can't have a generic mozilla::RefPtr that's compatible with both nsRefPtr and wtf::RefPtr, because we can't specialize for RefCounted<T> and "everything else".  (At least, I don't know of another way to do that.)

We do have the option of making our own mozilla::RefCounted/mozilla::RefPtr that just mirror the wtf:: interface.  Or import the wtf:: wholesale into mfbt.  Then we can add little mozilla::Ref*<->wtf::Ref* converters at the API boundaries, or use some other magic.

With mozilla:Ref* in hand, we could then rewrite all the Gecko code that uses NS_INLINE_DECL_REFCOUNTING to instead use mozilla::RefCounted<T>, then rewrite nsRefPtr's to those to mozilla::RefPtr's.  That would be a pretty big project, so probably worth a followup (if we ever want to do it.)

Another option would be to keep around a patch to the imported wtf:: code that changes its RefCounted<T>/RefPtr<T> to the existing NS_INLINE_DECL_REFCOUNTING/nsRefPtr interface.  I doubt those files change much.  Then we could implement a wtf-compatible mozilla::RefPtr<T> that allows nsRefPtr to be a typedef.
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-08 18:14:38 PDT
Or more briefly, I think the options are
 (1) Implement a new mozilla::RefCounted/RefPtr compatible with the wtf interface, then rewrite NS_INLINE_DECL_REFCOUNTING/nsRefPtr-using code.
 (2) Patch the relevant wtf files to be compatible with nsRefPtr, move nsRefPtr to mfbt as mozilla::RefPtr, |typedef mozilla::RefPtr nsRefPtr|.
Comment 9 David Mandelin [:dmandelin] 2011-06-08 18:18:53 PDT
(In reply to comment #8)
> Or more briefly, I think the options are
>  (1) Implement a new mozilla::RefCounted/RefPtr compatible with the wtf
> interface, then rewrite NS_INLINE_DECL_REFCOUNTING/nsRefPtr-using code.
>  (2) Patch the relevant wtf files to be compatible with nsRefPtr, move
> nsRefPtr to mfbt as mozilla::RefPtr, |typedef mozilla::RefPtr nsRefPtr|.

It sounds like Azure needs this more than I do, so their needs should take priority. But for me, (2) doesn't help because most of the reason I want this for Yarr is to avoid patching their files. Rewriting the Gecko code seems painful, though. I wouldn't mind wrapping some classes for Yarr, though.

The other thing I don't like about (1) is that it passes on the opportunity to modernize our refcounting interface. Does it make any sense to make a solid macroless C++ refcounting setup, then wrap that if needed for other uses?
Comment 10 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-08 18:52:51 PDT
For (2), I would expect we'd only need to patch RefCounted.h and RefPtr.h, which I would think only change infrequently if at all.

Comment 7 is a bit inaccurate, though: if all the wtf:: objects we need to use from within SM are |public RefCounted<T>|, then we *would* be able to specialize a base mozilla::RefPtr to work with wtf::RefCounted.  But because wtf::RefPtr operates on a "duck type", there's no guarantee all RefPtr'd objects are RefCounted<T>.

As for modernizing gecko's refcounting, IMHO there's not much value other than stylistic in de-macroizing NS_INLINE_DECL_REFCOUNTING, although there is a lot of value in having a non-xpcom/ mozilla::RefCounted/RefPtr for stuff like Azure.

So:
  (3) If all the wtf:: objects we need to use from within SM are |public RefCounted<T>|, we can make a mozilla::RefPtr typedef-able to nsRefPtr, and specializable for mozilla::/wtf::RefCounted.
Comment 11 David Mandelin [:dmandelin] 2011-06-08 19:04:55 PDT
(In reply to comment #10)
> For (2), I would expect we'd only need to patch RefCounted.h and RefPtr.h,
> which I would think only change infrequently if at all.

IIUC, those files are GPL'd, and so will never be imported into our tree. For now, I created classes RefCounted and RefPtr that basically do nothing, but do compile with Yarr. I then adapted Yarr to manage the memory without using refcounting. So it's JSC::Yarr:: objects that we use, not wtf:: objects.

> Comment 7 is a bit inaccurate, though: if all the wtf:: objects we need to
> use from within SM are |public RefCounted<T>|, then we *would* be able to
> specialize a base mozilla::RefPtr to work with wtf::RefCounted.  But because
> wtf::RefPtr operates on a "duck type", there's no guarantee all RefPtr'd
> objects are RefCounted<T>.

Seems fine.

> As for modernizing gecko's refcounting, IMHO there's not much value other
> than stylistic in de-macroizing NS_INLINE_DECL_REFCOUNTING, although there
> is a lot of value in having a non-xpcom/ mozilla::RefCounted/RefPtr for
> stuff like Azure.

Not enough value to change all the existing code. But it seems like it would nice to have a more modern API for new code. Maybe having a mix in Gecko is too icky, though.

> So:
>   (3) If all the wtf:: objects we need to use from within SM are |public
> RefCounted<T>|, we can make a mozilla::RefPtr typedef-able to nsRefPtr, and
> specializable for mozilla::/wtf::RefCounted.

That seems good.
Comment 12 Brendan Eich [:brendan] 2011-06-08 19:35:11 PDT
Sweep with a new broom, even if you can't sweep the entirety of Mount Vernon. The cleaning crew of elves (volunteer elves self-muster) will get through all the rooms eventually. (Mt. Vernon is visited by tourists all the time, yet seems to be always clean, yet cannot be cleaned all at once every night; the crew picks up where they left off....)

/be
Comment 13 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-08 21:13:02 PDT
I'm confused. Why do we need to specialize RefPtr<T> to behave differently for Refcounted<T> vs everything else? Is it because Webkit's Refcounted<T> needs ref() and deref() vs AddRef()/Release()? If so, wouldn't we just make our Refcounted<T> use AddRef()/Release() instead and everything's happy? Or if everything's not happy because Yarr or some other code wants to use ref() and deref() explicitly as well, why not have Refcounted<T> expose both AddRef()/Release() and ref()/deref()?
Comment 14 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-08 21:39:38 PDT
David: when you say "something compatible with Yarr/ExecutablePool", which of the following, or both or neither, do you mean
 - mozilla::RefPtr (or whatever) can store wtf::RefCounted (i.e. SM can store returned webkit objects in our ptr)
 - wtf::RefPtr can store mozilla::RefCounted (or whatever) objects (i.e. SM can pass SM objects to webkit's ptr)
Comment 15 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-08 21:49:11 PDT
(In reply to comment #11)
> (In reply to comment #10)
> > For (2), I would expect we'd only need to patch RefCounted.h and RefPtr.h,
> > which I would think only change infrequently if at all.
> 
> IIUC, those files are GPL'd, and so will never be imported into our tree.
> For now, I created classes RefCounted and RefPtr that basically do nothing,
> but do compile with Yarr. I then adapted Yarr to manage the memory without
> using refcounting. So it's JSC::Yarr:: objects that we use, not wtf::
> objects.

Somehow I totally missed this, sorry.  That makes things much easier, and what roc proposed in comment 13 should be fine.
Comment 16 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-12 19:01:36 PDT
Who's going to do this? We need these for Azure to land, and we're hoping to land it in a week. I can do it if no-one else will, but I'm probably not the best person to do it. cjones?
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-13 20:54:00 PDT
I'll most likely have time for this at the end of the week, but if you need it sooner I'm probably not the guy.  Sorry.
Comment 18 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-16 03:01:37 PDT
Created attachment 539759 [details] [diff] [review]
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutparamRef

Hopefully the comments and tests describe what's going on with these classes.  I'm not sure what to do with the tests in the long term; they can probably be deleted when gecko adopts all the tested features.

I'm about 95% sure nsRefPtr and already_AddRefed can be built on this.  I'm less sure that the wtf:: code will work with this impl, but we can cross that bridge we we come to it.  (Simple code should be fine, but they might have corner-case-y code that doesn't work with the mfbt stuff.)

How should should someone (Bas, Rob, me?) check if this impl works with Azure?
Comment 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-16 04:21:50 PDT
Comment on attachment 539759 [details] [diff] [review]
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutparamRef

Review of attachment 539759 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Refcounting.h
@@ +89,5 @@
> +        if (0 == --refCnt) {
> +#ifdef DEBUG
> +            refCnt = -0xdead;
> +#endif
> +            delete static_cast<T*>(this);

I believe XPCOM refcounting temporarily sets the refcount to 1 during the last Release, so that destructor code can addref and release the object being destroyed without blowing up.

I do not know whether we actually rely on that.

@@ +124,5 @@
> +class RefPtr
> +{
> +    // To allow them to use unref()
> +    friend class TemporaryRef<T>;
> +    friend class OutparamRef<T>;

OutParam. Outparam is not one word.

@@ +131,5 @@
> +    typedef OutparamRef<T> Outparam;
> +
> +public:
> +    RefPtr() : ptr(0) { }
> +    RefPtr(const RefPtr& o) : ptr(ref(o.ptr)) {}

Isn't this redundand with the RefPtr<U> constructor below?
Comment 20 Bas Schouten (:bas.schouten) 2011-06-16 08:26:26 PDT
Comment on attachment 539759 [details] [diff] [review]
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutparamRef

Review of attachment 539759 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Refcounting.h
@@ +58,5 @@
> + * RefCounted<T>, i.e. outside of the RefPtr machinery.  Attempts to
> + * do so will abort DEBUG builds.  It's recommended that RefCounted<T>
> + * be created using the following idiom
> + *
> + *   RefPtr<T> ref(new T());

Is there a reasonable objection to using it as RefPtr<T> ref = new T(); because personally I find that syntax much more attractive and I doubt it's any less efficient in an optimized build. So although I wouldn't want to force anyone that way I don't see a reason to discourage it.

@@ +136,5 @@
> +    RefPtr(const Temporary& o) : ptr(o.drop()) {}
> +    RefPtr(T* t) : ptr(ref(t)) {}
> +
> +    template<typename U>
> +    RefPtr(const RefPtr<U>& o) : ptr(ref(o.get())) {}

This allows casting to a different but related RefPtr U. I suspect we may also want an assignment operator that can assign to related types and possibly an Operator U*(). Although I'm not sure.

@@ +161,5 @@
> +    }
> +
> +    T* get() const { return ptr; }
> +    operator T*() const { return ptr; }
> +    T* operator->() const { return ptr; }

Do we want to assert ptr->refCount > 0 here?

@@ +165,5 @@
> +    T* operator->() const { return ptr; }
> +    T& operator*() const { return *ptr; }
> +    Outparam operator&() {
> +        return Outparam(&ptr);
> +    }

Any chance we can also have a operator Temporary() overload so we can lose the terrible '.forget()' syntax for function returns?

Also, I'm not 100% sure if 'TemporaryRef' is the best way to designate the class used for function return values? It might not be obvious this simply means 'an add-refed return value'.
Comment 21 Luke Wagner [:luke] 2011-06-16 08:45:57 PDT
Comment on attachment 539759 [details] [diff] [review]
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutparamRef

Review of attachment 539759 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Refcounting.h
@@ +236,5 @@
> + * Don't use T** outparams unless you have a good reason to.  T*
> + * return values are clearer and incur less runtime overhead.
> + */
> +template<typename T>
> +class OutparamRef

This utility is a bit creepy in that it lets you give away a naked lvalue to RefPtr's internal pointer.  The accounting done by ~OutparamRef() seems reasonable but it depends on the callee knowing and following a very specific interface.  Given how unassuming a call:
   RefPtr<T> p = ...
   foo(&p);
looks, one might assume that foo took a RefPtr<T>* and thus there was nothing to worry about.  Is this COM convention rock-solid throughout Mozilla so maybe I'm just being a n00b here?

Also, to be maximally safe and obey the "don't pass mutable references for outparams, use pointers instead since its more explicit at the call site" stylistic guideline we have in SpiderMonkey, I would want to pass a RefPtr<T>* outparam which, since operator& as been overloaded, isn't easy.

@@ +270,5 @@
> +#if 0
> +
> +// Command line that builds these tests
> +//
> +//   cp Refcounting.h test.cc && g++ -g -Wall -pedantic -DDEBUG -o test test.cc && ./test

This is a great idea for how to demonstrate sample usage.
Comment 22 Luke Wagner [:luke] 2011-06-16 08:55:39 PDT
Comment on attachment 539759 [details] [diff] [review]
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutparamRef

Review of attachment 539759 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/Refcounting.h
@@ +133,5 @@
> +public:
> +    RefPtr() : ptr(0) { }
> +    RefPtr(const RefPtr& o) : ptr(ref(o.ptr)) {}
> +    RefPtr(const Temporary& o) : ptr(o.drop()) {}
> +    RefPtr(T* t) : ptr(ref(t)) {}

Should this be 'explicit' so that we don't have implicit taking-of-ownership?  Unfortunately, this will disable the assignment-initialization syntax that is more attractive and Bas mentions in his review comment.
Comment 23 Luke Wagner [:luke] 2011-06-16 08:56:38 PDT
In comment 22, I mean just the RefPtr(T*) constructor (does splinter have any way to control the size of quotation?).
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2011-06-16 09:26:49 PDT
> does splinter have any way to control the size of quotation

No, which is the main reason to not use it.  :(
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-16 11:05:16 PDT
(In reply to comment #19)
> I believe XPCOM refcounting temporarily sets the refcount to 1 during the
> last Release, so that destructor code can addref and release the object
> being destroyed without blowing up.

Yep.

> I do not know whether we actually rely on that.

I'm not sure it's a good idea to, but I don't have strong feelings.  At any rate, the set-to-magic-value is just to check for an illegal state.  We can revert to COM behavior if necessary.

Possibly relevant is that wtf::RefCounted blows up on AddRef/Release after last Release, i.e. doesn't implement the COM behavior.

> OutParam. Outparam is not one word.

OK.

> > +    RefPtr(const RefPtr& o) : ptr(ref(o.ptr)) {}
> 
> Isn't this redundand with the RefPtr<U> constructor below?

Sadly no, the copy ctor is a magic beast.

(In reply to comment #20)
> Comment on attachment 539759 [details] [diff] [review] [review]
> Is there a reasonable objection to using it as RefPtr<T> ref = new T();

Not really, except what Luke points out below.  I should probably remove the example.

> > +    RefPtr(const RefPtr<U>& o) : ptr(ref(o.get())) {}
> 
> This allows casting to a different but related RefPtr U. I suspect we may
> also want an assignment operator that can assign to related types and
> possibly an Operator U*(). Although I'm not sure.

Yes.  These operators are a nice convenience; we'll probably want more as time goes on.  But oops, I had meant to add that operator=(), thanks.

> > +    T* get() const { return ptr; }
> > +    operator T*() const { return ptr; }
> > +    T* operator->() const { return ptr; }
> 
> Do we want to assert ptr->refCount > 0 here?

We'll catch erroneous refcounting upon ~RefPtr, but an early-warning system sounds good.

> > +    Outparam operator&() {
> > +        return Outparam(&ptr);
> > +    }
> 
> Any chance we can also have a operator Temporary() overload so we can lose
> the terrible '.forget()' syntax for function returns?

Not sure about "terrible" :) but I'm not against the magic operator.

> Also, I'm not 100% sure if 'TemporaryRef' is the best way to designate the
> class used for function return values? It might not be obvious this simply
> means 'an add-refed return value'.

It can also be used with function params to forward references, without using bare T*, a la wtf::PassRefPtr.  Or could be; TemporaryRef isn't quite set up for full PassRefPtr parity yet.  I'm not sure how much we need/want that though.

Really, it's an object meant to only be used as a temporary to forward a reference to a RefPtr, or release the reference if not forwarded.  TemporaryRef was the most concise /descriptive/ name I could think of.  If there's a better suggestion for a prescriptive name, let's hear it! :)

> > +template<typename T>
> > +class OutparamRef
> 
> This utility is a bit creepy in that it lets you give away a naked lvalue to
> RefPtr's internal pointer.

Agreed.  Bas points out that we can add a layer of indirection in OutparamRef, passing out a pointer to an internal OutparamRef pointer, which is the way this should have been written.

> Is this COM convention rock-solid throughout Mozilla so maybe I'm just being a n00b here?

It's prevalent, not sure about rock-solid.  I'm not sure yet that COM compatibility here is required for mozilla::RefPtr.  I would love to drop it and kill OutparamRef.  Still collecting details.

> Also, to be maximally safe and obey the "don't pass mutable references for
> outparams, use pointers instead since its more explicit at the call site"
> stylistic guideline we have in SpiderMonkey, I would want to pass a
> RefPtr<T>* outparam which, since operator& as been overloaded, isn't easy.

Yes.  If we need to keep OutparamRef, we should probably turn things around and make getting one of those require an explicit .outparam() call, and not overload operator&.  (Principle of least surprise.)

> > +    RefPtr(T* t) : ptr(ref(t)) {}
> 
> Should this be 'explicit' so that we don't have implicit
> taking-of-ownership?  Unfortunately, this will disable the
> assignment-initialization syntax that is more attractive and Bas mentions in
> his review comment.

The syntax Bas mentions is pretty much exclusively what gecko uses at the moment.  Taking that away would significantly increase the work to unify nsRefPtr and mozilla::RefPtr.  That's not a total deal-breaker though.
Comment 26 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-16 11:08:45 PDT
> (In reply to comment #19)
> > I believe XPCOM refcounting temporarily sets the refcount to 1 during the
> > last Release, so that destructor code can addref and release the object
> > being destroyed without blowing up.
> 
> Yep.
> 
> > I do not know whether we actually rely on that.
> 
> I'm not sure it's a good idea to, but I don't have strong feelings.  At any
> rate, the set-to-magic-value is just to check for an illegal state.  We can
> revert to COM behavior if necessary.
> 
> Possibly relevant is that wtf::RefCounted blows up on AddRef/Release after
> last Release, i.e. doesn't implement the COM behavior.

There are definitely classes in Gecko today that depend on refcount stabilization.  nsSupportsWeakReference and some of the stuff in imagelib come to mind, but I'm sure there are more.(In reply to comment #25)
Comment 27 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-16 11:22:48 PDT
Do you happen to know if any are NS_INLINE_DECL_REFCOUNTING classes?
Comment 28 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-16 11:32:48 PDT
(In reply to comment #27)
> Do you happen to know if any are NS_INLINE_DECL_REFCOUNTING classes?

They shouldn't be!  The only thing that makes AddRef after refcount == 0 not a complete abomination is that QueryInterface AddRefs, and an NS_INLINE_DECL_REFCOUNTING class doesn't have QI ...

I don't know off hand of any that do, but I do know that some of them do weird things (e.g. nsXBLInsertionPointEntry).  The numbers here are small enough (< 50 classes) that a manual audit is feasible.
Comment 29 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-16 11:54:35 PDT
(In reply to comment #19)
> ::: mfbt/Refcounting.h
> @@ +89,5 @@
> > +        if (0 == --refCnt) {
> > +#ifdef DEBUG
> > +            refCnt = -0xdead;
> > +#endif
> > +            delete static_cast<T*>(this);
> 
> I believe XPCOM refcounting temporarily sets the refcount to 1 during the
> last Release, so that destructor code can addref and release the object
> being destroyed without blowing up.
> 
> I do not know whether we actually rely on that.

Ignoring the issue of reliance interests, I think I've heard bsmedberg (or dbaron?  or someone else?) describe this as something we shouldn't really be doing.  But perhaps short-term parity outweighs that desire.
Comment 30 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-06-16 11:56:45 PDT
That would be Bug 508133.

FWIW, I'd love to see us create something that does not have refcount stabilization, even if that means we have to migrate everything manually over years.
Comment 31 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-16 14:23:44 PDT
Created attachment 539897 [details] [diff] [review]
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutParamRef, v2

It sounds like we're going to want COM compatibility in Azure for sanity.  That means we'll need to support T**-style outparams, which means we need OutParamRef or something like it.  C'est la guerre.

COM-compatibility also means that we can't assert refCount > 0 in RefPtr getters, because we don't have a sane way to ask for the refcount of a COM object.

Changes
 - s/Outparam/OutParam/
 - template<U> op=(RefPtr<U>) helper
 - operator Temporary()
 - better impl of OutParamRef
 - need explicit ptr.outparam() to get an OutParamRef, no operator& overload.  Suggestions for better syntax welcome.  (I pondered |outparam(ptr)|, not sure if that's better.)
Comment 32 Luke Wagner [:luke] 2011-06-16 15:01:19 PDT
Comment on attachment 539897 [details] [diff] [review]
Implement mozilla::RefCounted, RefPtr, TemporaryRef, and OutParamRef, v2

Review of attachment 539897 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for your answers; they make sense.
Comment 33 Bas Schouten (:bas.schouten) 2011-06-16 15:12:14 PDT
(In reply to comment #31)
> Created attachment 539897 [details] [diff] [review] [review]
>
>  - need explicit ptr.outparam() to get an OutParamRef, no operator&
> overload.  Suggestions for better syntax welcome.  (I pondered
> |outparam(ptr)|, not sure if that's better.)

This makes me die a little inside... But I'll yield this if it gets us COM compatibility :).
Comment 34 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-16 15:52:41 PDT
Created attachment 539917 [details] [diff] [review]
Bug 661973: Implement mozilla::RefCounted, RefPtr, TemporaryRef, OutParamRef, and byRef, v3

Carrying over Luke's r+.

This implements a |byRef()| helper instead of RefPtr.outparam(), on Bas's suggestion.  So one would now write
  result Foo(T** outparam);
  RefPtr<T> p;
  rv = Foo(byRef(p));
Comment 35 Bas Schouten (:bas.schouten) 2011-06-16 16:42:29 PDT
Comment on attachment 539917 [details] [diff] [review]
Bug 661973: Implement mozilla::RefCounted, RefPtr, TemporaryRef, OutParamRef, and byRef, v3

Review of attachment 539917 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 36 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-06-16 19:39:29 PDT
Comment on attachment 539917 [details] [diff] [review]
Bug 661973: Implement mozilla::RefCounted, RefPtr, TemporaryRef, OutParamRef, and byRef, v3

Review of attachment 539917 [details] [diff] [review]:
-----------------------------------------------------------------

I think Refcounting.h would be better as RefPtr.h. There is a small advantage to having the name of the commonly-used type be the name of the header file.

::: mfbt/Refcounting.h
@@ +67,5 @@
> + * RefPtr<super/subclass of T>.  Upon a transition from refcounted==1
> + * to 0, the RefCounted<T> "dies" and is destroyed.  The "destroyed"
> + * state is represented in DEBUG builds by refcount==-0xdead.  This
> + * state distinguishes use-before-ref (refcount==0) from
> + * use-after-destroy (refcount==-0xdead).

You probably should say here that T is expected to be the subclass of RefCounted, and that it's there so that the destructor can be called without necessarily being virtual.

@@ +125,5 @@
> +    friend class TemporaryRef<T>;
> +    friend class OutParamRef<T>;
> +
> +    typedef TemporaryRef<T> Temporary;
> +    typedef OutParamRef<T> OutParam;

I'd call these TemporaryRef/OutParamRef, it's just a little bit clearer.

@@ +162,5 @@
> +        T* tmp = ptr;
> +        ptr = 0;
> +        return Temporary(tmp);
> +    }
> +    operator Temporary() { return forget(); }

I'm uncomfortable with this operator; I prefer the explicit forget(). Basically I don't think conversion operators (which can be called implicitly) should have side effects.

I see that Bas disagrees on this. I'll let you make the final call or delegate it to whoever you please.
Comment 37 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-16 20:17:18 PDT
> I think Refcounting.h would be better as RefPtr.h. There is a small
> advantage to having the name of the commonly-used type be the name of the
> header file.

OK by me, I don't particularly care.

> You probably should say here that T is expected to be the subclass of
> RefCounted, and that it's there so that the destructor can be called without
> necessarily being virtual.

Good point, will add.

> > +    typedef TemporaryRef<T> Temporary;
> > +    typedef OutParamRef<T> OutParam;
> 
> I'd call these TemporaryRef/OutParamRef, it's just a little bit clearer.

I'd like to, but those typedefs conflict with the declarations of TemporaryRef and OutParamRef.  I could call them TemporaryRefT/OutParamRefT if you'd prefer.

> 
> @@ +162,5 @@
> > +        T* tmp = ptr;
> > +        ptr = 0;
> > +        return Temporary(tmp);
> > +    }
> > +    operator Temporary() { return forget(); }
> 
> I'm uncomfortable with this operator; I prefer the explicit forget().
> Basically I don't think conversion operators (which can be called
> implicitly) should have side effects.
> 
> I see that Bas disagrees on this. I'll let you make the final call or
> delegate it to whoever you please.

I think that's a good guideline.  In this particular case, I don't think there's much chance of the side-effecty implicit coercion being surprising or causing problems, but this also doesn't like a big enough convenience win to bend the general rule.  Let's drop it.  Sorry Bas :).
Comment 38 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-16 20:42:13 PDT
http://hg.mozilla.org/mozilla-central/rev/d87b04f9f9dd

There will almost certainly be problems/missing features when these are used with COM and wtf:: stuff.  Might as well just fix them in t-m and graphics and merge back to m-c.
Comment 39 Bas Schouten (:bas.schouten) 2011-06-20 08:59:02 PDT
Created attachment 540484 [details] [diff] [review]
Some changes to RefPtr code

In order to make usage sane I had to make some changes (for example to allow return NULL from a TemporaryRef<T> function and to return without forget).

I also think I found a way to allow & to be used both for T** and RefPtr<T>*. Let me know what you think.

For a stand alone build I also had to prevent Util dragging JS stuff in.
Comment 40 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-20 10:18:34 PDT
Comment on attachment 540484 [details] [diff] [review]
Some changes to RefPtr code

Review of attachment 540484 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/RefPtr.h
@@ +40,5 @@
>  
>  #ifndef mozilla_RefPtr_h_
>  #define mozilla_RefPtr_h_
>  
> +#ifndef MFBT_STAND_ALONE

I'm not currently getting any hits in m-c MXR for this.  What is it?

@@ +45,4 @@
>  #include "mozilla/Util.h"
> +#else
> +#define MOZ_ASSERT
> +#define MOZ_ALWAYS_INLINE inline

Is this some sort of workaround for mfbt not yet having its own library?  It's going to need to happen sooner or later.  If that's what you're doing, we should bite the bullet and make it happen, not hack out assertions and change desired inlining semantics.
Comment 41 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-20 12:24:22 PDT
(In reply to comment #39)
> For a stand alone build I also had to prevent Util dragging JS stuff in.

What breaks if that happens?
Comment 42 Bas Schouten (:bas.schouten) 2011-06-20 20:05:48 PDT
(In reply to comment #40)
> Comment on attachment 540484 [details] [diff] [review] [review]
> Some changes to RefPtr code
> 
> Review of attachment 540484 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: mfbt/RefPtr.h
> @@ +40,5 @@
> >  
> >  #ifndef mozilla_RefPtr_h_
> >  #define mozilla_RefPtr_h_
> >  
> > +#ifndef MFBT_STAND_ALONE
> 
> I'm not currently getting any hits in m-c MXR for this.  What is it?
> 
> @@ +45,4 @@
> >  #include "mozilla/Util.h"
> > +#else
> > +#define MOZ_ASSERT
> > +#define MOZ_ALWAYS_INLINE inline
> 
> Is this some sort of workaround for mfbt not yet having its own library? 
> It's going to need to happen sooner or later.  If that's what you're doing,
> we should bite the bullet and make it happen, not hack out assertions and
> change desired inlining semantics.

That's more or less what it was, only defined when including from the Azure stand-alone build. But I found a way to make it work without adding this define so consider these lines gone, as this obviously was far from ideal. (The only symbol actually needed was JS_Assert, so for the stand-alone Azure build I know #undef DEBUG before including it, which makes that go away). The latter also isn't pretty, but limits the unprettiness to Azure and only when Azure is built stand-alone  (which is never the case inside the mozilla tree). It also doesn't introduce a global define which cjones rightfully objected to.
Comment 43 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-20 20:06:21 PDT
Comment on attachment 540484 [details] [diff] [review]
Some changes to RefPtr code

I'm not against this a priori, but it turns out to have problems that
I don't see a solution too.  See below.

(There would be much to fix in the early part of the patch.  But see
below.)

>@@ -244,20 +255,20 @@ template<typename T>
> class OutParamRef
> {
>     friend OutParamRef byRef<T>(RefPtr<T>&);
> 
> public:
>     ~OutParamRef() { refPtr = tmp; }
> 
>     operator T**() { return &tmp; }
>+    operator RefPtr<T>*() { return refPtr.addr(); }

This is the crux of the patch.  It has a fundamental problem: if you
hand out &tmp, you need to do one thing in ~OutParamRef(), but if you
hand out refPtr.addr(), you have to do another thing.  I don't see any
way to implement both behaviors with the same code.

That means OutParamRef needs to remember which pointer it handed out.
I cooked up a test for this, see forthcoming patch for remaning
comments.
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-20 20:08:59 PDT
Created attachment 540667 [details] [diff] [review]
Track which ptr was handed out from OutParamRef

This patch works in the sense of being correct, but gcc -O2 isn't always able to optimize away the |which| indirection needed for correctness.  So I think this is a no-go.
Comment 45 Bas Schouten (:bas.schouten) 2011-06-20 20:18:09 PDT
(In reply to comment #44)
> Created attachment 540667 [details] [diff] [review] [review]
> Track which ptr was handed out from OutParamRef
> 
> This patch works in the sense of being correct, but gcc -O2 isn't always
> able to optimize away the |which| indirection needed for correctness.  So I
> think this is a no-go.

How strongly do you feel about not handing out the l-value..? Or creating another intermediate class that is handed out to T**, that does it on its destructor? That would solve the problem.
Comment 46 Bas Schouten (:bas.schouten) 2011-06-20 20:40:03 PDT
Created attachment 540670 [details] [diff] [review]
Add additional intermediate class

This should work correctly and always optimize to the correct code.

Also uses your obviously better method of just returning 'this' :P.
Comment 47 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-21 02:18:54 PDT
Comment on attachment 540670 [details] [diff] [review]
Add additional intermediate class

This patch doesn't actually compile, but with a missing forward-decl added:

test.cc: In function ‘int main(int, char**)’:
test.cc:413: error: cannot convert ‘mozilla::OutParamRef<Foo>’ to ‘mozilla::RefPtr<Foo>*’ for argument ‘1’ to ‘void GetNewFoo(mozilla::RefPtr<Foo>*)’
test.cc:420: error: cannot convert ‘mozilla::OutParamRef<Foo>’ to ‘mozilla::RefPtr<Foo>*’ for argument ‘1’ to ‘void GetPassedFoo(mozilla::RefPtr<Foo>*)’

I'm not sure how the compiler is supposed to get from OutParamRef to RefPtr*.  If RefPtr would have had |operator OutParam()| instead, then there would be a different problem: the compiler would refuse to deduce OutParam->OutParamRef->T**.

So still feedback-.  If you can show that the previous version of the patch with the explicit |which| always has no overhead for RefPtr<T>* outparams in gcc and MSVC, and the JS guys are OK with taking it, we could roll with that.  Otherwise I think COM gets the short end of the stick, sorry.
Comment 48 Bas Schouten (:bas.schouten) 2011-06-21 07:14:01 PDT
(In reply to comment #47)
> Comment on attachment 540670 [details] [diff] [review] [review]
> Add additional intermediate class
> 
> This patch doesn't actually compile, but with a missing forward-decl added:
> 
> test.cc: In function ‘int main(int, char**)’:
> test.cc:413: error: cannot convert ‘mozilla::OutParamRef<Foo>’ to
> ‘mozilla::RefPtr<Foo>*’ for argument ‘1’ to ‘void
> GetNewFoo(mozilla::RefPtr<Foo>*)’
> test.cc:420: error: cannot convert ‘mozilla::OutParamRef<Foo>’ to
> ‘mozilla::RefPtr<Foo>*’ for argument ‘1’ to ‘void
> GetPassedFoo(mozilla::RefPtr<Foo>*)’
> 
> I'm not sure how the compiler is supposed to get from OutParamRef to
> RefPtr*.  If RefPtr would have had |operator OutParam()| instead, then there
> would be a different problem: the compiler would refuse to deduce
> OutParam->OutParamRef->T**.
> 
> So still feedback-.  If you can show that the previous version of the patch
> with the explicit |which| always has no overhead for RefPtr<T>* outparams in
> gcc and MSVC, and the JS guys are OK with taking it, we could roll with
> that.  Otherwise I think COM gets the short end of the stick, sorry.

OK, yeah, I actually had a slightly different patch (hg fail on my side), but having done some proper testing I realize it doesn't do refcounting correctly. So that's not an option.

Are you okay with the rest of the changes? (will attach a patch soon)
Comment 49 Bas Schouten (:bas.schouten) 2011-06-21 07:43:16 PDT
Created attachment 540750 [details] [diff] [review]
Proposed RefPtr additions

These are the changes I'm still proposing:

TemporaryRef public T** constructor to allow returning an addrefed but non-refPtr object or NULL.

TemporaryRef operator overload to allow just returning the refptr directly from a function without a forget.
Comment 50 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-21 11:28:58 PDT
Comment on attachment 540750 [details] [diff] [review]
Proposed RefPtr additions

This is what we discussed in comment 36 and comment 37 ... do you have new arguments or evidence for the discussion?
Comment 51 Bas Schouten (:bas.schouten) 2011-06-21 16:50:44 PDT
(In reply to comment #50)
> Comment on attachment 540750 [details] [diff] [review] [review]
> Proposed RefPtr additions
> 
> This is what we discussed in comment 36 and comment 37 ... do you have new
> arguments or evidence for the discussion?

The operator overload is, because it looks a load better, the .forget syntax is silly. It prevents me from having to add .forget all over the place adding clutter to the code that really doesn't need to be there. Considering TemporaryRef is used for function return types, having that operator overload seems very natural. I really don't see the objection.
Comment 52 Bas Schouten (:bas.schouten) 2011-06-21 18:18:49 PDT
I found another flaw (and a flaw in my patch).

The first is that the OutParamRef assumes there's no references on the value it receives. In practice in COM (both Moz and MS) the 'getter_AddRefs'. In other words the outparamref should not addref the value it receives as it does currently.

The second is with the TemporaryRef T* constructor, I should RefPtr<T>::ref() the value passed in. That allows you to do the following:

TemporaryRef<Foo>
ReturnNewFoo()
{
  return new Foo();
}
Comment 53 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-21 21:35:48 PDT
(In reply to comment #52)
> I found another flaw (and a flaw in my patch).
> 
> The first is that the OutParamRef assumes there's no references on the value
> it receives. In practice in COM (both Moz and MS) the 'getter_AddRefs'. In
> other words the outparamref should not addref the value it receives as it
> does currently.

Yeah, that's my fault.

> The second is with the TemporaryRef T* constructor, I should
> RefPtr<T>::ref() the value passed in. That allows you to do the following:
> 
> TemporaryRef<Foo>
> ReturnNewFoo()
> {
>   return new Foo();
> }

That looks fine to me.
Comment 54 Bas Schouten (:bas.schouten) 2011-06-21 21:44:38 PDT
Created attachment 540967 [details] [diff] [review]
Proposed RefPtr additions v2

New patch, including a comment and the aforementioned fixes.
Comment 55 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-21 22:41:03 PDT
Comment on attachment 540967 [details] [diff] [review]
Proposed RefPtr additions v2

This patch has a leak bug that would have been caught by the, you know, tests ;).
Comment 56 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-21 22:42:52 PDT
Created attachment 540971 [details] [diff] [review]
Followup to bug 661973: Fix bug with COM outparams and add convenience operators

v2 with fixups
 - fixes leak on TemporaryRef return values
 - restores GetPassedFoo(T**) test, which should work if callers adhere to COM conventions
 - test for |return 0| to TemporaryRef return value

This is all exported and ready to go, with your name as the patch author.
Comment 57 Bas Schouten (:bas.schouten) 2011-06-21 22:52:32 PDT
Follow up pushed to mozilla-inbound

http://hg.mozilla.org/integration/mozilla-inbound/rev/ca61a0de54f1
Comment 58 :Ms2ger (⌚ UTC+1/+2) 2011-06-22 09:09:11 PDT
Why didn't you ask for sr when you reverted one of the superreviewer's comments?
Comment 59 Matt Brubeck (:mbrubeck) 2011-06-22 10:20:41 PDT
Followup landed on mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/ca61a0de54f1
Comment 60 Bas Schouten (:bas.schouten) 2011-06-22 10:54:50 PDT
(In reply to comment #58)
> Why didn't you ask for sr when you reverted one of the superreviewer's
> comments?

The superreviewer explicitly left the decision to the patch author. Who was the reviewer on this patch.
Comment 61 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-06-22 11:48:54 PDT
I'm also a superreviewer.

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