[webkit-reviews] review denied: [Bug 31639] Add asserts to RefCounted to make sure ref/deref happens on the right thread. : [Attachment 88125] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has denied David Levin <levin at chromium.org>'s
request for review:
Bug 31639: Add asserts to RefCounted to make sure ref/deref happens on the
right thread.
https://bugs.webkit.org/show_bug.cgi?id=31639

Attachment 88125: Patch
https://bugs.webkit.org/attachment.cgi?id=88125&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=88125&action=review

review- because the Windows build is failing

> Source/JavaScriptCore/JavaScriptCore.exp:608
> +__ZN3WTF26singleThreadVerifierCreateEv
> +__ZN3WTF20noVerificationCreateEv
> +__ZN3WTF19mutexVerifierCreateERNS_5MutexE

This file is sorted with the sort tool. New entries in it should be sorted in
the same way.

> Source/JavaScriptCore/jit/ExecutableAllocator.h:354
> +   
setNonThreadSafeVerifier(WTF::NonThreadSafeVerifier::createNoVerficationDoNotUs
eThis());

Classes in WTF should make use of "using" in the WTF way and not have WTF:: at
the call site.

> Source/JavaScriptCore/parser/SourceProvider.h:50
> +	      
setNonThreadSafeVerifier(WTF::NonThreadSafeVerifier::createNoVerficationDoNotUs
eThis());

While factoring-wise I think it’s OK to have the verifier be a separate object,
I think that it involves tons of memory allocation in debug builds and so could
slow them to a crawly, and the idiom of setting up a verifier is so wordy that
it almost obscures the meaning of what’s being done.

> Source/JavaScriptCore/wtf/NonThreadSafeVerifier.cpp:52
> +    virtual void setShared(bool);
> +    virtual bool verifySafety() const;

I suggest making these private instead of public on the derived class.

> Source/JavaScriptCore/wtf/NonThreadSafeVerifier.cpp:76
> +    virtual void setShared(bool);
> +    virtual bool verifySafety() const;

Ditto.

> Source/JavaScriptCore/wtf/NonThreadSafeVerifier.cpp:183
> +PassOwnPtr<NonThreadSafeVerifier> singleThreadVerifierCreate()
> +{
> +    return 0;
> +}
> +
> +PassOwnPtr<NonThreadSafeVerifier> mutexVerifierCreate(Mutex&)
> +{
> +    return 0;
> +}
> +
> +PassOwnPtr<NonThreadSafeVerifier> noVerificationCreate()
> +{
> +    return 0;
> +}

Why define these at all in NDEBUG versions?

> Source/JavaScriptCore/wtf/NonThreadSafeVerifier.h:54
> +    static PassOwnPtr<NonThreadSafeVerifier>
createNoVerficationDoNotUseThis();

Typo: Verfication

> Source/JavaScriptCore/wtf/NonThreadSafeVerifier.h:68
> +// Don't use these directly.
> +PassOwnPtr<NonThreadSafeVerifier> singleThreadVerifierCreate();
> +PassOwnPtr<NonThreadSafeVerifier> mutexVerifierCreate(Mutex&);
> +PassOwnPtr<NonThreadSafeVerifier> noVerificationCreate();

Why not? Why do we need the extra level of indirection?

If we don’t want them used directly, then we could obfuscate them a little more
by putting them into a namespace or making them private static member
functions. And we could not even define them if NDEBUG is set.

> Source/JavaScriptCore/wtf/RefCounted.h:45
> +	   // to another thread where to ref count get increased. This

“where to ref count get increased”?

> Source/JavaScriptCore/wtf/RefCounted.h:88
> +    void setNonThreadSafeVerifier(PassOwnPtr<NonThreadSafeVerifier>
nonThreadSafeVerifier)
> +    {
> +	   UNUSED_PARAM(nonThreadSafeVerifier.get());
> +#ifndef NDEBUG
> +	   m_nonThreadSafeVerifier = nonThreadSafeVerifier;
> +#endif
> +    }
> +
> +    void setMutexAsThreadSafeVerifier(Mutex& mutex)
> +    {
> +	   UNUSED_PARAM(&mutex);
> +#ifndef NDEBUG
> +	  
setNonThreadSafeVerifier(NonThreadSafeVerifier::createMutexVerifier(mutex));
> +#endif
> +    }

I suggest putting the whole functions inside an #ifndef to avoid the awkward
uses of UNUSED_PARAM.

I also question the need to inline the debug versions. I think you could just
declare these in the class, define the NDEBUG versions below with inline, and
for the debug versions have them in a cpp file.

> Source/JavaScriptCore/wtf/RefCounted.h:165
> +    OwnPtr<NonThreadSafeVerifier> m_nonThreadSafeVerifier;

I think this data member could just be named m_verifier without losing clarity.