1119080 - already_AddRefed in AlreadyAddRefed.h is not compatible with VS2015: error C2280: attempting to reference a deleted function

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Build log

Comment 1

[Nathan Froyd [:froydnj]]

This is sort of a bizarre error.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Patch that fixes some of the instances of the error by doing explicit nsCOMPtr<T>/nsRefPtr<T> conversions

The MSVC 2015 behavior may not be as crazy as it seems at first.

It seems like all the instances where the compiler complains are due to this scenerio:


already_AddRefed<Base>
foo()
{
   nsCOMPtr<Derived> subclass = ...;
   return subclass.forget();
}

nsCOMPtr<Derived>::forget() will return already_AddRefed<Derived>. Since we need to return already_AddRefed<Base>, we need to do a conversion. This conversion seems to confuse MSVC 2015's copy elision logic, so it doesn't do copy elision (??? not sure ???). Then, it feels like it needs to use already_Addrefed<T> copy constructor, which has been deleted.

As far as I understand the specification, the current definition of already_Addrefed<T> is not guaranteed to work according to the spec, because copy elision is always optional, and if the compiler doesn't do copy elision, then it needs a copy constructor.

By adding an explicit nsRefPtr<Base>/nsCOMPtr<Base> conversion, I can work around the error. It isn't clear how many instances of this pattern there are in the code base, so before I continue, I'd like to hear what you think. In particular, is there a better workaround I should attempt?

Comment 3

[(no longer active)]

How come <https://dxr.mozilla.org/mozilla-central/source/mfbt/AlreadyAddRefed.h?from=already_AddRefed&case=true#94> doesn't work with MSVC2015?

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Convert the already_Addrefed Derived->Base conversion operator into a move constructor

The problem may be caused by the fact that the conversion operator is non-const.

non-const conversion operators violate the principle of least surprise, because we don't expect static_cast<Base>(x) to modify x. In particular, consider the following code:

   already_Addrefed<Derived> foo = GetFoo();
   already_Addrefed<Base> base(foo);
   foo->doSomething(); // NULL POINTER DEREFERENCE!!!

So it's a good idea to avoid such non-const conversion operators anyway, regardless of the VS2015 issue.

Here is my attempt to do so. Note that the above-cited bad code sample won't compile, because of the missing std::move:

   already_Addrefed<Derived> foo = GetFoo();
   already_Addrefed<Base> base(std::move(foo));
   // NULL POINTER DEREFERENCE, but you expect that after
   // std::move! (This is why std::move exists, more or less.)
   foo->doSomething();

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=86b1d33f4f68

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Thanks for the quick review!

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f889f63ea6c

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f889f63ea6c