[webkit-dev] Watch out for std::optional's move constructor

Mon Dec 17 16:22:23 PST 2018

It's true that WTFMove or std::move doesn't do anything if the moved
variable is not used because WTFMove / std::move is just a type cast.

However, that behavior is orthogonal from the issue that calling WTFMove /
std::move on std::optional, and the returned value is assigned to another
std::optional, the original std::optional will be left a bad state.

I completely disagree with your assessment that this calls for the use of
std::exchange.

On Mon, Dec 17, 2018 at 3:55 PM Alex Christensen <achristensen at apple.com>
wrote:

> Let me give a concrete example on why, even with our nice-to-use WTF
> types, the state of a C++ object is undefined after being moved from:
>
> #include <wtf/RefCounted.h>
> #include <wtf/RefPtr.h>
> #include <iostream>
>
> class Test : public RefCounted<Test> { };
>
> void useParameter(RefPtr<Test>&& param)
> {
>   RefPtr<Test> usedParam = WTFMove(param);
> }
>
> void dontUseParameter(RefPtr<Test>&&) { }
>
> int main() {
>   RefPtr<Test> a = adoptRef(new Test);
>   RefPtr<Test> b = adoptRef(new Test);
>   std::cout << "a null? " << !a << std::endl;
>   std::cout << "b null? " << !b << std::endl;
>   useParameter(WTFMove(a));
>   dontUseParameter(WTFMove(b));
>   std::cout << "a null? " << !a << std::endl;
>   std::cout << "b null? " << !b << std::endl;
>   return 0;
> }
>
> // clang++ test.cpp -I Source/WTF -L WebKitBuild/Debug -l WTF -framework
> Foundation -L /usr/lib -l icucore --std=c++17 && ./a.out
>
> // a null? 0
>
>
> // b null? 0
>
>
> // a null? 1
>
>
> // b null? 0
>
>
>
>
> As you can see, the internals of callee dontUseParameter (which could be
> in a different translation unit) affects the state of the local variable b
> in this function.  This is one of the reasons why the state of a moved-from
> variable is intentionally undefined, and we can’t fix that by using our own
> std::optional replacement.  If we care about the state of a moved-from
> object, that is what std::exchange is for.  I think we should do something
> to track and prevent the use of moved-from values instead of introducing
> our own std::optional replacement.
>
> On Dec 17, 2018, at 2:47 PM, Ryosuke Niwa <rniwa at webkit.org> wrote:
>
> Yeah, it seems like making std::optional more in line with our own
> convention provides more merits than downsides here. People are using
> WTFMove as if it's some sort of a swap operation in our codebase, and as
> Maciej pointed out, having rules where people have to think carefully as to
> when & when not to use WTFMove seems more troublesome than the proposed
> fix, which would mean this work for optional.
>
> - R. Niwa
>
> On Mon, Dec 17, 2018 at 2:24 PM Geoffrey Garen <ggaren at apple.com> wrote:
>
>> I don’t understand the claim about “undefined behavior” here. As Maciej
>> pointed out, these are our libraries. We are free to define their behaviors.
>>
>> In general, “undefined behavior” is an unwanted feature of programming
>> languages and libraries, which we accept begrudgingly simply because there
>> are practical limits to what we can define. This acceptance is not a
>> mandate to carry forward undefined-ness as a badge of honor. In any case
>> where it would be practical to define a behavior, that defined behavior
>> would be preferable to undefined behavior.
>>
>> I agree that the behavior of move constructors in the standard library is
>> undefined. The proposal here, as I understand it, is to (a) define the
>> behaviors move constructors in WebKit and (b) avoid std::optional and use
>> an optional class with well-defined behavior instead.
>>
>> Because I do not ❤️ security updates, I do ❤️ defined behavior, and so I
>> ❤️ this proposal.
>>
>> Geoff
>>
>> On Dec 17, 2018, at 12:50 PM, Alex Christensen <achristensen at apple.com>
>> wrote:
>>
>> This one and the many others like it are fragile, relying on undefined
>> behavior, and should be replaced by std::exchange.  Such a change was made
>> in https://trac.webkit.org/changeset/198755/webkit and we probably need
>> many more like that, but we are getting away with relying on undefined
>> behavior which works for us in most places.
>>
>> On Dec 17, 2018, at 11:24 AM, Chris Dumez <cdumez at apple.com> wrote:
>>
>>
>>
>> On Dec 17, 2018, at 11:10 AM, Chris Dumez <cdumez at apple.com> wrote:
>>
>>
>>
>> On Dec 17, 2018, at 10:27 AM, Alex Christensen <achristensen at apple.com>
>> wrote:
>>
>> On Dec 14, 2018, at 1:37 PM, Chris Dumez <cdumez at apple.com> wrote:
>>
>>
>> As far as I know, our convention in WebKit so far for our types has been
>> that types getting moved-out are left in a valid “empty” state.
>>
>> This is not necessarily true.  When we move out of an object to pass into
>> a function parameter, for example, the state of the moved-from object
>> depends on the behavior of the callee.  If the callee function uses the
>> object, we often have behavior that leaves the object in an “empty” state
>> of some kind, but we are definitely relying on fragile undefined behavior
>> when we do so because changing the callee to not use the parameter changes
>> the state of the caller.  We should never assume that WTFMove or std::move
>> leaves the object in an empty state.  That is always a bug that needs to be
>> replaced by std::exchange.
>>
>>
>> Feel like we’re taking about different things. I am talking about move
>> constructors (and assignment operators), which have a well defined behavior
>> in WebKit. And it seems you are talking about WTFMove(), which despite the
>> name does not “move” anything, it is merely a cast.
>> In the case you’re talking about the caller does NOT call the move
>> constructor, it merely does a cast so I do not think your comment
>> invalidates my statement. Note that in my patch, I was nearly WTFMove()ing
>> the data member and assigning it to a local variable right away, calling
>> the move constructor.
>>
>>
>> Also note that may of us already rely on our move constructors’ behavior,
>> just search for WTFMove(m_responseCompletionHandler) in:
>> https://trac.webkit.org/changeset/236463/webkit
>>
>>
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>>
>> _______________________________________________
>> webkit-dev mailing list
>> webkit-dev at lists.webkit.org
>> https://lists.webkit.org/mailman/listinfo/webkit-dev
>>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-dev/attachments/20181217/de22f7ab/attachment-0001.html>