Status

()

defect
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: botond, Assigned: botond)

Tracking

Trunk
mozilla42
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(3 attachments)

Assignee

Description

5 years ago
Sometimes, I use nsRefPtr to manage the lifetime of an object, but I want to express that in some part of the code, that object should not be modified (in the sense that non-const methods should not be called on it).

It would be great if we could write 'nsRefPtr<const T>' to express this.

I attempted to write 'nsRefPtr<const T>' with our current implementation, and ran into a problem because AddRef() and Release() were not const methods.

I initially attempted to work around this problem by writing my own AddRef() and Release() methods which were 'const' (and correspondingly, the refcount variable they manipulated was 'mutable').

This worked fine, but Ehsan raised an objection that destroying the object, as Release() might do, is conceptually a modification of the object, and we are subverting the concept of constness by doing this.

So I propose an alternative, where we specialize nsRefPtr<const T> such that it:

  - Stores a non-const T* internally.

  - When constructed from a raw pointer, requires the raw pointer to be
    non-const.

  - Returns a const T* in its conversion operator and get() functions.

This way, AddRef() and Release() could be unchanged and they would not be deleting (or doing whatever else) to a const T* - thus addressing Ehsan's objection - but users of the nsRefPtr could not call non-const methods on the object - thus addressing my use case.

Any thoughts?

Comment 1

5 years ago
I'm not sure I agree with Ehsan on this one. You can `delete` a const object, and Release() is basically a form of refcounted deletion. So *if* we decide to expose a const interface on refcounted objects, I think it makese sense for AddRef and Release to be const methods.

However, I'm skeptical of the value of refcounted objects having a const interface in general. Having a const-correct API which provides useful API guarantees is often hard, and mostly useful for parameter references where passing by value is too expensive.

In either case, I don't think I agree with the proposal at hand to specialize nsRefPtr.

NI?ehsan so he has a chance to disagree with me!
Flags: needinfo?(ehsan)
FWIW, I agree with Botond here, and I think it's useful and important.

I agree because the problem (the need for `mutable`) should be seen as an accidental side effect of our refcounting being intrusive, i.e. the refcount being stored as a member in the refcounted object. That problem is sidestepped altogether in non-intrusive refcounting implementations, such as in the C++ STL.

C++ 'constness' is not fine-grained enough for our needs here; that is exactly what the 'mutable' keyword is there for.

I think that's important and useful because holding strong references is often useful, constness is often useful, so it's unfortunate to have to choose only one among these two useful things.
By the way, what I agree with is Botond's original proposal based on mutable. The other proposal, to specialize nsRefPtr<const T>, sounds much more complicated, requires separate work for each refptr class (nsRefPtr vs nsCOMPtr vs MFBT RefPtr...) and I still don't see a problem in the original 'mutable' proposal.
On the question of the value of trying to come up with good const correct API designs, I have mostly given up on that idea for years myself, but I do appreciate others wanting to use more const correct APIs than I usually deal with.  So while this proposal won't immediately be useful to myself, I have no reason to prevent others from wanting it if they do.

But about the proposal at hand, to me the fact that you can delete pointers to const objects in C++ is a mistake (and also a by-product of the language constraints), one that I don't think we need to replicate in our code.  I think a logically const operation doesn't modify the object's state, and killing an object _is_ a modification to its state (as the object is no longer alive.)  We did have a rather lengthy discussion about this with two other people the other day at the office and basically half of people agreed with me, and the other half agreed with Botond.  I think it's entirely subjective, so I don't know if one of us can convince the other on that.

About comment 3, nsCOMPtr is designed for classes inheriting from nsISupports which has non-const AddRef and Release, so this proposal wouldn't apply to it, and the MFBT RefPtr is a pointless class which I would really like to kill very soon, so I don't think that this proposal applies to anything beyond nsRefPtr, so I don't think it's as complicated as it may first seem like.
Flags: needinfo?(ehsan)
Assignee

Comment 5

5 years ago
I'm OK with either approach (specializing, or making AddRef/Release 'const') - I'd just like to get nsRefPtr<const T> working and begin benefitting from the additional compiler checking that it will lend us.

Nathan, which approach would you prefer?
Flags: needinfo?(nfroyd)
On a theoretical level, I think I prefer the |mutable| refcount approach.

