278452 2024-08-21 00:35:46 -0700 REGRESSION(282012@main): [GTK][WPE][Skia] Crash using threaded rendering 2024-08-30 00:12:10 -0700 1 1 1 Unclassified WebKit WebKitGTK WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 magomez zimmermann akeerthi bugs-noreply cgarcia fujii mcatanzaro sabouhallawa oldest_to_newest 2054364 0 magomez 2024-08-21 00:35:46 -0700 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. 2054375 1 magomez 2024-08-21 02:16:30 -0700 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? 2054418 2 akeerthi 2024-08-21 09:01:45 -0700 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. 2054420 3 akeerthi 2024-08-21 09:03:41 -0700 282012@main itself fixed a thread safety issue where the shared factory was being used on multiple threads. 2054550 4 fujii 2024-08-21 15:49:30 -0700 282487@main introduced ControlFactoryAdwaita. GTK and WPE no longer creates EmptyControlFactory. 2056403 5 zimmermann 2024-08-29 15:23:29 -0700 Pull request: https://github.com/WebKit/WebKit/pull/32911 2056483 6 ews-feeder 2024-08-30 00:12:06 -0700 Committed 282951@main (e46ab4de36b4): <https://commits.webkit.org/282951@main> Reviewed commits have been landed. Closing PR #32911 and removing active labels.