[WTF] Align assumption in RunLoopWin to the other platform's RunLoop
authorHironori.Fujii@sony.com <Hironori.Fujii@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2019 02:36:23 +0000 (02:36 +0000)
committerHironori.Fujii@sony.com <Hironori.Fujii@sony.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 11 Mar 2019 02:36:23 +0000 (02:36 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181151

Source/WTF:

Reviewed by Don Olmstead.

This patch fixes RunLoop in Windows to align it to the implementations in the other platforms
to use RunLoop more aggressively.

* wtf/RunLoop.h:
(WTF::RunLoop::Timer::Timer):
* wtf/win/MainThreadWin.cpp:
(initializeMainThreadPlatform): Call RunLoop::registerRunLoopMessageWindowClass.
* wtf/win/RunLoopWin.cpp:
(WTF::RunLoop::wndProc):
(WTF::RunLoop::iterate):
(WTF::RunLoop::stop):
PostQuitMessage is only available in the RunLoop's thread. We should post a message and call
it inside this task.

(WTF::RunLoop::registerRunLoopMessageWindowClass):
Changed the return type from bool to void, and added RELEASE_ASSERT to check the return value of RegisterClass.

(WTF::RunLoop::~RunLoop):
When the RunLoop's thread is freed, its associated window is freed. We do not need to do here.

(WTF::RunLoop::TimerBase::timerFired):
(WTF::RunLoop::TimerBase::TimerBase):
(WTF::RunLoop::TimerBase::start):
(WTF::RunLoop::TimerBase::stop):
(WTF::RunLoop::TimerBase::isActive const):
(WTF::RunLoop::TimerBase::secondsUntilFire const):
(WTF::generateTimerID): Deleted.
We can use TimerBase's pointer as ID since it is uintptr_t.

Tools:

Patch by Yusuke Suzuki <utatane.tea@gmail.com> on 2019-03-10
Reviewed by Don Olmstead.

* TestWebKitAPI/CMakeLists.txt:
* TestWebKitAPI/PlatformWin.cmake:
Enable TestWTF RunLoop tests in all platforms.

* TestWebKitAPI/Tests/WTF/RunLoop.cpp:
(TestWebKitAPI::DerivedOneShotTimer::DerivedOneShotTimer):
(TestWebKitAPI::DerivedOneShotTimer::fired):
(TestWebKitAPI::TEST):
Only a few platforms support nested RunLoop.

(TestWebKitAPI::DerivedRepeatingTimer::DerivedRepeatingTimer):
(TestWebKitAPI::DerivedRepeatingTimer::fired):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242694 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WTF/ChangeLog
Source/WTF/wtf/RunLoop.h
Source/WTF/wtf/win/MainThreadWin.cpp
Source/WTF/wtf/win/RunLoopWin.cpp
Tools/ChangeLog
Tools/TestWebKitAPI/CMakeLists.txt
Tools/TestWebKitAPI/PlatformWin.cmake
Tools/TestWebKitAPI/Tests/WTF/RunLoop.cpp

index 305b232..f5b716d 100644 (file)
@@ -1,3 +1,39 @@
+2019-03-10  Yusuke Suzuki <utatane.tea@gmail.com> and Fujii Hironori  <Hironori.Fujii@sony.com>
+
+        [WTF] Align assumption in RunLoopWin to the other platform's RunLoop
+        https://bugs.webkit.org/show_bug.cgi?id=181151
+
+        Reviewed by Don Olmstead.
+
+        This patch fixes RunLoop in Windows to align it to the implementations in the other platforms
+        to use RunLoop more aggressively.
+
+        * wtf/RunLoop.h:
+        (WTF::RunLoop::Timer::Timer):
+        * wtf/win/MainThreadWin.cpp:
+        (initializeMainThreadPlatform): Call RunLoop::registerRunLoopMessageWindowClass.
+        * wtf/win/RunLoopWin.cpp:
+        (WTF::RunLoop::wndProc):
+        (WTF::RunLoop::iterate):
+        (WTF::RunLoop::stop):
+        PostQuitMessage is only available in the RunLoop's thread. We should post a message and call
+        it inside this task.
+
+        (WTF::RunLoop::registerRunLoopMessageWindowClass):
+        Changed the return type from bool to void, and added RELEASE_ASSERT to check the return value of RegisterClass.
+
+        (WTF::RunLoop::~RunLoop):
+        When the RunLoop's thread is freed, its associated window is freed. We do not need to do here.
+
+        (WTF::RunLoop::TimerBase::timerFired):
+        (WTF::RunLoop::TimerBase::TimerBase):
+        (WTF::RunLoop::TimerBase::start):
+        (WTF::RunLoop::TimerBase::stop):
+        (WTF::RunLoop::TimerBase::isActive const):
+        (WTF::RunLoop::TimerBase::secondsUntilFire const):
+        (WTF::generateTimerID): Deleted.
+        We can use TimerBase's pointer as ID since it is uintptr_t.
+
 2019-03-07  Said Abou-Hallawa  <sabouhallawa@apple.com>
 
         requestAnimationFrame should execute before the next frame
index f0da3ef..d615e07 100644 (file)
@@ -68,11 +68,15 @@ public:
     WTF_EXPORT_PRIVATE GMainContext* mainContext() const { return m_mainContext.get(); }
 #endif
 
-#if USE(GENERIC_EVENT_LOOP)
+#if USE(GENERIC_EVENT_LOOP) || USE(WINDOWS_EVENT_LOOP)
     // Run the single iteration of the RunLoop. It consumes the pending tasks and expired timers, but it won't be blocked.
     WTF_EXPORT_PRIVATE static void iterate();
 #endif
 
+#if USE(WINDOWS_EVENT_LOOP)
+    static void registerRunLoopMessageWindowClass();
+#endif
+
 #if USE(GLIB_EVENT_LOOP) || USE(GENERIC_EVENT_LOOP)
     WTF_EXPORT_PRIVATE void dispatchAfter(Seconds, Function<void()>&&);
 #endif
@@ -110,11 +114,11 @@ public:
 
 #if USE(WINDOWS_EVENT_LOOP)
         bool isActive(const AbstractLocker&) const;
-        static void timerFired(RunLoop*, uint64_t ID);
-        uint64_t m_ID;
+        void timerFired();
         MonotonicTime m_nextFireDate;
         Seconds m_interval;
-        bool m_isRepeating;
+        bool m_isRepeating { false };
+        bool m_isActive { false };
 #elif USE(COCOA_EVENT_LOOP)
         static void timerFired(CFRunLoopTimerRef, void*);
         RetainPtr<CFRunLoopTimerRef> m_timer;
@@ -139,16 +143,18 @@ public:
 
         Timer(RunLoop& runLoop, TimerFiredClass* o, TimerFiredFunction f)
             : TimerBase(runLoop)
-            , m_object(o)
             , m_function(f)
+            , m_object(o)
         {
         }
 
     private:
         void fired() override { (m_object->*m_function)(); }
 
-        TimerFiredClass* m_object;
+        // This order should be maintained due to MSVC bug.
+        // http://computer-programming-forum.com/7-vc.net/6fbc30265f860ad1.htm
         TimerFiredFunction m_function;
+        TimerFiredClass* m_object;
     };
 
     class Holder;
@@ -162,14 +168,11 @@ private:
     Deque<Function<void()>> m_functionQueue;
 
 #if USE(WINDOWS_EVENT_LOOP)
-    static bool registerRunLoopMessageWindowClass();
     static LRESULT CALLBACK RunLoopWndProc(HWND, UINT, WPARAM, LPARAM);
     LRESULT wndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam);
     HWND m_runLoopMessageWindow;
 
-    typedef HashMap<uint64_t, TimerBase*> TimerMap;
-    Lock m_activeTimersLock;
-    TimerMap m_activeTimers;
+    Lock m_loopLock;
 #elif USE(COCOA_EVENT_LOOP)
     static void performWork(void*);
     RetainPtr<CFRunLoopRef> m_runLoop;
index e3ef336..5d0121a 100644 (file)
@@ -31,6 +31,7 @@
 #include <wtf/MainThread.h>
 
 #include <wtf/Assertions.h>
+#include <wtf/RunLoop.h>
 #include <wtf/Threading.h>
 #include <wtf/WindowsExtras.h>
 
@@ -68,6 +69,7 @@ void initializeMainThreadPlatform()
     mainThread = Thread::currentID();
 
     Thread::initializeCurrentThreadInternal("Main Thread");
+    RunLoop::registerRunLoopMessageWindowClass();
 }
 
 bool isMainThread()
index 30e4359..670914b 100644 (file)
@@ -56,7 +56,7 @@ LRESULT RunLoop::wndProc(HWND hWnd, UINT message, WPARAM wParam, LPARAM lParam)
         performWork();
         return 0;
     case WM_TIMER:
-        RunLoop::TimerBase::timerFired(this, wParam);
+        bitwise_cast<RunLoop::TimerBase*>(wParam)->timerFired();
         return 0;
     }
 
@@ -74,27 +74,36 @@ void RunLoop::run()
     }
 }
 
-void RunLoop::stop()
+void RunLoop::iterate()
 {
-    ::PostQuitMessage(0);
+    MSG message;
+    while (::PeekMessage(&message, 0, 0, 0, PM_REMOVE)) {
+        ::TranslateMessage(&message);
+        ::DispatchMessage(&message);
+    }
 }
 
-bool RunLoop::registerRunLoopMessageWindowClass()
+void RunLoop::stop()
 {
-    // FIXME: This really only needs to be called once.
+    // RunLoop::stop() can be called from threads unrelated to this RunLoop.
+    // We should post a message that call PostQuitMessage in RunLoop's thread.
+    dispatch([] {
+        ::PostQuitMessage(0);
+    });
+}
 
-    WNDCLASS windowClass { };
+void RunLoop::registerRunLoopMessageWindowClass()
+{
+    WNDCLASS windowClass = { };
     windowClass.lpfnWndProc     = RunLoop::RunLoopWndProc;
     windowClass.cbWndExtra      = sizeof(RunLoop*);
     windowClass.lpszClassName   = kRunLoopMessageWindowClassName;
-
-    return !!::RegisterClass(&windowClass);
+    bool result = ::RegisterClass(&windowClass);
+    RELEASE_ASSERT(result);
 }
 
 RunLoop::RunLoop()
 {
-    registerRunLoopMessageWindowClass();
-
     m_runLoopMessageWindow = ::CreateWindow(kRunLoopMessageWindowClassName, 0, 0,
         CW_USEDEFAULT, 0, CW_USEDEFAULT, 0, HWND_MESSAGE, 0, 0, this);
     ASSERT(::IsWindow(m_runLoopMessageWindow));
@@ -102,7 +111,6 @@ RunLoop::RunLoop()
 
 RunLoop::~RunLoop()
 {
-    // FIXME: Tear down the work item queue here.
 }
 
 void RunLoop::wakeUp()
@@ -114,39 +122,26 @@ void RunLoop::wakeUp()
 
 // RunLoop::Timer
 
-void RunLoop::TimerBase::timerFired(RunLoop* runLoop, uint64_t ID)
+void RunLoop::TimerBase::timerFired()
 {
-    TimerBase* timer = nullptr;
     {
-        LockHolder locker(runLoop->m_activeTimersLock);
-        TimerMap::iterator it = runLoop->m_activeTimers.find(ID);
-        if (it == runLoop->m_activeTimers.end()) {
-            // The timer must have been stopped after the WM_TIMER message was posted to the message queue.
-            return;
-        }
+        LockHolder locker(m_runLoop->m_loopLock);
 
-        timer = it->value;
+        if (!m_isActive)
+            return;
 
-        if (!timer->m_isRepeating) {
-            runLoop->m_activeTimers.remove(it);
-            ::KillTimer(runLoop->m_runLoopMessageWindow, ID);
+        if (!m_isRepeating) {
+            m_isActive = false;
+            ::KillTimer(m_runLoop->m_runLoopMessageWindow, bitwise_cast<uintptr_t>(this));
         } else
-            timer->m_nextFireDate = MonotonicTime::now() + timer->m_interval;
+            m_nextFireDate = MonotonicTime::now() + m_interval;
     }
 
-    timer->fired();
-}
-
-static uint64_t generateTimerID()
-{
-    static uint64_t uniqueTimerID = 1;
-    return uniqueTimerID++;
+    fired();
 }
 
 RunLoop::TimerBase::TimerBase(RunLoop& runLoop)
     : m_runLoop(runLoop)
-    , m_ID(generateTimerID())
-    , m_isRepeating(false)
 {
 }
 
@@ -157,43 +152,41 @@ RunLoop::TimerBase::~TimerBase()
 
 void RunLoop::TimerBase::start(Seconds nextFireInterval, bool repeat)
 {
-    LockHolder locker(m_runLoop->m_activeTimersLock);
+    LockHolder locker(m_runLoop->m_loopLock);
     m_isRepeating = repeat;
-    m_runLoop->m_activeTimers.set(m_ID, this);
+    m_isActive = true;
     m_interval = nextFireInterval;
     m_nextFireDate = MonotonicTime::now() + m_interval;
-    ::SetTimer(m_runLoop->m_runLoopMessageWindow, m_ID, nextFireInterval.millisecondsAs<unsigned>(), 0);
+    ::SetTimer(m_runLoop->m_runLoopMessageWindow, bitwise_cast<uintptr_t>(this), nextFireInterval.millisecondsAs<UINT>(), 0);
 }
 
 void RunLoop::TimerBase::stop()
 {
-    LockHolder locker(m_runLoop->m_activeTimersLock);
-    TimerMap::iterator it = m_runLoop->m_activeTimers.find(m_ID);
-    if (it == m_runLoop->m_activeTimers.end())
+    LockHolder locker(m_runLoop->m_loopLock);
+    if (!isActive(locker))
         return;
 
-    m_runLoop->m_activeTimers.remove(it);
-    ::KillTimer(m_runLoop->m_runLoopMessageWindow, m_ID);
+    m_isActive = false;
+    ::KillTimer(m_runLoop->m_runLoopMessageWindow, bitwise_cast<uintptr_t>(this));
 }
 
 bool RunLoop::TimerBase::isActive(const AbstractLocker&) const
 {
-    return m_runLoop->m_activeTimers.contains(m_ID);
+    return m_isActive;
 }
 
 bool RunLoop::TimerBase::isActive() const
 {
-    LockHolder locker(m_runLoop->m_activeTimersLock);
+    LockHolder locker(m_runLoop->m_loopLock);
     return isActive(locker);
 }
 
 Seconds RunLoop::TimerBase::secondsUntilFire() const
 {
-    LockHolder locker(m_runLoop->m_activeTimersLock);
+    LockHolder locker(m_runLoop->m_loopLock);
     if (isActive(locker))
         return std::max<Seconds>(m_nextFireDate - MonotonicTime::now(), 0_s);
     return 0_s;
 }
 
-
 } // namespace WTF
index bf90be9..51b1330 100644 (file)
@@ -1,3 +1,23 @@
+2019-03-10  Yusuke Suzuki <utatane.tea@gmail.com>
+
+        [WTF] Align assumption in RunLoopWin to the other platform's RunLoop
+        https://bugs.webkit.org/show_bug.cgi?id=181151
+
+        Reviewed by Don Olmstead.
+
+        * TestWebKitAPI/CMakeLists.txt:
+        * TestWebKitAPI/PlatformWin.cmake:
+        Enable TestWTF RunLoop tests in all platforms.
+
+        * TestWebKitAPI/Tests/WTF/RunLoop.cpp:
+        (TestWebKitAPI::DerivedOneShotTimer::DerivedOneShotTimer):
+        (TestWebKitAPI::DerivedOneShotTimer::fired):
+        (TestWebKitAPI::TEST):
+        Only a few platforms support nested RunLoop.
+
+        (TestWebKitAPI::DerivedRepeatingTimer::DerivedRepeatingTimer):
+        (TestWebKitAPI::DerivedRepeatingTimer::fired):
+
 2019-03-10  David Quesada  <david_quesada@apple.com>
 
         ASSERT(m_downloads.isEmpty()) fails in DownloadProxyMap::~DownloadProxyMap()
index ca9acc8..6843d03 100644 (file)
@@ -153,6 +153,7 @@ set(TestWTF_SOURCES
     ${TESTWEBKITAPI_DIR}/Tests/WTF/RefCounter.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/RefLogger.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/RefPtr.cpp
+    ${TESTWEBKITAPI_DIR}/Tests/WTF/RunLoop.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/SHA1.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/SaturatedArithmeticOperations.cpp
     ${TESTWEBKITAPI_DIR}/Tests/WTF/Scope.cpp
@@ -186,7 +187,6 @@ set(TestWTF_SOURCES
 if (NOT WIN32)
     list(APPEND TestWTF_SOURCES
         ${TESTWEBKITAPI_DIR}/Tests/WTF/FileSystem.cpp
-        ${TESTWEBKITAPI_DIR}/Tests/WTF/RunLoop.cpp
     )
 endif ()
 
index 24194a9..b49c92e 100644 (file)
@@ -107,6 +107,7 @@ endif ()
 add_library(TestWTFLib SHARED
     ${test_main_SOURCES}
     ${TestWTF_SOURCES}
+    ${TESTWEBKITAPI_DIR}/win/UtilitiesWin.cpp
 )
 set_target_properties(TestWTFLib PROPERTIES OUTPUT_NAME "TestWTFLib")
 target_link_libraries(TestWTFLib ${test_wtf_LIBRARIES})
index 93b3d8c..9350b0e 100644 (file)
@@ -27,6 +27,7 @@
 
 #include "Utilities.h"
 #include <wtf/RunLoop.h>
+#include <wtf/Threading.h>
 
 namespace TestWebKitAPI {
 
@@ -54,90 +55,69 @@ TEST(WTF_RunLoop, Deadlock)
     Util::run(&testFinished);
 }
 
-TEST(WTF_RunLoop, NestedRunLoop)
-{
-    RunLoop::initializeMainRunLoop();
+class DerivedOneShotTimer : public RunLoop::Timer<DerivedOneShotTimer> {
+public:
+    DerivedOneShotTimer(bool& testFinished)
+        : RunLoop::Timer<DerivedOneShotTimer>(RunLoop::current(), this, &DerivedOneShotTimer::fired)
+        , m_testFinished(testFinished)
+    {
+    }
 
-    bool testFinished = false;
-    RunLoop::current().dispatch([&] {
-        RunLoop::current().dispatch([&] {
-            testFinished = true;
-        });
-        Util::run(&testFinished);
-    });
+    void fired()
+    {
+        m_testFinished = true;
+        stop();
+    }
+
+private:
+    bool& m_testFinished;
+};
 
-    Util::run(&testFinished);
-}
 
 TEST(WTF_RunLoop, OneShotTimer)
 {
     RunLoop::initializeMainRunLoop();
 
     bool testFinished = false;
+    DerivedOneShotTimer timer(testFinished);
+    timer.startOneShot(100_ms);
+    Util::run(&testFinished);
+}
 
-    class DerivedTimer : public RunLoop::Timer<DerivedTimer> {
-    public:
-        DerivedTimer(bool& testFinished)
-            : RunLoop::Timer<DerivedTimer>(RunLoop::current(), this, &DerivedTimer::fired)
-            , m_testFinished(testFinished)
-        {
-        }
+class DerivedRepeatingTimer : public RunLoop::Timer<DerivedRepeatingTimer> {
+public:
+    DerivedRepeatingTimer(bool& testFinished)
+        : RunLoop::Timer<DerivedRepeatingTimer>(RunLoop::current(), this, &DerivedRepeatingTimer::fired)
+        , m_testFinished(testFinished)
+    {
+    }
 
-        void fired()
-        {
+    void fired()
+    {
+        if (++m_count == 10) {
             m_testFinished = true;
             stop();
         }
+    }
 
-    private:
-        bool& m_testFinished;
-    };
+private:
+    unsigned m_count { 0 };
+    bool& m_testFinished;
+};
 
-    {
-        DerivedTimer timer(testFinished);
-        timer.startOneShot(100_ms);
-        Util::run(&testFinished);
-    }
-}
 
 TEST(WTF_RunLoop, RepeatingTimer)
 {
     RunLoop::initializeMainRunLoop();
 
     bool testFinished = false;
-
-    class DerivedTimer : public RunLoop::Timer<DerivedTimer> {
-    public:
-        DerivedTimer(bool& testFinished)
-            : RunLoop::Timer<DerivedTimer>(RunLoop::current(), this, &DerivedTimer::fired)
-            , m_testFinished(testFinished)
-        {
-        }
-
-        void fired()
-        {
-            if (++m_count == 10) {
-                m_testFinished = true;
-                stop();
-            }
-        }
-
-    private:
-        unsigned m_count { 0 };
-        bool& m_testFinished;
-    };
-
-    {
-        DerivedTimer timer(testFinished);
-        timer.startRepeating(10_ms);
-        Util::run(&testFinished);
-    }
+    DerivedRepeatingTimer timer(testFinished);
+    timer.startRepeating(10_ms);
+    Util::run(&testFinished);
 }
 
 TEST(WTF_RunLoop, ManyTimes)
 {
-    RunLoop::initializeMainRunLoop();
-
     class Counter {
     public:
         void run()
@@ -155,12 +135,13 @@ TEST(WTF_RunLoop, ManyTimes)
         unsigned m_count { 0 };
     };
 
-    Counter counter;
-
-    RunLoop::current().dispatch([&counter] {
-        counter.run();
-    });
-    RunLoop::run();
+    Thread::create("RunLoopManyTimes", [] {
+        Counter counter;
+        RunLoop::current().dispatch([&counter] {
+            counter.run();
+        });
+        RunLoop::run();
+    })->waitForCompletion();
 }
 
 } // namespace TestWebKitAPI