[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 #52 from Carlos Garcia Campos <cgarcia at igalia.com> ---
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

>> 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.

This is always called in the main thread.

>> 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.

That would mean it's not the rtkit service we expect to talk to, we always expect the properties to be either a int64 or int32.

>> Source/WTF/wtf/linux/RealTimeThreads.h:63
>> +#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?

RunLoop::Timer is thread safe. it's a *main loop* timer, so the callback is always called in the main thread. And m_realTimeKitProxy is also called in the main thread only, because promoteThreadToRealTime() is always called in the main thread.

>> 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.

Yes, I followed the pattern of the previous LOG_ERROR in this function...

-- 
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/20211019/f7ec1272/attachment-0001.htm>