Bug 214809 – DisallowVMEntry needs a copy assignment operator, detected by gcc's -Wdeprecated-copy warning

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Catanzaro]

Introduced by r264736:

[1651/4306] Building CXX object Source/WebCore/CMakeFiles...es/WebCore/unified-sources/UnifiedSource-68aea4ac-6.cpp.o
In file included from DerivedSources/ForwardingHeaders/wtf/Seconds.h:29,
                 from DerivedSources/ForwardingHeaders/wtf/WallTime.h:29,
                 from DerivedSources/ForwardingHeaders/wtf/ThreadingPrimitives.h:37,
                 from DerivedSources/ForwardingHeaders/wtf/MainThread.h:35,
                 from DerivedSources/ForwardingHeaders/wtf/NeverDestroyed.h:31,
                 from DerivedSources/ForwardingHeaders/wtf/text/AtomString.h:24,
                 from DerivedSources/ForwardingHeaders/wtf/text/WTFString.h:690,
                 from ../../Source/WebCore/dom/Exception.h:30,
                 from ../../Source/WebCore/dom/ExceptionOr.h:29,
                 from ../../Source/WebCore/bindings/js/JSDOMBinding.h:28,
                 from ../../Source/WebCore/bindings/js/JSDOMMapLike.h:28,
                 from ../../Source/WebCore/bindings/js/JSDOMMapLike.cpp:27,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource-68aea4ac-6.cpp:1:
DerivedSources/ForwardingHeaders/wtf/Optional.h: In instantiation of ‘WTF::Optional< <template-parameter-1-1> >& WTF::Optional< <template-parameter-1-1> >::operator=(const WTF::Optional< <template-parameter-1-1> >&) [with T = JSC::DisallowVMEntryImpl<JSC::VM>]’:
DerivedSources/ForwardingHeaders/JavaScriptCore/PropertySlot.h:87:7:   required from here
DerivedSources/ForwardingHeaders/wtf/Optional.h:465:84: warning: implicitly-declared ‘constexpr JSC::DisallowVMEntryImpl<JSC::VM>& JSC::DisallowVMEntryImpl<JSC::VM>::operator=(const JSC::DisallowVMEntryImpl<JSC::VM>&)’ is deprecated [-Wdeprecated-copy]
  465 |     else if (initialized() == true  && rhs.initialized() == true)  contained_val() = *rhs;
      |                                                                    ~~~~~~~~~~~~~~~~^~~~~~
In file included from DerivedSources/ForwardingHeaders/JavaScriptCore/VM.h:38,
                 from DerivedSources/ForwardingHeaders/JavaScriptCore/Identifier.h:24,
                 from DerivedSources/WebCore/JSDOMBindingInternalsBuiltins.h:34,
                 from DerivedSources/WebCore/WebCoreJSBuiltinInternals.h:39,
                 from ../../Source/WebCore/bindings/js/JSDOMGlobalObject.h:29,
                 from ../../Source/WebCore/bindings/js/DOMWrapperWorld.h:24,
                 from ../../Source/WebCore/bindings/js/JSDOMWrapperCache.h:26,
                 from ../../Source/WebCore/bindings/js/JSDOMBinding.h:29,
                 from ../../Source/WebCore/bindings/js/JSDOMMapLike.h:28,
                 from ../../Source/WebCore/bindings/js/JSDOMMapLike.cpp:27,
                 from DerivedSources/WebCore/unified-sources/UnifiedSource-68aea4ac-6.cpp:1:
DerivedSources/ForwardingHeaders/JavaScriptCore/DisallowVMEntry.h:46:5: note: because ‘JSC::DisallowVMEntryImpl<JSC::VM>’ has user-provided ‘JSC::DisallowVMEntryImpl<VMType>::DisallowVMEntryImpl(const JSC::DisallowVMEntryImpl<VMType>&) [with VMType = JSC::VM]’
   46 |     DisallowVMEntryImpl(const DisallowVMEntryImpl& other)
      |     ^~~~~~~~~~~~~~~~~~~

