[webkit-reviews] review granted: [Bug 180308] WTF shouldn't have both Thread and ThreadIdentifier : [Attachment 328295] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 180308: WTF shouldn't have both Thread and ThreadIdentifier
https://bugs.webkit.org/show_bug.cgi?id=180308

Attachment 328295: Patch

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




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

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

Good idea to clean this up; messy.

> Source/WTF/wtf/RecursiveLockAdapter.h:86
> +    Thread* m_owner { nullptr }; // Use Thread* instead of RefPtr<Thread>
since owner thread is always alive while m_onwer is et.

Typo: m_onwer

> Source/WTF/wtf/Threading.h:146
>      bool operator==(const Thread& thread)

Keeping these operator== is a little bit peculiar. It means that we can compare
a thread with another without using .get(), .ptr() or &, which I suppose is
handy, but we could have done that for almost any other object that represents
something unique. Like even, say, DOM::Document.

It was certainly strange before that we guaranteed uniqueness of both the ID an
of the reference counted thread object.

I would be tempted to remove these entirely; we would need to change more code
then, but I think it would be logical to compare RefPtr<Thread> or Thread*
rather than doing == on the Thread objects themselves since they are all unique
and no two are equal unless they are the same one. Or if we are keeping these
operators, I would be tempted to add even more overloads so we can compare a
Thread* with a Thread& without doing anything.

> Source/WTF/wtf/ThreadingPthreads.cpp:303
> +	   LOG_ERROR("Thread %p was unable to be detached\n", this);

Also need to remove the reference to ThreadIdentifier in the comment in
Thread::initializeCurrentTLS.

> Source/WebCore/Modules/webaudio/AudioContext.cpp:136
> -    , m_graphOwnerThread(UndefinedThreadIdentifier)
> +    , m_graphOwnerThread(nullptr)

Initialize in class definition instead?

> Source/WebCore/Modules/webaudio/AudioContext.cpp:152
> -    , m_graphOwnerThread(UndefinedThreadIdentifier)
> +    , m_graphOwnerThread(nullptr)

Ditto.

> Source/WebCore/Modules/webaudio/AudioContext.h:387
> -    volatile ThreadIdentifier m_audioThread { 0 };
> -    volatile ThreadIdentifier m_graphOwnerThread; // if the lock is held
then this is the thread which owns it, otherwise == UndefinedThreadIdentifier
> +    // FIXME: Using volatile seems incorrect.
> +    // https://bugs.webkit.org/show_bug.cgi?id=180332
> +    volatile Thread* m_audioThread { nullptr };
> +    volatile Thread* m_graphOwnerThread; // if the lock is held then this is
the thread which owns it, otherwise == nullptr.

If volatile needs to be kept at all, then it needs to be after the "*":

    Thread* volatile m_audioThread { nullptr };
    Thread* volatile m_graphOwnerThread { nullptr }; // Thread which owns lock
currently held.

It’s the storage of the data member that was marked volatile before, previously
an integer, now a pointer, not the storage of the contents of a Thread object,
which is what volatile Thread* means. I am not sure the volatile is needed, but
the volatile before the * is definitely not helpful.

> Source/WebCore/Modules/webdatabase/DatabaseDetails.h:41
>	   , m_creationTime(0)

This seems way more like a struct than a class, not to be changed in this
patch, I guess.

> Source/WebCore/Modules/webdatabase/DatabaseDetails.h:80
> +    RefPtr<Thread> m_thread;

Why not Ref<Thread> m_thread { Thread::current() } as we have been doing other
places?

> Source/WebCore/platform/Supplementable.h:127
> +    Ref<Thread> m_thread;

Why not initialize here as you have in other places?

> Source/WebCore/platform/graphics/texmap/TextureMapperPlatformLayerProxy.h:94
> +    RefPtr<Thread> m_compositorThread { nullptr };

No need for { nullptr } for a RefPtr. They all do that automatically.

> Source/WebCore/workers/service/ServiceWorkerContainer.cpp:67
> +    ASSERT(m_creationThread.get() == Thread::current());

I am really surprised that these .get() are needed. What happens if we don’t do
get()? I think maybe the reason they are needed is that we overload the
operator== as a class member of the Thread class instead of writing it as a
free function that takes two Thread& or maybe const Thread&.

If we are going so far as to actually keep the overload of ==, then I think we
should try to do it in a way where we don’t need to do all this get().

Without the overload of ==, I think it would be:

    ASSERT(m_creationThread.ptr() == &Thread::current());

> Source/WebCore/workers/service/ServiceWorkerJob.cpp:50
> +    ASSERT(Thread::current() == m_creationThread.get());

Annoying to have the same idiom here as ServicesWorkerContainer, but with all
the assertions in the opposite order! We should fix them to match.

> Source/WebCore/xml/parser/XMLDocumentParserLibxml2.cpp:500
> +    static std::once_flag flag;

Why move from a simple boolean to a std::call_once? I don’t think this function
needs to be thread-safe; but maybe I am mistaken.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1069
> +static Thread* Thread1 { nullptr };
> +static Thread* Thread2 { nullptr };

There is no reason these need to be outside the function. They can be static,
but inside the function. Also no need for the { nullptr } since all globals get
initialized.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1071
>  static void testThreadIdentifierMap()

The name of this function no longer makes sense. Could be ThreadIdentityMap or
ThreadObjectMap or just ThreadMap.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1089
>      // Now create another thread using WTF. On OSX, it will have the same
pthread handle

The comment here confidently states that on macOS this will end up getting the
same pthread handle. But I am not sure that will always happen, the code
doesn’t assert it, and if it doesn’t happen then the test isn’t really testing
what it claims to test.

> Tools/DumpRenderTree/mac/DumpRenderTree.mm:1090
> +    // but should get a different RefPtr<Thread> if the previous
RefPtr<thread> is held.

Type here, "thread" instead of "Thread".

But also, this old test is in the wrong place. If we were adding it today it
would be in TestWebKitAPI, not inside DumpRenderTree. We should consider moving
it there.