[Webkit-unassigned] [Bug 220115] [GTK][WPE] UserInteractive threads are not Real-time in Linux

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=220115

--- Comment #45 from Michael Catanzaro <mcatanzaro at gnome.org> ---
Comment on attachment 441595
  --> https://bugs.webkit.org/attachment.cgi?id=441595
Patch

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

Looks mostly good. Couple issues:

> Source/WTF/wtf/linux/RealTimeThreads.cpp:128
> +        LOG_ERROR("Failed to set current thread as real time: %s", strerror(error));

I will complain about strerr below.

> Source/WTF/wtf/linux/RealTimeThreads.cpp:173
> +    RELEASE_ASSERT_NOT_REACHED();

Woah! We assert if an rtkit properly is invalid. That's not programmer error in WebKit, and it's not enough reason to crash either. Instead, call g_set_error_literal() with some canned message and return -1.

> Source/WTF/wtf/linux/RealTimeThreads.h:63
> +#if USE(GLIB)
> +    std::optional<GRefPtr<GDBusProxy>> m_realTimeKitProxy;
> +    RunLoop::Timer<RealTimeThreads> m_discardRealTimeKitProxyTimer;
> +#endif

You're careful to lock access to m_threadGroup, and to use m_enabled only on the main thread, but I see no such care with m_realTimeKitProxy and m_discardRealTimeKitProxyTimer. I think these both need to be guarded by a mutex. Correct?

> Source/WTF/wtf/posix/ThreadingPOSIX.cpp:308
> +            LOG_ERROR("Failed to set sched policy %d for thread %d: %s", policy, threadHandle, strerror(error));

Careful! strerror is not threadsafe and cannot be safely used *anywhere* in WebKit, or in other libraries too. (It returns a pointer to static data that, sadly, may be mutated by another thread.) Since g_strerror is not available here, you have to use strerror_r instead, which is frustratingly awkward to use since you have to manually allocate a buffer for the error string and then free it. But it's even worse, because glibc sabotaged everything by implementing it differently than POSIX, see http://club.cc.cmu.edu/~cmccabe/blog_strerror.html, and it's impossible to write code that is compatible with both versions, so it's all just such a huge pain to do correctly.

Anyway, there are a few other places in WebKit that make the same mistake, so you might want to define a wrapper in WTF that works similarly to g_strerror and avoids the problem. If you don't want to spend time on that, honestly the easiest thing to do would be to not print the error message, or just print it as an integer code. Actually using strerror_r requires extreme care. To get the standard version, you need (_POSIX_C_SOURCE >= 200112L) && ! _GNU_SOURCE.

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20211018/d8fc2843/attachment.htm>