Bug 278452 – REGRESSION(282012@main): [GTK][WPE][Skia] Crash using threaded rendering

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Miguel Gomez]

The threaded rendering option is used by both GTK and WPE when using the skia cpu backend. This option uses DisplayLists to record the commands required to render each of the GraphicsLayer tiles, and then uses some rendering threads to replay those commands. Since 282012@main, we're getting a random crash during the replay stage of the rendering.

[Miguel Gomez]

What we're doing in both GTK and WPE is recording the commands needed to render the GraphicsLayer contents on the main thread, and then replaying them in some secondary renderer threads.

The code that does this is CoordinatedGraphicsLayer::paintTile() in CoordinatedGraphicsLayerSkia.cpp. We're creating the recording context with

auto recordingContext = makeUnique<DisplayList::DrawingContext>(tileRect.size());

And then moving that recordingContext to the renderer thread and replaying it with

recordingContext->replayDisplayList(context);


I think the problem is that non Apple platforms were always receiving nullptr when trying to get a ControlFactory, but they are now receiving the new EmptyControlFactory. But EmptyControlFactory is not a thread safe object and it's intended to be used on the main thread only, and I'm seeing calls to ControlFactory::shared() that are happening inside the rendering threads (in the Replayer constructor).

Probably just forcing GTK and WPE to return nullptr when trying to get ControlFactory would fix our problem, but I'd like to try to understand how is the feature intended to work before doing any change.

So I'm not sure what's exactly the problem here:
- Isn't DisplayList replay in threads not supported? The current implementation with the new EmptyControlFactory seems to suggest so.
- Maybe in order to replay in threads we need to use a RemoteDisplayListRecorder, which don't use the shared ControlFactory, but a new one?
- Is replaying in threads supported but this is a bug in how ControlFactory is handled? Can't ControlFactory be made thread safe? Is there any platform limitation for this?

[Aditya Keerthi]

Correct, `ControlFactory` is not thread-safe due to platform limitations. The members that a control factory holds are platform objects which themselves are not thread-safe.

Also correct that we create a `ControlFactory` per `RemoteDisplayListRecorder`, which is tied a given `RemoteRenderingBackend` thread.

You may also be able to get away with using `std::call_once` when creating the factory, as `EmptyControlFactory` doesn't actually contain any logic? However, it would be ideal to create a unique factory per thread, as intended.

[Aditya Keerthi]

282012@main itself fixed a thread safety issue where the shared factory was being used on multiple threads.

[Fujii Hironori]

282487@main introduced ControlFactoryAdwaita. GTK and WPE no longer creates EmptyControlFactory.

[Nikolas Zimmermann]

Pull request: https://github.com/WebKit/WebKit/pull/32911

[EWS]

Committed 282951@main (e46ab4de36b4): <https://commits.webkit.org/282951@main>

Reviewed commits have been landed. Closing PR #32911 and removing active labels.