Bug 188480 - [macOS] Multiple third party apps crash due to the thread safety check in TimerBase::setNextFireTime
Summary: [macOS] Multiple third party apps crash due to the thread safety check in Tim...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-08-10 15:33 PDT by Ryosuke Niwa
Modified: 2019-06-19 20:12 PDT (History)
13 users (show)

See Also:


Attachments
Diable release assert in WebKit1 (2.51 KB, patch)
2018-08-10 15:36 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Updated per review comment (4.44 KB, patch)
2018-08-10 17:19 PDT, Ryosuke Niwa
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2018-08-10 15:33:06 PDT
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>
Comment 1 Ryosuke Niwa 2018-08-10 15:36:51 PDT
Created attachment 346935 [details]
Diable release assert in WebKit1
Comment 2 Ryosuke Niwa 2018-08-10 15:38:48 PDT
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 3 Jon Lee 2018-08-10 15:42:49 PDT
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 4 Ryosuke Niwa 2018-08-10 15:42:53 PDT
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 5 Simon Fraser (smfr) 2018-08-10 16:04:29 PDT
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 6 Alexey Proskuryakov 2018-08-10 16:18:14 PDT
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 7 David Kilzer (:ddkilzer) 2018-08-10 16:32:31 PDT
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.
Comment 8 Ryosuke Niwa 2018-08-10 17:13:07 PDT
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.
Comment 9 Ryosuke Niwa 2018-08-10 17:19:15 PDT
Created attachment 346944 [details]
Updated per review comment
Comment 10 Ryosuke Niwa 2018-08-10 17:20:24 PDT
(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.
Comment 11 Ryosuke Niwa 2018-08-10 19:01:26 PDT
Committed r234778: <https://trac.webkit.org/changeset/234778>
Comment 12 Radar WebKit Bug Importer 2018-08-10 19:03:28 PDT
<rdar://problem/43166861>
Comment 13 Yusuke Suzuki 2019-06-19 20:12:51 PDT
@Ryosuke

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