Multiple third party applications are crashing inside TimerBase::setNextFireTime due to the thread-safety release assert introduced in http://trac.webkit.org/r233821 to diagnose shared timer heap corruption caused by thread safety issues. We did catch & fix multiple bugs thanks to this release assert but this release assertion is hit by third party apps invoking WebView APIs in non-main threads on macOS. <rdar://problem/43158535>
Created attachment 346935 [details] Diable release assert in WebKit1
We initially planned to add a linked-on-or-after check but Jon Lee pointed out that then third party app developers may not realize the crash is caused by their misuse of the API because it'd be crashing deep down in WebKit framework. I think we should add a thread-safety check in our public API instead but that should probably be done in a separate patch.
Comment on attachment 346935 [details] Diable release assert in WebKit1 View in context: https://bugs.webkit.org/attachment.cgi?id=346935&action=review > Source/WebCore/ChangeLog:9 > + Supress the release assert in WebKit1 on macOS (isInWebProcess is always true in non-Cocoa platforms). Suppress* > Source/WebCore/ChangeLog:11 > + In the future, we should consider throwing Objective-C exceptions when thrid party apps call WebKit1 third*
Comment on attachment 346935 [details] Diable release assert in WebKit1 View in context: https://bugs.webkit.org/attachment.cgi?id=346935&action=review > Source/WebCore/platform/Timer.cpp:198 > - RELEASE_ASSERT(WebThreadIsEnabled() || canAccessThreadLocalDataForThread(m_thread.get())); > + RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()) || WebThreadIsEnabled()); I'm changing the order for consistency & reduce the runtime cost. After this, we'd only check WebThreadIsEnabled() when the assertion failed.
Comment on attachment 346935 [details] Diable release assert in WebKit1 View in context: https://bugs.webkit.org/attachment.cgi?id=346935&action=review > Source/WebCore/platform/Timer.cpp:200 > + RELEASE_ASSERT(canAccessThreadLocalDataForThread(m_thread.get()) || !isInWebProcess()); Why don't we use a linked-on-or-after check for this? We want people to fix their apps when the re-compile.
Comment on attachment 346935 [details] Diable release assert in WebKit1 View in context: https://bugs.webkit.org/attachment.cgi?id=346935&action=review >> Source/WebCore/ChangeLog:9 >> + Supress the release assert in WebKit1 on macOS (isInWebProcess is always true in non-Cocoa platforms). > > Suppress* It is possible that this will replace one common crash signature with many random ones - WebKit is quite likely to break when on a background thread. What evidence do we have that this patch reduces crash count?
Comment on attachment 346935 [details] Diable release assert in WebKit1 r- to add linked-on-or-after check and logging for developer to tell them how to fix the issue.
Turns out that are a lot of crashes that don't go through [WebView loadRequest:] so I guess we'd just add a linked-on-or-after check and not add a logging.
Created attachment 346944 [details] Updated per review comment
(In reply to Alexey Proskuryakov from comment #6) > Comment on attachment 346935 [details] > Diable release assert in WebKit1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=346935&action=review > > >> Source/WebCore/ChangeLog:9 > >> + Supress the release assert in WebKit1 on macOS (isInWebProcess is always true in non-Cocoa platforms). > > > > Suppress* > > It is possible that this will replace one common crash signature with many > random ones - WebKit is quite likely to break when on a background thread. > What evidence do we have that this patch reduces crash count? That's true but we have some reports saying that the apps that used not to crash are now crashing with a much higher frequency in Mojave with this stack trace.
Committed r234778: <https://trac.webkit.org/changeset/234778>
<rdar://problem/43166861>
@Ryosuke We fixed the root cause of this at r246578. So, at some point, we can revert this suppression.