Problem is the code makes a copy of the DisallowVMEntryImpl, and this copy fails to increment the VM's disallowVMEntryCount. But disallowVMEntryCount will be decremented when the DisallowVMEntryImpl is destroyed. ***I think that results in underflow.***

Let's add a copy constructor.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0)
> Let's add a copy constructor.

Maybe it should be noncopyable instead? Seems safer? But then we can't use Optional<DisallowVMEntry>.

[Mark Lam]

(In reply to Michael Catanzaro from comment #1)
> (In reply to Michael Catanzaro from comment #0)
> > Let's add a copy constructor.
> 
> Maybe it should be noncopyable instead? Seems safer? But then we can't use
> Optional<DisallowVMEntry>.

Optional will work just fine with non-copyable.  That’s not the issue.  The issue is that one place in WebCore wants to copy PropertySlots, which may mean copying DisallowVMEntry as well.

[Mark Lam]

(In reply to Michael Catanzaro from comment #0)
...
> DerivedSources/WebCore/unified-sources/UnifiedSource-68aea4ac-6.cpp:1:
> DerivedSources/ForwardingHeaders/JavaScriptCore/DisallowVMEntry.h:46:5:
> note: because ‘JSC::DisallowVMEntryImpl<JSC::VM>’ has user-provided
> ‘JSC::DisallowVMEntryImpl<VMType>::DisallowVMEntryImpl(const
> JSC::DisallowVMEntryImpl<VMType>&) [with VMType = JSC::VM]’
>    46 |     DisallowVMEntryImpl(const DisallowVMEntryImpl& other)
>       |     ^~~~~~~~~~~~~~~~~~~
> 
> Problem is the code makes a copy of the DisallowVMEntryImpl, and this copy
> fails to increment the VM's disallowVMEntryCount. But disallowVMEntryCount
> will be decremented when the DisallowVMEntryImpl is destroyed. ***I think
> that results in underflow.***
> 
> Let's add a copy constructor.

Looking at the original patch, my copy constructor does increment vm->disallowVMEntryCount.  What am I missing?

[Michael Catanzaro]

Created attachment 405257 [details]
Patch

[Michael Catanzaro]

(In reply to Mark Lam from comment #3)
> Looking at the original patch, my copy constructor does increment
> vm->disallowVMEntryCount.  What am I missing?

The copy assignment operator!

[Darin Adler]

Comment on attachment 405257 [details]
Patch

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

> Source/JavaScriptCore/runtime/DisallowVMEntry.h:52
> +    DisallowVMEntryImpl& operator=(const DisallowVMEntryImpl& other)

Could you instead do a "delete"?

    void operator=(const DisallowVMEntryImpl&) = delete;

I ask because I think we do not want to assign these and would prefer a compiler error. Not even sure why we want to copy them.

[Darin Adler]

Comment on attachment 405257 [details]
Patch

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

>> Source/JavaScriptCore/runtime/DisallowVMEntry.h:52
>> +    DisallowVMEntryImpl& operator=(const DisallowVMEntryImpl& other)
> 
> Could you instead do a "delete"?
> 
>     void operator=(const DisallowVMEntryImpl&) = delete;
> 
> I ask because I think we do not want to assign these and would prefer a compiler error. Not even sure why we want to copy them.

Oh, I see you already discussed this with Mark, and I guess we do want to assign these.

[Darin Adler]

Comment on attachment 405257 [details]
Patch

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

> Source/JavaScriptCore/runtime/DisallowVMEntry.h:55
> +        m_vm = other.m_vm;
> +        m_vm->disallowVMEntryCount++;

This is wrong. Needs to decrement the disallowVMEntryCount of the m_vm we are already pointing at. I suggest writing it in this more or less foolproof way instead:

    auto copy = other;
    std::swap(m_vm, copy.m_vm);

Or you could add lines to this function:

    RELEASE_ASSERT(m_vm->disallowVMEntryCount);
    m_vm->disallowVMEntryCount--;

[Mark Lam]

(In reply to Darin Adler from comment #8)
> Comment on attachment 405257 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=405257&action=review
> 
> > Source/JavaScriptCore/runtime/DisallowVMEntry.h:55
> > +        m_vm = other.m_vm;
> > +        m_vm->disallowVMEntryCount++;
> 
> This is wrong. Needs to decrement the disallowVMEntryCount of the m_vm we
> are already pointing at. I suggest writing it in this more or less foolproof
> way instead:
> 
>     auto copy = other;
>     std::swap(m_vm, copy.m_vm);
> 
> Or you could add lines to this function:
> 
>     RELEASE_ASSERT(m_vm->disallowVMEntryCount);
>     m_vm->disallowVMEntryCount--;

I’m not sure swapping is the correct solution.  I made this copyable in the first place because one place in WebCore was copying the PropertySlot that embeds it.  From my reading of the code then, copying and incrementing the ref count is the right thing to do.  That said, I can take another close look at this and see if we can solve this a different way without copying.  I agree that not copying is preferable.  I’ll do that later: away from my computer right now.

[Michael Catanzaro]

(In reply to Darin Adler from comment #8)
> This is wrong. Needs to decrement the disallowVMEntryCount of the m_vm we
> are already pointing at. I suggest writing it in this more or less foolproof
> way instead:
> 
>     auto copy = other;
>     std::swap(m_vm, copy.m_vm);

I agree with Mark... I don't think swapping is safe here.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #10)
> (In reply to Darin Adler from comment #8)
> > This is wrong. Needs to decrement the disallowVMEntryCount of the m_vm we
> > are already pointing at.

You're right about that, though: my implementation is not safe if m_vm is initially nonnull.

[Mark Lam]

The function that wants to do PropertySlot assignment / copying is JSDOMWindow::getOwnPropertySlot().  The reason it wants to do this is because it wants to do is because it wants to call its Base::getOwnPropertySlot().  If Base::getOwnPropertySlot() does not find the requested property, JSDOMWindow::getOwnPropertySlot() wants to try again with the original PropertySlot value (in case Base::getOwnPropertySlot() has modified it though it isn't able to find the property).

After looking at this code again, I think there are certainly ways around having an assignment operator for DisallowVMEntry.  However, the alternatives may be worse than the straightforward approach of just having an assignment operator.

When I wrote the code originally, I also made the same mistake of assuming that copy assignment is the same thing as simply invoking the copy constructor, which is probably why this is deprecated.  I'll make the patch to fix the issue.  Thanks for filing this bug.

[Mark Lam]

*** Bug 214815 has been marked as a duplicate of this bug. ***

[Mark Lam]

I already have a patch in hand.  Just want to write a few tests and make sure it is functioning as expected before posting.

[Mark Lam]

For the record, there is no reference count mismatch.  The only issue here is that C++ does not allow auto-generation of the copy assignment operator when a user declared destructor or copy constructor is present.  In DisallowVMEntry's case, it has both a user declared destructor and copy constructor.

[Mark Lam]

(In reply to Mark Lam from comment #15)
> For the record, there is no reference count mismatch.  The only issue here
> is that C++ does not allow auto-generation of the copy assignment operator
> when a user declared destructor or copy constructor is present.  In
> DisallowVMEntry's case, it has both a user declared destructor and copy
> constructor.

I take that back: I'm not sure how the compiler generates the default copy assignment operator.  If it calls the copy constructor, then there can be a reference count issue.  My tests shows that this issue does not manifest with Clang though, FWIW.

[Mark Lam]

Created attachment 405263 [details]
proposed patch.

[Michael Catanzaro]

(In reply to Mark Lam from comment #16)
> I take that back: I'm not sure how the compiler generates the default copy
> assignment operator.  If it calls the copy constructor, then there can be a
> reference count issue.  My tests shows that this issue does not manifest
> with Clang though, FWIW.

It doesn't call the copy constructor, it will just do a default copy of all data members. Combined with your RELEASE_ASSERT to ensure both DisallowVMEntry objects  already correspond to the same VM before assignment, that explains why there's not currently any reference count issue.

[Darin Adler]

(In reply to Michael Catanzaro from comment #10)
> (In reply to Darin Adler from comment #8)
> > This is wrong. Needs to decrement the disallowVMEntryCount of the m_vm we
> > are already pointing at. I suggest writing it in this more or less foolproof
> > way instead:
> > 
> >     auto copy = other;
> >     std::swap(m_vm, copy.m_vm);
> 
> I agree with Mark... I don't think swapping is safe here.

The new patch is a better idea.

But the assignment operator based on swap here is *definitely* safe and correct. We don’t need to go into it since we have a different, better proposal, but I am concerned about this misunderstanding.

[Michael Catanzaro]

(In reply to Darin Adler from comment #19)
> But the assignment operator based on swap here is *definitely* safe and
> correct. We don’t need to go into it since we have a different, better
> proposal, but I am concerned about this misunderstanding.

Hm... squinting at it:

    auto copy = other;
    std::swap(m_vm, copy.m_vm);

Isn't it actually recursive?

DisallowVMEntryImpl& operator=(const DisallowVMEntryImpl& other)
{
    auto copy = other; // <-- calls itself?
    std::swap(m_vm, copy.m_vm);
    return *this;
}

I haven't tested it, but I guess we'd never reach the swap as the first line would keep calling itself forever?

Doesn't matter, since Mark's solution is good. And it has so many tests!

[Mark Lam]

(In reply to Michael Catanzaro from comment #20)
> (In reply to Darin Adler from comment #19)
> > But the assignment operator based on swap here is *definitely* safe and
> > correct. We don’t need to go into it since we have a different, better
> > proposal, but I am concerned about this misunderstanding.
> 
> Hm... squinting at it:
> 
>     auto copy = other;
>     std::swap(m_vm, copy.m_vm);
> 
> Isn't it actually recursive?
> 
> DisallowVMEntryImpl& operator=(const DisallowVMEntryImpl& other)
> {
>     auto copy = other; // <-- calls itself?

I think Darin meant that we should use the copy constructor here:
      auto copy(other);

>     std::swap(m_vm, copy.m_vm);
>     return *this;
> }
> 
> I haven't tested it, but I guess we'd never reach the swap as the first line
> would keep calling itself forever?
> 
> Doesn't matter, since Mark's solution is good. And it has so many tests!

[Yusuke Suzuki]

Comment on attachment 405263 [details]
proposed patch.

r=me

[Darin Adler]

(In reply to Michael Catanzaro from comment #20)
> Hm... squinting at it:
> 
>     auto copy = other;
>     std::swap(m_vm, copy.m_vm);
> 
> Isn't it actually recursive?

No.

> DisallowVMEntryImpl& operator=(const DisallowVMEntryImpl& other)
> {
>     auto copy = other; // <-- calls itself?

No, I am pretty sure that this is not assignment, although it looks like assignment (C++ trivia, I can provide a reference if you can't easily find it); just copy construction that can use any non-explicit constructor. If it's clearer, we can also write this line one of these ways:

    auto copy { other };
    auto copy(other);

[Mark Lam]

Comment on attachment 405263 [details]
proposed patch.

Thanks for the review.  Landing now.

[EWS]

Committed r264937: <https://trac.webkit.org/changeset/264937>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 405263 [details].

[Radar WebKit Bug Importer]

<rdar://problem/66174144>

[Michael Catanzaro]

(In reply to Darin Adler from comment #23)
> No, I am pretty sure that this is not assignment, although it looks like
> assignment (C++ trivia, I can provide a reference if you can't easily find
> it); just copy construction that can use any non-explicit constructor. If
> it's clearer, we can also write this line one of these ways:
> 
>     auto copy { other };
>     auto copy(other);

You're right! Learning is good....