Closed Bug 1179451 Opened 10 years ago Closed 10 years ago

disallow implicit conversion to T* from nsRefPtr<T>&& values

Categories

(Core :: XPCOM, defect)

Product:
Core
Component:
XPCOM
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: froydnj, Assigned: ayg)

References

(Depends on 3 open bugs)

Details

Attachments

(7 files, 3 obsolete files)

disallow implicit conversion to T* from nsCOMPtr<T>&& values
31.92 KB, patch
Details | Diff | Splinter Review
Part 1 -- Rewrite some ternary operators as if/else
10.32 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 2 -- Fix incorrect return type
1.66 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Part 3 -- Use .get() to assign nsRefPtrs to raw pointers
1.86 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 4, option 1 -- Don't pass nsRefPtr&& to functions that want raw pointers
9.40 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 5 -- Delete nsRefPtr<T>::operator T*()&&
2.70 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
Part 4, option 2, v3 -- Introduce RefParam
46.33 KB, patch
Details | Diff | Splinter Review
Various people have proposed this for compilers which are new enough. Let's try it and see what breaks.
This patch is my first attempt and there's a surprising amount that breaks. This patch is also *incomplete*. Notable categories of failures: 1) We construct temporary nsCOMPtr<> for function arguments...and then expect those to convert to T*. Explicit get() calls are needed. 2) Ternary expressions involving nsCOMPtr<>, because: booleanExpr ? mSmartPtr : nullptr I am not enough of a language lawyer to describe exactly what happens here, but I infer from the failure that we are converting nullptr to nsCOMPtr<>, so that both arms of the ternary have the same type, and them implicitly converting to T* after the proper arm has been selected. Again, this requires an explicit get() call. 3) Runnable methods like: NS_RunnableMethodWithArg<>(this, &Class::MethodTakingRawPointer, localCOMPtr) fails, because we have an implicit conversion to T* in the runnable's call to MethodTakingRawPointer. Typically this is fixed by making the method accept |const nsCOMPtr<>&|, which many people will probably find preferable. 4) We sometimes construct temporary nsCOMPtr<> to hold the results of an already_AddRefed'd-returning function, then implicitly convert that to T*. Again, explicit get() calls are our friend. The first one does not bother me all that much, because doing so is an unusual practice. The second one is slightly cryptic, but I don't think it'd be too much of a problem... The third one I think I could live with. The fourth one seems like we ought to be handling those cases differently anyway... Anyway, comments welcome, and people picking up the patch even more so. There may yet be other categories of failures lurking...
Depends on: 1179787
Depends on: 1181164
This patch can't land on central, as it uses ref qualified methods, which don't compile on the versions of gcc or MSVC which we support, see https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code. I think that there are other ways which we can deal with these problems, ranging in extremeness, but for now we can use something like this as a way to discover problems (like bug 1181164). nfroyd, do you think we need to look into other ways to deal with this problem?
Flags: needinfo?(nfroyd)
(In reply to Michael Layzell [:mystor] from comment #2) > nfroyd, do you think we need to look into other ways to deal with this > problem? Other than some sort of static analysis, I don't think there's a good way to address this currently.
Flags: needinfo?(nfroyd)
FWIW, Visual Studio 2015 will support ref-qualifiers [1], so perhaps we can revisit that possibility when we eventually switch. [1] http://blogs.msdn.com/b/vcblog/archive/2015/06/19/c-11-14-17-features-in-vs-2015-rtm.aspx
(In reply to Michael Layzell [:mystor] from comment #2) > This patch can't land on central, as it uses ref qualified methods, which > don't compile on the versions of gcc or MSVC which we support, see > https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code. is there any reason we can't add a MOZ_HAVE_REF_QUALIFIERS macro defined appropriately and condition the bits of this patch that need it on that? that seems pretty simple. Not perfect sure, but I expect in practice most people are using compilers that support ref qualifiers so most people should find the bustage locally.
(In reply to Trevor Saunders (:tbsaunde) from comment #5) > is there any reason we can't add a MOZ_HAVE_REF_QUALIFIERS macro defined > appropriately and condition the bits of this patch that need it on that? > that seems pretty simple. Not perfect sure, but I expect in practice most > people are using compilers that support ref qualifiers so most people should > find the bustage locally. But for people who don't have such compilers, they risk bustage when pushing their patches, which is not very nice. We should strive for "works fine without, works better with" when having compiler version-specific features like this, and this one doesn't qualify.
(In reply to Trevor Saunders (:tbsaunde) from comment #5) > is there any reason we can't add a MOZ_HAVE_REF_QUALIFIERS macro defined > appropriately and condition the bits of this patch that need it on that? > that seems pretty simple. Not perfect sure, but I expect in practice most > people are using compilers that support ref qualifiers so most people should > find the bustage locally. This is true, I was actually talking about that possibility with ehsan. The problem is that it would be possible to introduce code which will pass our testing infrastructure (have code which uses this method in windows-specific code), and then have that break building on more recent versions of MSVC. I feel like if the code passes on our testing infra, it should compile on the most recent MSVC too.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6) > But for people who don't have such compilers, they risk bustage when pushing > their patches, which is not very nice. We should strive for "works fine > without, works better with" when having compiler version-specific features > like this, and this one doesn't qualify. We already have that (working locally, but breaking on try) as the case with static analysis (most people, even when building with clang, build with the plugin disabled), the problem in this case is that incorrect might _not_ break try/CI (as I think try uses the old version of MSVC), but would break users of the better, more recent compilers. Which is not OK.
(In reply to Michael Layzell [:mystor] from comment #7) > (In reply to Trevor Saunders (:tbsaunde) from comment #5) > > is there any reason we can't add a MOZ_HAVE_REF_QUALIFIERS macro defined > > appropriately and condition the bits of this patch that need it on that? > > that seems pretty simple. Not perfect sure, but I expect in practice most > > people are using compilers that support ref qualifiers so most people should > > find the bustage locally. > > This is true, I was actually talking about that possibility with ehsan. > > The problem is that it would be possible to introduce code which will pass > our testing infrastructure (have code which uses this method in > windows-specific code), and then have that break building on more recent > versions of MSVC. I feel like if the code passes on our testing infra, it > should compile on the most recent MSVC too. I think there's already ways you can write code that is fine with the version of msvc in automation, but broken with newer msvc. I'm not saying this is great, but it seems better than having a separate static analysis.
(In reply to Michael Layzell [:mystor] from comment #8) > (as I think try uses the old version of MSVC) As of bug 1117900, everything in automation should be using at least MSVC 2013 Update 3.
(In reply to Emanuel Hoogeveen [:ehoogeveen] from comment #10) > As of bug 1117900, everything in automation should be using at least MSVC > 2013 Update 3. According to https://developer.mozilla.org/en-US/docs/Using_CXX_in_Mozilla_code we need at least MSVC 2015 to use these.
Ah, I misunderstood what you meant. Visual Studio 2015 isn't out yet (its release date is July 20th); everything in automation is on Visual Studio 2013 right now.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #1) Just about this point: > Notable categories of failures: > [...] > 3) Runnable methods like: > NS_RunnableMethodWithArg<>(this, &Class::MethodTakingRawPointer, > localCOMPtr) > fails, because we have an implicit conversion to T* in the runnable's call > to MethodTakingRawPointer. Typically this is fixed by making the method > accept |const nsCOMPtr<>&|, which many people will probably find > preferable. I'm assuming you mean NS_NewRunnableMethodWithArg or NS_NewRunnableMethodWithArgs. First, the argument type (or storage class) should be compulsory; I.e., it shouldn't compile without it -- If it does, it's a bug! If that argument is a ref-counted type, the chosen storage class should be StorensRefPtrPassByPtr, which passes the argument by doing an explicit get(), so we should be good there. However if the argument is an nsRefPtr<T> or nsCOMPtr<T>, the storage class will just be StoreCopyPassByValue, which would explain the implicit conversion to T*. To fix these, either we could remove nsRefPtr/nsCOMPtr from the argument types in all NewRunnableMethodWithArg[s] calls -- Not sure whether a nsCOMPtr'd object can be stored in an nsRefPtr, I don't know enough about these things (yet). Or, I think it would be easy to add storage classes/tests so that we do an explicit get() on these shared pointers -- Though it might not be what we want if the target function actually wants an nsRefPtr/nsCOMPtr? Please let me know what you think, and if I can help with this.
(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #6) > But for people who don't have such compilers, they risk bustage when pushing > their patches, which is not very nice. This is common already. It's not really so different from having a test fail when you push to try because you didn't run it locally. It's best to minimize this type of thing, but I don't think any of the outlawed code patterns are so common that it would noticeably reduce anyone's efficiency. If these were types of code that got introduced all the time, then yes, I would agree that the annoyingness might outweigh the benefit. (In reply to Michael Layzell [:mystor] from comment #7) > The problem is that it would be possible to introduce code which will pass > our testing infrastructure (have code which uses this method in > windows-specific code), and then have that break building on more recent > versions of MSVC. I feel like if the code passes on our testing infra, it > should compile on the most recent MSVC too. This can only occur if reasonably uncommon (I think) constructs are used in a very small fraction of the codebase (Windows-specific). I don't think it's going to cause too many problems. With warnings-as-errors, for instance, you can already get this sort of thing when a compiler fixes a warning to cover more cases. (I don't have a specific example handy, but I've heard of it happening.) IIRC, the nsresult-as-enum-class change was ifdefed off for a long time on some supported compilers, and I don't recall any complaints. All this does depend somewhat on how common the outlawed constructs are, but I doubt it's much more common than nsresult abuse. If I'm wrong and people are introducing new uses of these constructs all the time, then yes, it would be best to wait for better compiler support.
It may be interesting to try this out and see how many existing places will fail to compile. That should give us a better sense about the extent of the issue.
Is there any easy way to get a list of all the errors without having to fix them a few at a time to get the compile to continue?
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor (inactive until July 27) from comment #16) > Is there any easy way to get a list of all the errors without having to fix > them a few at a time to get the compile to continue? Not that I know of, unfortunately.
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor (inactive until July 27) from comment #16) > Is there any easy way to get a list of all the errors without having to fix > them a few at a time to get the compile to continue? I was thinking of trying to mark the method which we want to delete as deprecated (you can do this in clang with something like this: http://clang.llvm.org/docs/LanguageExtensions.html#messages-on-deprecated-and-unavailable-attributes), and then grepping through the output to find every deprecated warning with the message you defined. You can try that.
Do you know how to fix this error? I haven't the foggiest idea what it means. A bunch of such errors occur when I try to compile a patch along these lines (I didn't try your exact patch). 0:16.50 In file included from /mnt/ssd/checkouts/central/netwerk/base/BackgroundFileSaver.cpp:21:0, 0:16.50 from /mnt/ssd/checkouts/central/obj/netwerk/base/Unified_cpp_netwerk_base0.cpp: 11: 0:16.50 ../../dist/include/nsThreadUtils.h: In instantiation of ‘void nsRunnableMethodArguments<T0>::app ly(C*, M) [with C = mozilla::net::Dashboard; M = nsresult (mozilla::net::Dashboard::*)(mozilla::net::Conn ectionData*); T0 = nsRefPtr<mozilla::net::ConnectionData>]’: 0:16.50 ../../dist/include/nsThreadUtils.h:820:7: required from ‘nsresult nsRunnableMethodImpl<Method, Owning, Storages>::Run() [with Method = nsresult (mozilla::net::Dashboard::*)(mozilla::net::ConnectionData*); bool Owning = true; Storages = {nsRefPtr<mozilla::net::ConnectionData>}]’ 0:16.50 /mnt/ssd/checkouts/central/netwerk/base/TLSServerSocket.cpp:476:1: required from here 0:16.50 ../../dist/include/nsThreadUtils.h:626:5: error: use of deleted function ‘nsRefPtr<T>::operator T*() && [with T = mozilla::net::ConnectionData]’ 0:16.50 ((*o).*m)(m0.PassAsParameter()); 0:16.50 ^ 0:16.50 In file included from ../../dist/include/nsAutoPtr.h:11:0, 0:16.50 from ../../dist/include/nsHashKeys.h:13, 0:16.50 from /mnt/ssd/checkouts/central/netwerk/base/nsNetUtil.h:16, 0:16.50 from /mnt/ssd/checkouts/central/netwerk/base/BackgroundFileSaver.cpp:20, 0:16.51 from /mnt/ssd/checkouts/central/obj/netwerk/base/Unified_cpp_netwerk_base0.cpp:11: 0:16.51 ../../dist/include/nsRefPtr.h:254:3: error: declared here 0:16.51 operator T*() && = delete; 0:16.51 ^ 0:16.51
Flags: needinfo?(nfroyd)
The problem is that there's code somewhere that says: NS_RunnableMethodWithArguments<nsRefPtr<ConnectionData>>(..., &SomeMethod); with SomeMethod declared like so: nsresult Dashboard::SomeMethod(ConnectionData*) NS_RunnableMethodWithArguments's runnable is storing that nsRefPtr<ConnectionData> as an nsRefPtr<>, but it's also trying to pass it to SomeMethod as an nsRefPtr<> (nsRefPtr<> gets returned from the PassAsParameter method the error message is complaining about). This works today, because we'll happily implicitly convert the temporary nsRefPtr<ConnectionData> return value into a ConnectionData*. But it's inefficient, and caught by this new thing you're trying to implement. Bug 1179787 tracks fixing this bit in NS_RunnableMethod*. Ideally I will get those patches cleaned up today or tomorrow and into the tree.
Flags: needinfo?(nfroyd)
Okay, great. When you do, I'll pick up this bug and fix the other problems. It doesn't look like it will be so many problems to fix, if I had to guess.
I think this is nicer than .get(). .get() should be a bit scary and I'd like to only use it if necessary.
Assignee: nfroyd → ayg
Status: NEW → ASSIGNED
Attachment #8642411 - Flags: review?(nfroyd)
There's no reason I see why this shouldn't return a raw pointer. One existing caller already assigns the result to a raw pointer (which is why I'm changing it).
Attachment #8642413 - Flags: review?(nfroyd)
These look like .get() is the best way to fix them. They're actually doing something potentially unsafe, so it should be explicit.
Attachment #8642414 - Flags: review?(nfroyd)
There are two ways to fix this problem. This way is the unsustainable hack, IMO. But it's simple and it works for now, so I'm submitting it as an option. I preferred to avoid .get() where practical (e.g., changing return type to already_AddRefed). The MozPromiseRequestHolder::Begin thing is because there are a ton of places that pass an nsRefPtr and I didn't want to add .get() to all of them. The reason that IMO this isn't a good solution is because it's perfectly safe to pass nsRefPtr<T>&& to a function that wants T*, because the nsRefPtr will outlive the function call. Thus the next solution.
Attachment #8642417 - Flags: review?(nfroyd)
This is the exciting option! The comments in RefParam.h should be self-explanatory. If this option is selected, new code should start using RefParam<T>/RawParam<T> instead of T* for parameters. In theory I think it should be possible to update XPCOM IDLs to use RefParam instead of raw pointers; see discussion in bug 767178 comment 31 and on, particularly comment 35.
Attachment #8642418 - Flags: review?(nfroyd)
This compiles locally. It presumably doesn't pass try yet. I thought submitting it for review first made more sense in this case, since I'm not sure everyone agrees yet that we want it.
Attachment #8642420 - Flags: review?(nfroyd)
Comment on attachment 8642411 [details] [diff] [review] Part 1 -- Rewrite some ternary operators as if/else Review of attachment 8642411 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/events/PaintRequest.h @@ +91,5 @@ > aFound = aIndex < mArray.Length(); > + if (aFound) { > + return mArray.ElementAt(aIndex); > + } > + return nullptr; Minor consistency nit: everywhere else, you seem to prefer: if (!aFound) { return nullptr; } return mArray.ElementAt(aIndex);
Attachment #8642411 - Flags: review?(nfroyd) → review+
Comment on attachment 8642413 [details] [diff] [review] Part 2 -- Fix incorrect return type Review of attachment 8642413 [details] [diff] [review]: ----------------------------------------------------------------- I'm not a peer of this code, but I'm inclined to think that the assignment to a raw pointer in ImportLoader::Updater::UpdateMainReferrer is a thinko, and we should fix that instead.
Attachment #8642413 - Flags: review?(nfroyd)
Comment on attachment 8642414 [details] [diff] [review] Part 3 -- Use .get() to assign nsRefPtrs to raw pointers Review of attachment 8642414 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/plugins/ipc/PluginScriptableObjectChild.cpp @@ +40,5 @@ > > /* static */ PluginScriptableObjectChild::StoredIdentifier* > PluginScriptableObjectChild::HashIdentifier(const nsCString& aIdentifier) > { > + StoredIdentifier* stored = sIdentifiers.Get(aIdentifier).get(); Yuck.
Attachment #8642414 - Flags: review?(nfroyd) → review+
Attachment #8642413 - Flags: review?(ehsan)
Comment on attachment 8642418 [details] [diff] [review] Part 4, option 2 -- Introduce RefParam to allow passing nsRefPtr without implicit raw pointer conversion Review of attachment 8642418 [details] [diff] [review]: ----------------------------------------------------------------- I'm not necessarily opposed to doing this, but I have some questions. Also, since we know this would break a tier3 platform (any Sparc platform, it sounds like), we should try to yell about that at compile time, either in the sparc-specific files in xpcom, or even in configure. ::: xpcom/base/RefParam.h @@ +6,5 @@ > + > +#ifndef mozilla_RefParam_h > +#define mozilla_RefParam_h > + > +#include "mozilla/dom/OwningNonNull.h" I realize we already have some mozilla/dom/ #includes throughout xpcom/, but this is the first one exposed via a header. If OwningNonNull is really that useful, then let's lift it out of dom/ and put it in xpcom/ or similar. @@ +77,5 @@ > + T& operator*() const { return *mRawPtr; } > + T* get() const { return mRawPtr; } > + > + operator nsRefPtr<T>() const { return mRawPtr; } > + operator nsCOMPtr<T>() const { return mRawPtr; } What are these here for? @@ +86,5 @@ > + * RawParam behaves the same as RefParam, except it accepts conversions from raw > + * pointers as well. This is useful because the caller may hold a > + * reference-counting pointer that does not implicitly convert to a raw pointer, > + * so using RawParam as the parameter type instead of a raw pointer saves the > + * caller from invoking .get() manually. Obviously, this must only be used if This description reads oddly to me. We have some method: f(..., RawParam<T>); and we have some caller: // CustomRefPtr does not implicitly convert to T*. CustomRefPtr<T> ptr = ...; f(..., ptr); This doesn't even compile, right? Since we can't get from CustomRefPtr<T> to T* without .get()? But that seems to be exactly the scenario the comment is describing, and how RawParam is superior in this situation... I'm not sure this outparam annotation will really add anything, because you're not guaranteed the caller is keeping things alive for you...what am I missing? @@ +107,5 @@ > +}; > + > +} // namespace mozilla > + > +// Declared in nsRefPtr.h Ew. I suppose if we try to declare these in the appropriate places, we run into some sort of circular dependency problem, because RefParam needs to know about the definition of nsRefPtr (etc.) and nsRefPtr needs to know about the definition of RefParam?
Attachment #8642418 - Flags: review?(nfroyd)
Comment on attachment 8642417 [details] [diff] [review] Part 4, option 1 -- Don't pass nsRefPtr&& to functions that want raw pointers Review of attachment 8642417 [details] [diff] [review]: ----------------------------------------------------------------- This seems reasonable enough. ::: dom/media/MozPromise.h @@ +815,5 @@ > ~MozPromiseRequestHolder() { MOZ_ASSERT(!mRequest); } > > + void Begin(nsRefPtr<typename PromiseType::Request>&& aRequest) > + { > + Begin(aRequest.get()); Shouldn't this be just: mRequest = Move(aRequest); with appropriate asserts? Or perhaps the Begin(Request*) method should be rewritten in terms of this one, to avoid too much duplication... ::: dom/media/webm/WebMDemuxer.cpp @@ +881,5 @@ > skipSamplesQueue.PushFront(sample); > } > while(skipSamplesQueue.GetSize()) { > + nsRefPtr<MediaRawData> data = skipSamplesQueue.PopFront(); > + mSamples.PushFront(data); I wonder if these PushFront methods should take && arguments as well or instead of pointer arguments, just to avoid unnecessary reference-counting. File a followup bug?
Attachment #8642417 - Flags: review?(nfroyd) → review+
Comment on attachment 8642420 [details] [diff] [review] Part 5 -- Delete nsRefPtr<T>::operator T*()&& Review of attachment 8642420 [details] [diff] [review]: ----------------------------------------------------------------- Presumably we want the same treatment for RefPtr.h and nsCOMPtr.h as well. Needs to be some compiler feature-checking for ref qualifiers and methods deleted appropriately. ::: mfbt/nsRefPtr.h @@ +229,5 @@ > { > return const_cast<T*>(mRawPtr); > } > > + operator T*() const& This is new territory as far as formatting is concerned, but I think a space between the const and |&| is appropriate. Likewise for the &&-qualifiers below. @@ +249,5 @@ > + operator T*() const&& = delete; > + > + // XXX Why are these needed to disambiguate? > + operator T*() & { return get(); } > + operator T*() && = delete; I am curious about this too, since it wasn't required for the nsCOMPtr patch I posted earlier... @@ +255,5 @@ > + // These are needed to avoid the deleted operator above. XXX Why is operator! > + // needed separately? Shouldn't the compiler prefer using the non-deleted > + // operator bool instead of the deleted operator T*? > + explicit operator bool() const { return !!mRawPtr; } > + bool operator!() const { return !mRawPtr; } Guess this is some sort of conversion preference thing; would have to trace through the spec to be sure...
Attachment #8642420 - Flags: review?(nfroyd) → feedback+
Depends on: 1190472
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #31) > I'm not necessarily opposed to doing this, but I have some questions. How would we go about deciding whether to go ahead with this? > Also, since we know this would break a tier3 platform (any Sparc platform, > it sounds like), we should try to yell about that at compile time, either in > the sparc-specific files in xpcom, or even in configure. This is only relevant if we convert XPCOM stuff, which this bug doesn't, so I won't worry about it for now. > I realize we already have some mozilla/dom/ #includes throughout xpcom/, but > this is the first one exposed via a header. If OwningNonNull is really that > useful, then let's lift it out of dom/ and put it in xpcom/ or similar. Yes, I think we should. > @@ +77,5 @@ > > + T& operator*() const { return *mRawPtr; } > > + T* get() const { return mRawPtr; } > > + > > + operator nsRefPtr<T>() const { return mRawPtr; } > > + operator nsCOMPtr<T>() const { return mRawPtr; } > > What are these here for? Apparently . . . nothing. I added them in some iteration to get the code to compile, but deleting them now works fine. > This description reads oddly to me. We have some method: > > f(..., RawParam<T>); > > and we have some caller: > > // CustomRefPtr does not implicitly convert to T*. > CustomRefPtr<T> ptr = ...; > f(..., ptr); > > This doesn't even compile, right? Since we can't get from CustomRefPtr<T> > to T* without .get()? But that seems to be exactly the scenario the comment > is describing, and how RawParam is superior in this situation... > > I'm not sure this outparam annotation will really add anything, because > you're not guaranteed the caller is keeping things alive for you...what am I > missing? After the last patch, nsRefPtr<T>&& no longer converts to T*. So if you have // void Function(T*); nsRefPtr<T> t = GetT(); Function(IsFoo() ? t : nullptr); it doesn't compile. Changing Function to take RefParam<T> instead of T* will work, but then you can't pass a T*. Changing it to RawParam<T> allows it to take both nsRefPtr<T>&& and T*. > Ew. I suppose if we try to declare these in the appropriate places, we run > into some sort of circular dependency problem, because RefParam needs to > know about the definition of nsRefPtr (etc.) and nsRefPtr needs to know > about the definition of RefParam? Well, RefParam.h includes nsRefPtr.h, so it would be a bit of a problem for nsRefPtr.h to include RefParam.h, no? (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #32) > I wonder if these PushFront methods should take && arguments as well or > instead of pointer arguments, just to avoid unnecessary reference-counting. > File a followup bug? Bug 1190472. (In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #33) > Presumably we want the same treatment for RefPtr.h and nsCOMPtr.h as well. Yep -- one thing at a time. > Needs to be some compiler feature-checking for ref qualifiers and methods > deleted appropriately. I'm working on patches now that pass try. > > + // XXX Why are these needed to disambiguate? > > + operator T*() & { return get(); } > > + operator T*() && = delete; > > I am curious about this too, since it wasn't required for the nsCOMPtr patch > I posted earlier... Now it compiles without. Mysterious. > > + // These are needed to avoid the deleted operator above. XXX Why is operator! > > + // needed separately? Shouldn't the compiler prefer using the non-deleted > > + // operator bool instead of the deleted operator T*? > > + explicit operator bool() const { return !!mRawPtr; } > > + bool operator!() const { return !mRawPtr; } > > Guess this is some sort of conversion preference thing; would have to trace > through the spec to be sure... Likewise, if (Foo()) now fails if Foo() returns an nsRefPtr. I need to change it to if (!!Foo()). I don't know why, but it's a bit annoying.
Attachment #8642418 - Attachment is obsolete: true
Attachment #8643090 - Flags: review?(nfroyd)
Comment on attachment 8643093 [details] [diff] [review] Part 5 -- Delete nsRefPtr<T>::operator T*()&& Review of attachment 8643093 [details] [diff] [review]: ----------------------------------------------------------------- This is better, thank you. ::: mfbt/nsRefPtr.h @@ +11,5 @@ > +# define MOZ_HAVE_REF_QUALIFIERS > +#elif defined(__clang__) > +// All supported Clang versions > +# define MOZ_HAVE_REF_QUALIFIERS > +#elif !defined(__clang__) && defined(__GNUC__) You can just say defined(__GNUC__) here, since !__clang__ is a given: you checked it above. @@ +281,5 @@ > + operator T*() const && = delete; > + > + // XXX Why are these needed to disambiguate? > + operator T*() & { return get(); } > + operator T*() && = delete; You said things compiled without these in comment 34? Is that "things compiled locally but failed on try" or "I forgot to remove these"? :) ::: widget/android/nsWindow.cpp @@ +2050,4 @@ > ALOGIME("IME: REQUEST_TO_CANCEL_COMPOSITION"); > > // Cancel composition on Gecko side > + if (!!GetIMEComposition()) { :(
Attachment #8643093 - Flags: review?(nfroyd) → review+
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #37) > You said things compiled without these in comment 34? Is that "things > compiled locally but failed on try" or "I forgot to remove these"? :) The latter. :) New green try with this fixed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=27428da5303f
Attachment #8642413 - Flags: review?(ehsan) → review+
Comment on attachment 8643090 [details] [diff] [review] Part 4, option 2, v2 -- Introduce RefParam Review of attachment 8643090 [details] [diff] [review]: ----------------------------------------------------------------- I'm going to say no on this one. The alternative doesn't require nearly as many modifications, and is generally modifying ternary operators anyway, where it's desirable to use .get() to avoid unnecessary refcounting anyway. You are correct that nsRefPtr<T>&& is perfectly fine to pass to T*. Please write a dev-platform post when these patches go in, and state this limitation. Please advise people that if they find this limitation burdensome when writing new code that returns nsRefPtr<T>, that they should yell about it (to me, to you, file a bug, whatever), and we can see what to do about it then.
Attachment #8643090 - Flags: review?(nfroyd)
So really the reason I think we should go with the RefParam solution is that the current convention is that the caller passes a raw pointer, and the callee sort of hopes that someone out there is holding a strong reference. If the caller is not actually holding a strong reference, the callee might do something to release the pointer, and you'll get a use-after-free, which can be sec-critical (and I know of at least one case where it happened). RefParam does a lot to ensure that the caller actually holds a strong reference, so it's that much safer. And it does so without any overhead, generally speaking. It's also worth pointing out that this patch solves one of the major issue some people had (at least Ehsan) with functions returning nsRefPtr etc. instead of already_AddRefed. After this bug is fixed, it will be safe to return nsRefPtr, so it might make sense to deprecate already_AddRefed as a return type. If we do go that route, we'll suddenly have tons of nsRefPtr&& that people will want to pass to functions, and if they don't implicitly convert to raw pointers, people will just slap .get() everywhere, which IMO is not a good solution because it desensitizes programmers to an unsafe operation. RefParam solves this problem too. I'd also be interested in Ehsan's opinion on the approach of options 1 vs. 2 in patch 4 of this bug.
Flags: needinfo?(nfroyd)
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor (working until August 13) from comment #40) > So really the reason I think we should go with the RefParam solution is that > the current convention is that the caller passes a raw pointer, and the > callee sort of hopes that someone out there is holding a strong reference. > If the caller is not actually holding a strong reference, the callee might > do something to release the pointer, and you'll get a use-after-free, which > can be sec-critical (and I know of at least one case where it happened). > RefParam does a lot to ensure that the caller actually holds a strong > reference, so it's that much safer. And it does so without any overhead, > generally speaking. So does |const nsRefPtr<T>&| or equivalent. I'd have to think more carefully about the calling conventions for this...RefParam might be as efficient as passing T* on 64-bit and ARM, but I think it might be worse on 32-bit (and the same as |const nsRefPtr<T>&|). > It's also worth pointing out that this patch solves one of the major issue > some people had (at least Ehsan) with functions returning nsRefPtr etc. > instead of already_AddRefed. After this bug is fixed, it will be safe to > return nsRefPtr, so it might make sense to deprecate already_AddRefed as a > return type. If we do go that route, we'll suddenly have tons of nsRefPtr&& > that people will want to pass to functions, and if they don't implicitly > convert to raw pointers, people will just slap .get() everywhere, which IMO > is not a good solution because it desensitizes programmers to an unsafe > operation. RefParam solves this problem too. So does |const RefPtr<T>&| or equivalent. I realize that doesn't provide for the interchange between smart pointer types that RefParam does. But given: - nsRefPtr/RefPtr are getting merged, so that's one less interchange problem to deal with; - If you have to use nsCOMPtr<T> for some T, you probably shouldn't be using nsRefPtr<T> anyway, which eliminates another set of mismatches; Now, what to do with OwningNonNull vs. RefPtr mismatches? I'm optimistic that this can be handled with: - Returning OwningNonNull, taking const RefPtr& - Returning RefPtr, taking const RefPtr& - Returning OwningNonNull, taking const OwningNonNull& (this requires a move constructor for OwningNonNull, which I *think* is OK, but would have to consider it a bit more) So returning RefPtr&, taking const OwningNonNull& is the only problematic case, and that would require a bit more work on the caller's side, which strikes me as OK. We also have nsCOMPtr vs. OwningNonNull& mismatches, which are a bit harder to work out. I am less sure what to do there. I think all of the above cases for RefPtr come out about the same for nsCOMPtr, though. The long and short of it is that I'm attempting to be conservative here. We're trying to close a loophole that people have pointed out as a concern for eliminating already_AddRefed. Great! Let's close that loophole first, without building a bunch of stuff we think might be necessary on top of that. Then as people gain experience with the new style of programming, we can start to talk about what would solve problems for them. And I think in this case, the tools available in "raw" C++ are workable without the safety concerns that have been raised in the outparam bug...happy to be proven wrong on that, though.
Flags: needinfo?(nfroyd)
(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #41) > So does |const nsRefPtr<T>&| or equivalent. That was my first thought too, but AFAIK, that's not binary-compatible with XPCOM, whereas it seems RefParam is. See discussion on bug 767178 comment 29 ff., particularly 35. > I'd have to think more > carefully about the calling conventions for this...RefParam might be as > efficient as passing T* on 64-bit and ARM, but I think it might be worse on > 32-bit (and the same as |const nsRefPtr<T>&|). The previously-referenced comment says it's ABI-compatible with T* (at least on tier-1 and tier-2 platforms), which implies it's as efficient. Presumably const nsRefPtr<T>& is less efficient. > Now, what to do with OwningNonNull vs. RefPtr mismatches? I'm optimistic > that this can be handled with: > > - Returning OwningNonNull, taking const RefPtr& > - Returning RefPtr, taking const RefPtr& > - Returning OwningNonNull, taking const OwningNonNull& (this requires a move > constructor for OwningNonNull, which I *think* is OK, but would have to > consider it a bit more) > > So returning RefPtr&, taking const OwningNonNull& is the only problematic > case, and that would require a bit more work on the caller's side, which > strikes me as OK. As long as nsRefPtr can be implicitly constructed from OwningNonNull, I think all this will compile. However, it will addref unnecessarily. > The long and short of it is that I'm attempting to be conservative here. > We're trying to close a loophole that people have pointed out as a concern > for eliminating already_AddRefed. Great! Let's close that loophole first, > without building a bunch of stuff we think might be necessary on top of > that. Then as people gain experience with the new style of programming, we > can start to talk about what would solve problems for them. And I think in > this case, the tools available in "raw" C++ are workable without the safety > concerns that have been raised in the outparam bug...happy to be proven > wrong on that, though. At this point I think the advantages of RefParam<T> over const nsRefPtr<T>& are: 1) Can be used for XPCOM interfaces without breaking binary compatibility. (SPARC can just redefine it to T* if they want and it will compile fine, so I'm not worried about them.) 2) More efficient -- no extra dereferencing. 3) No extra addref/release when converting from OwningNonNull. None of these are killer advantages, but they seem worthwhile. So I'm indifferent, up to you. If you don't want RefParam, I guess I'll start using const nsRefPtr<T>&.
Flags: needinfo?(nfroyd)
I haven't studied this bug enough to have a fully informed opinion, but binary compat with XPCOM seems like a *huge* benefit to RefParam, and all other things being equal, I think that is an outstanding vote in its favor. Also note that fixing this bug doesn't necessarily mean that we can remove already_AddRefed. Returning an already_AddRefed still has a semantic difference from returning a smart pointer, in that it documents an ownership transfer, and forces the caller to treat it as such. I've been thinking about this for a *long time* and go back and forth between deciding that is a huge benefit and that we can ignore that benefit and remove already_AddRefed once this is fixed, and I don't think I can tell what the right thing to do is, but I think we should not assume that already_AddRefed will be removed, at least for now.
Flags: needinfo?(ehsan)
(In reply to :Aryeh Gregor (working until August 13) from comment #42) > > I'd have to think more > > carefully about the calling conventions for this...RefParam might be as > > efficient as passing T* on 64-bit and ARM, but I think it might be worse on > > 32-bit (and the same as |const nsRefPtr<T>&|). > > The previously-referenced comment says it's ABI-compatible with T* (at least > on tier-1 and tier-2 platforms), which implies it's as efficient. > Presumably const nsRefPtr<T>& is less efficient. So it is. I was unaware of how this works out everywhere. So that's a point in RefParam's favor. > At this point I think the advantages of RefParam<T> over const nsRefPtr<T>& > are: > > 1) Can be used for XPCOM interfaces without breaking binary compatibility. > (SPARC can just redefine it to T* if they want and it will compile fine, so > I'm not worried about them.) > > 2) More efficient -- no extra dereferencing. > > 3) No extra addref/release when converting from OwningNonNull. > > None of these are killer advantages, but they seem worthwhile. Thanks for the concise summary, and for persisting through my inability to understand. My comment on the documentation for RawParam in comment 31 still stands; I think the response you gave in comment 34 explains the situation, but I don't think it matches with the text that you wrote. I am still a little skeptical of the need for RawParam, but maybe as we give up on our fixation with raw pointers, the need for RawParam will go away? We also need some |= default| for various things in RefParam/RawParam per bug 767178, I think. Some |= delete|s for move constructors might be a bad thing, either? I'd almost say this is worth landing in a separate bug, but I guess there's no point changing all those callsites to do .get() and then changing them back, is there?
Flags: needinfo?(nfroyd)
I agree this should have been a separate bug, and I could still land it in a separate bug (to land *before* this one) if you want, but at this point it seems a bit pointless to file the new bug. I updated the comment -- if you think it could use further work, let me know what you want it to say. I didn't delete the move constructor, because it didn't compile if I did that (I don't know why). I did delete the assignment operator, though.
Attachment #8643090 - Attachment is obsolete: true
Attachment #8646299 - Flags: review?(nfroyd)
Summary: disallow implicit conversion to T* from nsCOMPtr<T>&& values → disallow implicit conversion to T* from nsRefPtr<T>&& values
Depends on: 1193298
Blocks: 1193298
No longer depends on: 1193298
By the way, I really don't think option 1 for patch 4 is tenable for converting nsCOMPtr, particularly in editor. Deleting nsCOMPtr<T>::operator T*()&& almost certainly requires RefParam.
Comment on attachment 8646299 [details] [diff] [review] Part 4, option 2, v3 -- Introduce RefParam Review of attachment 8646299 [details] [diff] [review]: ----------------------------------------------------------------- Sorry, I am doing a horrible job of reviewing these patches. :( TL; DR: RawParam is a more pragmatic choice than RefParam, but isn't really buying us that much. Looking at why the RawParam changes were necessary just from the dom/media/ changes, it looks like we're using RawParam because we would like to enforce the invariant that members are never passed to RefParam parameters. I see two sets of problems with this: - Absent static analysis, people will see RefParam as the safe choice, and use that. Except that might only be providing the illusion of safety, because they didn't carefully analyze all the callsites. Maybe somebody adds a new callsite that passes a member, and that violates the RefParam contract. Just one rogue instance means you have to use RawParam, or you're not using the class correctly. So we can't actually provide the safety guarantees in this case without careful review. - With static analysis, it seems likely that people will just use RawParam, so they don't have to think about how the function is being used. Alternatively, one might be using RefParam, but a new callsite passes a member for the parameter...what now? Change the function signature? That seems potentially dangerous, since we have to check all the other callsites for non-trivial invariants. Hold a local strong reference? Maybe, seems like unnecessary work and/or verbiage, but that's just the price we pay, I guess. It's certainly possible that some owners will carefully enforce RefParam usage, but whether that can be maintained over time or whether this dichotomy means that we have even more module-specific dialects ("we don't use RefParam in this module") is a worthwhile open question. This patch has to use RawParam several times as much as RefParam, and that's without me carefully verifying that all the RefParam functions aren't getting members (or members-of-members, etc.) passed in to them. That the trivial "make-it-work" patch (and similar patches for nsCOMPtr) can't use the safe choice very often suggests that the safe choice isn't going to get used that much. So the safe choice can't be the default, and the alternative choice, RawParam, doesn't seem to provide obvious safety guarantees over T*. Maybe we could require that all RawParam-taking functions have to hold strong references and call RefParam-taking functions...except that's extra work in a lot of cases and the overloads probably don't work right, since things implicitly convert to both kinds of parameter types. I guess we get to eliminate potentially verbose temporaries, which seems sort of meh to me. Maybe we get to eliminate implicit |operator T*|...with a *lot* of work, and this funky value class scattered all over. I don't feel good about r+'ing this patch as part of this bug. It's entirely possible my verbiage above is mistaken. Can we do option 1 here and consider refining these changes (or abandoning them altogether) in a separate dependent bug? ::: xpcom/base/RefParam.h @@ +90,5 @@ > + > + explicit operator bool() const { return mRawPtr; } > + > + // No assignment allowed > + RefParam<T>& operator=(const RefParam<T>&) = delete; I realize that you think this is needless verbiage, but I think it's worth |= default|'ing the move constructor/assignment members, per https://en.wikipedia.org/wiki/Rule_of_three_%28C%2B%2B_programming%29 , just to be clear. @@ +99,5 @@ > + * pointers as well. This is the same as a raw pointer, except that smart > + * pointers will implicitly convert to it, even in cases where they don't > + * implicitly convert to a raw pointer. For instance, nsRefPtr&& doesn't > + * implicitly convert to a raw pointer, but does implicitly convert to RawParam, > + * so this code will work if Function takes RawParam<T> but not if it takes T*: This is a much better explanation, thank you for clarifying it! @@ +169,5 @@ > + > + operator T*() const { return RefParam<T>::mRawPtr; } > + > + // No assignment allowed > + RawParam<T>& operator=(const RawParam<T>&) = delete; Likewise for |= default|'ing things here.
Attachment #8646299 - Flags: review?(nfroyd)
Depends on: 1194195
Depends on: 1194215
(In reply to :Aryeh Gregor (working until August 13) from comment #49) > Green Mac builds with adjusted patch: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=071d97aede2b Inbound is closed right now and I won't be around, so I need someone to check this in for me. ***DO NOT CHECK IN THE ATTACHED PATCHES, THEY WILL BREAK THE BUILD, CHECK IN THE PATCHES FROM THE TRY RUN QUOTED ABOVE.***
Keywords: checkin-needed
(In reply to :Aryeh Gregor (working until August 13) from comment #50) > ***DO NOT CHECK IN THE ATTACHED PATCHES, THEY WILL BREAK THE BUILD, CHECK IN > THE PATCHES FROM THE TRY RUN QUOTED ABOVE.*** Just attach the updated patches next time?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: