Skip to content

Conversation

iangrunert
Copy link
Contributor

@iangrunert iangrunert commented Aug 22, 2024

f659dfd

[Win] Fix Wasm Compilation From Web Worker
https://bugs.webkit.org/show_bug.cgi?id=267323

Reviewed by Yusuke Suzuki.

We were calling SetTimer from the wasm compilation thread, which isn't
the thread that owns the run loop message window. That silently failed,
the timer wass never registered / fires as it's on the wrong thread.

In addition - the WorkerRunLoop was not cycling the window message pump
run loop - on other platforms this worked as both run loops used similar
message queue mechanisms.

We could fix this through wrapping timer start operations in a run loop
dispatch(), but that increases complexity through introducing another
source / type of async tasks in the loop. There's also a number of call
sites, and the risk of additional call sites being added with Windows
being the only platform where TimerBase::start() must be called on the
run loop thread.

We could also fix this through using run loop dispatch inside
TimerBase::start() / TimerBase::stop() - however it's difficult to
capture the TimerBase safely for the dispatch lambda with various
subclasses of TimerBase involved. This is necessary because the dispatch
introduces cases where the timer is destroyed before the dispatch
callback runs.

The solution used here is to make RunLoopWin PostMessage to the window
for scheduling / killing timers, so it can call SetTimer / KillTimer
from the correct thread. A HashSet is used in addition to keep track
of live timers - as sometimes a timer is able to fire but not have the
WM_TIMER event processed before KillTimer (and the timer destructor
running).

WorkerRunLoop calls RunLoop::cycle, and adjusts it's timeout based on
when the next deferredWorkTimer will fire.

* Source/WTF/wtf/RunLoop.h:
* Source/WTF/wtf/win/RunLoopWin.cpp:
(WTF::RunLoop::wndProc):
(WTF::RunLoop::TimerBase::start):
(WTF::RunLoop::TimerBase::stop):
* Source/WebCore/workers/WorkerRunLoop.cpp:
(WebCore::WorkerDedicatedRunLoop::runInMode):

Canonical link: https://commits.webkit.org/282906@main

4152509

Misc iOS, visionOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2 ✅ 🧪 wincairo-tests
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🧪 api-wpe
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🛠 wpe-cairo
✅ 🛠 🧪 jsc ✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🛠 gtk
✅ 🛠 🧪 jsc-arm64 ✅ 🛠 vision ✅ 🧪 mac-AS-debug-wk2 ✅ 🧪 gtk-wk2
✅ 🛠 vision-sim ✅ 🧪 mac-wk2-stress ✅ 🧪 api-gtk
✅ 🛠 🧪 merge ✅ 🧪 vision-wk2 ✅ 🧪 mac-intel-wk2 ✅ 🛠 jsc-armv7
✅ 🛠 tv ✅ 🧪 jsc-armv7-tests
✅ 🛠 tv-sim
✅ 🛠 watch
✅ 🛠 watch-sim

@iangrunert iangrunert requested a review from cdumez as a code owner August 22, 2024 20:14
@iangrunert iangrunert self-assigned this Aug 22, 2024
@iangrunert iangrunert force-pushed the ig/bug-267323-wasm-compilation-web-worker-win branch from 29b445b to 962cfb9 Compare August 22, 2024 20:49
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Aug 22, 2024
Copy link
Member

@Constellation Constellation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me

@@ -170,7 +179,8 @@ void RunLoop::TimerBase::start(Seconds interval, bool repeat)
m_isActive = true;
m_interval = interval;
m_nextFireDate = MonotonicTime::timePointFromNow(m_interval);
::SetTimer(m_runLoop->m_runLoopMessageWindow, bitwise_cast<uintptr_t>(this), interval.millisecondsAs<UINT>(), nullptr);
m_runLoop->m_liveTimers.add(bitwise_cast<uintptr_t>(this));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this accessing m_liveTimers from multiple threads without locking?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m_runLoop->m_loopLock is held here and inside RunLoop::TimerBase::stop when we call m_liveTimers.remove.

The m_runLoop->m_loopLock isn't held when we do the m_liveTimers.contains check inside wndProc. I could take the m_runLoop->m_loopLock there as well - if it's live we take it after we call timerFired() anyway.

@iangrunert iangrunert force-pushed the ig/bug-267323-wasm-compilation-web-worker-win branch from 962cfb9 to a7ddab5 Compare August 28, 2024 17:04
@iangrunert iangrunert force-pushed the ig/bug-267323-wasm-compilation-web-worker-win branch from a7ddab5 to 4152509 Compare August 28, 2024 17:07
@iangrunert iangrunert added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Aug 29, 2024
https://bugs.webkit.org/show_bug.cgi?id=267323

Reviewed by Yusuke Suzuki.

We were calling SetTimer from the wasm compilation thread, which isn't
the thread that owns the run loop message window. That silently failed,
the timer wass never registered / fires as it's on the wrong thread.

In addition - the WorkerRunLoop was not cycling the window message pump
run loop - on other platforms this worked as both run loops used similar
message queue mechanisms.

We could fix this through wrapping timer start operations in a run loop
dispatch(), but that increases complexity through introducing another
source / type of async tasks in the loop. There's also a number of call
sites, and the risk of additional call sites being added with Windows
being the only platform where TimerBase::start() must be called on the
run loop thread.

We could also fix this through using run loop dispatch inside
TimerBase::start() / TimerBase::stop() - however it's difficult to
capture the TimerBase safely for the dispatch lambda with various
subclasses of TimerBase involved. This is necessary because the dispatch
introduces cases where the timer is destroyed before the dispatch
callback runs.

The solution used here is to make RunLoopWin PostMessage to the window
for scheduling / killing timers, so it can call SetTimer / KillTimer
from the correct thread. A HashSet is used in addition to keep track
of live timers - as sometimes a timer is able to fire but not have the
WM_TIMER event processed before KillTimer (and the timer destructor
running).

WorkerRunLoop calls RunLoop::cycle, and adjusts it's timeout based on
when the next deferredWorkTimer will fire.

* Source/WTF/wtf/RunLoop.h:
* Source/WTF/wtf/win/RunLoopWin.cpp:
(WTF::RunLoop::wndProc):
(WTF::RunLoop::TimerBase::start):
(WTF::RunLoop::TimerBase::stop):
* Source/WebCore/workers/WorkerRunLoop.cpp:
(WebCore::WorkerDedicatedRunLoop::runInMode):

Canonical link: https://commits.webkit.org/282906@main
@webkit-commit-queue webkit-commit-queue force-pushed the ig/bug-267323-wasm-compilation-web-worker-win branch from 4152509 to f659dfd Compare August 29, 2024 14:52
@webkit-commit-queue
Copy link
Collaborator

Committed 282906@main (f659dfd): https://commits.webkit.org/282906@main

Reviewed commits have been landed. Closing PR #32602 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit f659dfd into WebKit:main Aug 29, 2024
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Aug 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants