[webkit-reviews] review granted: [Bug 190113] ASAN failure in ~GCReachableRef() : [Attachment 351200] Fixes the bug

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 190113: ASAN failure in ~GCReachableRef()
https://bugs.webkit.org/show_bug.cgi?id=190113

Attachment 351200: Fixes the bug

https://bugs.webkit.org/attachment.cgi?id=351200&action=review




--- Comment #12 from Darin Adler <darin at apple.com> ---
Comment on attachment 351200
  --> https://bugs.webkit.org/attachment.cgi?id=351200
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=351200&action=review

> Source/WebCore/dom/GCReachableRef.h:66
> +	   : m_ptr(WTFMove(other.m_ptr))

This line of code is wrong and uses of this function template won’t compile.
Since "other" is a Ref, it does not have an "m_ptr" member to move from. The
reason we don’t see a compiler error is that there are no uses of this function
template. Maybe we should delete it for now? Or we could add unit tests for it
if we want to keep it.

> Source/WebCore/dom/GCReachableRef.h:75
>      GCReachableRef(GCReachableRef&& other)
> -	   : m_ref(WTFMove(other.m_ref))
> +	   : m_ptr(WTFMove(other.m_ptr))
>      {
>      }

This is the same as the default implementation; we could get the same effect
with "= default" here or maybe by omitting this entirely and letting it get
generated.

On reflection, though, I realize that one thing this lacks is an assertion that
the "other" is not already null. Perhaps we should leave this explicitly
written out, but also explicitly forbid moving out of the same object twice
just as the move constructor in Ref does, by asserting the pointer is non-null
in the function body.

> Source/WebCore/dom/GCReachableRef.h:77
>      template<typename X, typename Y> GCReachableRef(const GCReachableRef<X,
Y>& other) = delete;

It doesn't makes sense that we would have to delete this function template.
Yes, we need to delete the automatically generated copy constructor, since the
code it generates would be incorrect, but this copy-constructor-like function
template should *not* be automatically generated so it should not need to be
deleted.

The same logic applies in the Ref class. I have no idea why the
copy-constructor-like and assignment-operator-like template functions need to
be deleted in that class. I believe those lines of code in Ref.h have no effect
and are unnecessary.

When GCReachableRef was using a Ref for a member, though, we did *not* need to
delete its copy constructor, the Ref member already prevented it from being
generated. But now that we are using RefPtr instead, we need to prevent the
automatic generation of the copy constructor and copy assignment operator
either directly with delete or indirectly by defining the move constructor and
move assignment operator.

So this line should go. And we have additional work to do to make sure we don’t
automatically generate an incorrect copy constructor and assignment operator.

> Source/WebCore/dom/GCReachableRef.h:80
> +    T* operator->() const { ASSERT(m_ptr); return m_ptr.get(); }
> +    T* ptr() const RETURNS_NONNULL { ASSERT(m_ptr); return m_ptr.get(); }

Would be nice to not have to write out the assertion. Maybe just call &get() in
these functions?

> Source/WebCore/dom/GCReachableRef.h:83
> +    T& get() const { ASSERT(m_ptr); return *m_ptr; }
> +    operator T&() const { ASSERT(m_ptr); return *m_ptr; }
> +    bool operator!() const { ASSERT(m_ptr); return !*m_ptr; }

RefPtr::operator* already asserts, so there is no need for additional assertion
here