Make sure implementation of InitializeWebKit2() always run on the main thread

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Make sure implementation of InitializeWebKit2() always run on the main thread as it is not safe to run it on any other thread (see <rdar://problem/32280185>).

[Radar WebKit Bug Importer]

<rdar://problem/32295678>

[Chris Dumez]

Created attachment 310670 [details]
Patch

[Chris Dumez]

I have verified locally that <rdar://problem/32280185> no longer reproduces with this patch, even though the client is using the API on a background thread.

[Geoffrey Garen]

Comment on attachment 310670 [details]
Patch

I think this is a fine work-around for the threading problems we've discovered in Safari.

In the long run, I think we need to remove this design. Instead of wallpapering over threading problems, since this is a main-thread-only API, we should ASSERT that it's only invoked on the main thread, and fix the cases where that's not true.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #4)
> Comment on attachment 310670 [details]
> Patch
> 
> I think this is a fine work-around for the threading problems we've
> discovered in Safari.
> 
> In the long run, I think we need to remove this design. Instead of
> wallpapering over threading problems, since this is a main-thread-only API,
> we should ASSERT that it's only invoked on the main thread, and fix the
> cases where that's not true.

Understood, we should try and ASSERT later once we've fixed API usage on non-main thread on client side.

[WebKit Commit Bot]

Comment on attachment 310670 [details]
Patch

Clearing flags on attachment: 310670

Committed r217137: <http://trac.webkit.org/changeset/217137>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

(In reply to Chris Dumez from comment #5)
> Understood, we should try and ASSERT later once we've fixed API usage on
> non-main thread on client side.

In the past for Objective-C APIs we made a linked on or after check. If the call was from code linked with a new enough version of WebKit, we threw an Objective-C exception if the API was called on the wrong thread. That worked well.

[Csaba Osztrogonác]

(In reply to WebKit Commit Bot from comment #6)
> Comment on attachment 310670 [details]
> Patch
> 
> Clearing flags on attachment: 310670
> 
> Committed r217137: <http://trac.webkit.org/changeset/217137>

FYI, it broke the Apple Mac cmake build, because you missed to add the
new file to the build system:
Undefined symbols for architecture x86_64:
  "WebKit::InitializeWebKit2()", referenced from:
      API::Object::Object() in APIObject.cpp.o
      WebKit::WebProcessPool::create(API::ProcessPoolConfiguration&) in WebProcessPool.cpp.o
      API::WebsiteDataStore::defaultDataStore() in APIWebsiteDataStore.cpp.o
      -[WKWebViewConfiguration init] in WKWebViewConfiguration.mm.o
      -[_WKUserStyleSheet initWithSource:forMainFrameOnly:] in _WKUserStyleSheet.mm.o
      -[_WKUserStyleSheet initWithSource:forMainFrameOnly:legacyWhitelist:legacyBlacklist:userContentWorld:] in _WKUserStyleSheet.mm.o
      -[_WKUserStyleSheet initWithSource:forMainFrameOnly:legacyWhitelist:legacyBlacklist:baseURL:userContentWorld:] in _WKUserStyleSheet.mm.o
      ...
ld: symbol(s) not found for architecture x86_64

[Csaba Osztrogonác]

fix landed in https://trac.webkit.org/changeset/217484/webkit

[Rob Phillips]

Hi everybody - this bug fix has caused a dead lock race condition in ad frameworks (all being init'ed at the same time) that use WebKit to check user agent strings in the same way. This started happening for us in iOS 13.4.x but likely has been around a while.

A dead lock happens under this scenario:

- Both Frameworks A and B are initialized on a background queue in a serial order
- Framework A creates a WKWebView on the background queue without dispatching back to main thread first
- Framework B dispatches onto main thread from the background queue to create a WKWebView as well
- Both Framework A and B are now stuck in InitializeWebKit2()
- Within InitializeWebKit2(), Framework A's dispatch_sync is waiting on the main thread to be available so it can 
- Within InitializeWebKit2(), Framework B hangs the main thread while waiting for the std::call_once to finish (the same std::call_once used in Framework A's initialization)

See:

https://github.com/WebKit/webkit/blob/master/Source/WebKit/Shared/Cocoa/WebKit2InitializeCocoa.mm#L71

Please advise on whether this can be changed back to just asserting since web views should just assert if they are init'ed on anything but the main thread. From this bug report, it appears this just worked around a Safari bug a few years ago.

This advice from the comments below is sound:
>In the long run, I think we need to remove this design. Instead of wallpapering over threading problems, since this is a main-thread-only API, we should ASSERT that it's only invoked on the main thread, and fix the cases where that's not true.

Thanks!

[Chris Dumez]

(In reply to Rob Phillips from comment #11)
> Hi everybody - this bug fix has caused a dead lock race condition in ad
> frameworks (all being init'ed at the same time) that use WebKit to check
> user agent strings in the same way. This started happening for us in iOS
> 13.4.x but likely has been around a while.
> 
> A dead lock happens under this scenario:
> 
> - Both Frameworks A and B are initialized on a background queue in a serial
> order
> - Framework A creates a WKWebView on the background queue without
> dispatching back to main thread first

That's a bug and should be fixed in the framework.

> - Framework B dispatches onto main thread from the background queue to
> create a WKWebView as well
> - Both Framework A and B are now stuck in InitializeWebKit2()
> - Within InitializeWebKit2(), Framework A's dispatch_sync is waiting on the
> main thread to be available so it can 
> - Within InitializeWebKit2(), Framework B hangs the main thread while
> waiting for the std::call_once to finish (the same std::call_once used in
> Framework A's initialization)
> 
> See:
> 
> https://github.com/WebKit/webkit/blob/master/Source/WebKit/Shared/Cocoa/
> WebKit2InitializeCocoa.mm#L71
> 
> Please advise on whether this can be changed back to just asserting since
> web views should just assert if they are init'ed on anything but the main
> thread. From this bug report, it appears this just worked around a Safari
> bug a few years ago.
> 
> This advice from the comments below is sound:
> >In the long run, I think we need to remove this design. Instead of wallpapering over threading problems, since this is a main-thread-only API, we should ASSERT that it's only invoked on the main thread, and fix the cases where that's not true.
> 
> Thanks!

[Rob Phillips]

Agreed re: it being an ad framework bug; but it's also a bug in WebKit. 

Instead of asserting (which is what a lot of UIKit does now) which would allow for a proper stack trace and crash report, we instead get a dead lock / watchdog kill which we can't know about or debug unless customers send us crash reports from their device.

The proper thing to do here would be to assert in InitializeWebKit2() if not called from the main thread

[Darin Adler]

(In reply to Rob Phillips from comment #13)
> The proper thing to do here would be to assert in InitializeWebKit2() if not
> called from the main thread

An interesting claim!

[Chris Dumez]

(In reply to Darin Adler from comment #14)
> (In reply to Rob Phillips from comment #13)
> > The proper thing to do here would be to assert in InitializeWebKit2() if not
> > called from the main thread
> 
> An interesting claim!

We may be able to get away with it if we do it behind a linked-on-after check so that only apps that get rebuilt against the latest SDK would get the new behavior (CRASH if InitializeWebKit2() is called from non main thread).

[Darin Adler]

Sure. That’s what the WebCore ThreadCheck.h header is about.

WebCoreThreadViolationCheckRoundOne
WebCoreThreadViolationCheckRoundTwo
WebCoreThreadViolationCheckRoundThree

[Rob Phillips]

(In reply to Darin Adler from comment #14)
> (In reply to Rob Phillips from comment #13)
> > The proper thing to do here would be to assert in InitializeWebKit2() if not
> > called from the main thread
> 
> An interesting claim!

😄Apologies, my *preference would be. Either way, thanks for the consideration folks. I'm patching around the ad frameworks we're using for now

[Yusuke Suzuki]

This can be a dead lock if the main thread is waiting for the initialization of WebKit done in the different thread, is my understanding correct?

1. The main thread dispatches a task in a A thread and waiting for the result from that thread at some point.
2. A thread calls WebKit APIs
3. A thread posts a task to the main thread to initialize WebKit
4. dead lock

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #18)
> This can be a dead lock if the main thread is waiting for the initialization
> of WebKit done in the different thread, is my understanding correct?
> 
> 1. The main thread dispatches a task in a A thread and waiting for the
> result from that thread at some point.
> 2. A thread calls WebKit APIs
> 3. A thread posts a task to the main thread to initialize WebKit
> 4. dead lock

Ah, I was just looking at the patch, and not looking at @Rob's comment!

[Chris Dumez]

(In reply to Chris Dumez from comment #15)
> (In reply to Darin Adler from comment #14)
> > (In reply to Rob Phillips from comment #13)
> > > The proper thing to do here would be to assert in InitializeWebKit2() if not
> > > called from the main thread
> > 
> > An interesting claim!
> 
> We may be able to get away with it if we do it behind a linked-on-after
> check so that only apps that get rebuilt against the latest SDK would get
> the new behavior (CRASH if InitializeWebKit2() is called from non main
> thread).

Doing so in Bug 212283.