Bug 188480 – [macOS] Multiple third party apps crash due to the thread safety check in TimerBase::setNextFireTime

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Ryosuke Niwa]

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>

[Ryosuke Niwa]

Created attachment 346935 [details]
Diable release assert in WebKit1

[Ryosuke Niwa]

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.

[Jon Lee]

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*

[Ryosuke Niwa]

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.

[Simon Fraser (smfr)]

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.

[Alexey Proskuryakov]

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?

[David Kilzer (:ddkilzer)]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 346944 [details]
Updated per review comment

[Ryosuke Niwa]

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

[Ryosuke Niwa]

Committed r234778: <https://trac.webkit.org/changeset/234778>

[Radar WebKit Bug Importer]

<rdar://problem/43166861>

[Yusuke Suzuki]

@Ryosuke

We fixed the root cause of this at r246578. So, at some point, we can revert this suppression.