On a practical level, though, I am concerned about the edge case of:

class nsFoo : public nsIFoo
{
  NS_DECL_ISUPPORTS
  NS_DECL_IFOO
  ...
};

and making |nsRefPtr<const nsFoo>| do the right thing.  And I think to make that work, the template specialization approach would need to be taken.  Or do folks think that's not worth worrying about, given that we have less nsIFoo stuff floating around nowadays?

I'm skeptical that trying to introduce const-correctness here is going to win us a lot for the complexity required for nsRefPtr (and, I think, by extension, already_AddRefed?).  Can you give a sketch of code that can be improved significantly with nsRefPtr<const T>?
Flags: needinfo?(nfroyd) → needinfo?(botond)
Assignee

Comment 7

5 years ago
(In reply to Nathan Froyd (:froydnj) from comment #6)
> On a theoretical level, I think I prefer the |mutable| refcount approach.
> 
> On a practical level, though, I am concerned about the edge case of:
> 
> class nsFoo : public nsIFoo
> {
>   NS_DECL_ISUPPORTS
>   NS_DECL_IFOO
>   ...
> };
> 
> and making |nsRefPtr<const nsFoo>| do the right thing.  And I think to make
> that work, the template specialization approach would need to be taken.

To clarify, the potential problem here is that nsISupports::AddRef() and Release() are not 'const'. If we can change them to 'const' without problems, then the "mutable refcount" approach should still be feasible even for classes that derive from nsISupports. I'll give this a try.

> I'm skeptical that trying to introduce const-correctness here is going to
> win us a lot for the complexity required for nsRefPtr (and, I think, by
> extension, already_AddRefed?).  Can you give a sketch of code that can be
> improved significantly with nsRefPtr<const T>?

The use case I have in mind is:

  - One piece of code constructs an object and passes it to another piece of code.

  - We want to express that this second piece of code should not modify the object,
    hence the 'const' part.

  - The object might be referred to from several places with different lifetimes,
    and we want the object to stay alive while any references to it exist, hence
    the 'nsRefPtr' part.

I currently use an nsRefPtr<const T> in APZ code [1] (by writing by own AddRef/Release methods). You can have a look at how that field is set and used to get a picture of a concrete use of this.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/InputBlockState.h?rev=9a507b307d1d#30
Flags: needinfo?(botond)
I can't figure out what the concern I was raising in comment 6 was...maybe nsFoo was supposed to be a base class?  And I note that in bug 912299, I advocated going the |mutable| refcount route.

But I discovered yesterday a very practical reason for doing this: RefCounted<T>'s AddRef/Release are const methods, and so people are using RefCounted<T> in preference to XPCOM's refcounting macros to enable this use case.  So, that seems like an argument to fix the issue raised in this bug, as we'd like to limit RefCounted<T> use cases to as few places as possible.

Botond, are you willing to to implement one or both of the approaches described here and see what breaks?
Flags: needinfo?(botond)
Assignee

Comment 9

4 years ago
Sure. I'll give the |mutable| refcount a try, as that seems like the nicer / more straightforward approach.

Leaving needinfo until I actually do it.
Assignee

Comment 10

4 years ago
So I tried playing around with this, and one of the first things I ran into is that some of the AddRef() implementations cast the |this| pointer to |void*| to pass to various things.

Some of these things, like NS_LogAddRef() / NS_LogRelease() can easily be changed to take |const void*| instead. For others, I'm less sure. For example, NS_IMPL_CC_NATIVE_ADDREF_BODY() passes a |void*| to nsCycleCollectingAutoRefCnt::incr(). Trying to change that to |const void*| and chasing that down the rabbit-hole leads to things like having to make nsISupports::QueryInterface() |const| [1].

Do you think that's a useful direction to go down, or would it perhaps be appropriate to |const_cast| somewhere along the line?

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsCycleCollector.cpp?rev=40bf29309bf5#930
Flags: needinfo?(botond) → needinfo?(nfroyd)
I don't entirely understand what is going on in this bug, but the pointer that is passed into nsCycleCollectingAutoRefCnt::incr() is certainly not const. Eventually, the cycle collector can pass it to an unlink method if it decides the object is garbage, which will clear pointers on the object. So you aren't going to be able to propagate const everywhere. Maybe in the call to incr() is the place to do a const_cast if you were to do one.
Assignee

Comment 12

4 years ago
Discussed this with :mccr8 over IRC; we concluded that const_casting going into incr() and decr() should be safe because the only way the object itself (as opposed to just a reference/pointer to it) would be const is it it were allocated via something like 'new const T', which doesn't make sense for cycle-collected types to begin with.
Flags: needinfo?(nfroyd)
Assignee

Comment 13

4 years ago
So I've been working on the "make AddRef() and Release() |const|" approach, and I'm not really liking how it's turning out. There are a *lot* of places in the codebase where we declare or define these methods, and a lot of implementations (particularly of Release()) that perform non-const operations on the object. Having to |const_cast| in all these places (and imposing the need to do so on future classes) is rather unfortunate.

What do you think if we, instead, left AddRef() and Release() as non-const methods, and |const_cast|ed |nsRefPtr::mRawPtr| in the places where nsRefPtr calls these methods, to accomnodate the case where |mRawPtr| is const? I expected that would reduce the scope of this change from hundreds of changes to dozens of files, to a handful of changes in one file.
Flags: needinfo?(nfroyd)
(In reply to Botond Ballo [:botond] from comment #13)
> What do you think if we, instead, left AddRef() and Release() as non-const
> methods, and |const_cast|ed |nsRefPtr::mRawPtr| in the places where nsRefPtr
> calls these methods, to accomnodate the case where |mRawPtr| is const? I
> expected that would reduce the scope of this change from hundreds of changes
> to dozens of files, to a handful of changes in one file.

This seems reasonable to me, perhaps with an explanatory comment about why we're doing it this way.
Flags: needinfo?(nfroyd)
Assignee

Comment 15

4 years ago
I have this working locally on Linux. A Try push is showing some compiler errors on other platforms which I need to investigate: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d1b6b0695ad
Assignee

Comment 16

4 years ago
The failures turned out to be unrelated (caused by another patch in my local tree). Here's a Try push with just these patches which is looking good:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b69f9ec3af00

Posting patches for review.
Assignee

Comment 17

4 years ago
Bug 1056356 - Add support for nsRefPtr<const T>. r=froydnj
Attachment #8641811 - Flags: review?(nfroyd)
Assignee

Comment 18

4 years ago
Bug 1056356 - Allow calling NewRunnableMethod() with a const pointer to the callee object. r=froydnj
Attachment #8641812 - Flags: review?(nfroyd)
Assignee

Comment 19

4 years ago
Bug 1056356 - Remove the hand-rolled mechanism used to get nsRefPtr<const OverscrollHandoffChaiin> to work. r=kats
Attachment #8641813 - Flags: review?(bugmail.mozilla)
Attachment #8641813 - Flags: review?(bugmail.mozilla) → review+
Comment on attachment 8641813 [details]
MozReview Request: Bug 1056356 - Remove the hand-rolled mechanism used to get nsRefPtr<const OverscrollHandoffChaiin> to work. r=kats

https://reviewboard.mozilla.org/r/14615/#review13209

\o/
Comment on attachment 8641811 [details]
MozReview Request: Bug 1056356 - Add support for nsRefPtr<const T>. r=froydnj

https://reviewboard.mozilla.org/r/14611/#review13267

There seems to be a fair amount of trailing whitespace in your patch.  Please fix those instances up before committing.
Attachment #8641811 - Flags: review?(nfroyd) → review+
Comment on attachment 8641812 [details]
MozReview Request: Bug 1056356 - Allow calling NewRunnableMethod() with a const pointer to the callee object. r=froydnj

https://reviewboard.mozilla.org/r/14613/#review13269

Ship It!
Attachment #8641812 - Flags: review?(nfroyd) → review+
Assignee

Comment 23

4 years ago
Removed trailing whitespace and landed.
https://hg.mozilla.org/mozilla-central/rev/eeab332c767c
https://hg.mozilla.org/mozilla-central/rev/d22e3da3ebcf
https://hg.mozilla.org/mozilla-central/rev/27d20d1b6f9a
Assignee: nobody → botond
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
You need to log in before you can comment on or make changes to this bug.