Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WPE][GTK] Enable fatal criticals in WebKit subprocesses #28651

Conversation

mcatanzaro
Copy link
Contributor

@mcatanzaro mcatanzaro commented May 16, 2024

807cbc0

[WPE][GTK] Enable fatal criticals in WebKit subprocesses
https://bugs.webkit.org/show_bug.cgi?id=274255

Reviewed by NOBODY (OOPS!).

Critical warnings should only be used to indicate programmer error. By
default, they are "soft assertions" that allow program execution to
continue even though we know the process is no lonnger operating
correctly because a bug has occurred. This is a bad default. Let's crash
instead, to make it easier to debug the problem, encourage bug reports,
and downgrade any code execution issues into denial of service.

This will significantly increase crashes in our multimedia code. I think
the rest of WebKit is more or less in good state, but there will
certainly be more crashes here and there. I'm confident it will be worth
it!

We cannot do this in the UI process because that decision should be left
up to the application. Only do this for subprocesses.

* Source/WebKit/Shared/unix/AuxiliaryProcessMain.cpp:
(WebKit::ignoreSigpipe):
(WebKit::AuxiliaryProcess::platformInitialize):

807cbc0

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug βœ… πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ§ͺ api-wpe
βœ… πŸ§ͺ ios-wk2-wpt ⏳ πŸ›  wpe-skia
βœ… πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk2 βœ… πŸ›  gtk
βœ… πŸ›  tv βœ… πŸ§ͺ mac-AS-debug-wk2 βœ… πŸ§ͺ gtk-wk2
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-wk2-stress ❌ πŸ§ͺ api-gtk
βœ… πŸ›  watch
βœ… πŸ›  watch-sim

https://bugs.webkit.org/show_bug.cgi?id=274255

Reviewed by NOBODY (OOPS!).

Critical warnings should only be used to indicate programmer error. By
default, they are "soft assertions" that allow program execution to
continue even though we know the process is no lonnger operating
correctly because a bug has occurred. This is a bad default. Let's crash
instead, to make it easier to debug the problem, encourage bug reports,
and downgrade any code execution issues into denial of service.

This will significantly increase crashes in our multimedia code. I think
the rest of WebKit is more or less in good state, but there will
certainly be more crashes here and there. I'm confident it will be worth
it!

We cannot do this in the UI process because that decision should be left
up to the application. Only do this for subprocesses.

* Source/WebKit/Shared/unix/AuxiliaryProcessMain.cpp:
(WebKit::ignoreSigpipe):
(WebKit::AuxiliaryProcess::platformInitialize):
@mcatanzaro mcatanzaro requested a review from a team as a code owner May 16, 2024 13:19
@mcatanzaro mcatanzaro self-assigned this May 16, 2024
@mcatanzaro mcatanzaro added the WebKitGTK Bugs related to the Gtk API layer. label May 16, 2024
@carlosgcampos
Copy link
Contributor

Criticals can come from other glib dependencies, I don't think we want to crash our web process after a dependency update because of a critical that is not ours.

@mcatanzaro
Copy link
Contributor Author

Criticals can come from other glib dependencies,

Yes.

I don't think we want to crash our web process after a dependency update because of a critical that is not ours.

We could make criticals fatal only for our own log domain. (We actually don't currently have a log domain for WebKit? Really should define one using G_LOG_DOMAIN.)

But I'd strongly prefer to crash on bugs in our dependencies as well. E.g. The motivating case for this today was this bug and the critical there is coming from GStreamer.

@carlosgcampos
Copy link
Contributor

I don't think it's a good idea, you can get a web process crash because of other dependency and there's no way to get you browser working, even when it's fully functional, until your distro or sdk is updated.

@mcatanzaro
Copy link
Contributor Author

Well, that was my goal. If you don't want to do it, there's no point in keeping this open. I'll do it in the Epiphany web process extension instead.

@mcatanzaro mcatanzaro closed this May 17, 2024
gnomesysadmins pushed a commit to GNOME/epiphany that referenced this pull request May 17, 2024
I proposed this for WebKit in
WebKit/WebKit#28651 but Carlos is reluctant, so
let's do it only for Epiphany.

Unfortunately there's no way to block criticals in the network process
(or the future GPU process) without using the G_DEBUG environment
variable. Too bad.
gnomesysadmins pushed a commit to GNOME/epiphany that referenced this pull request May 23, 2024
I proposed this for WebKit in
WebKit/WebKit#28651 but Carlos is reluctant, so
let's do it only for Epiphany.

Unfortunately there's no way to block criticals in the network process
(or the future GPU process) without using the G_DEBUG environment
variable. Too bad.

Part-of: <https://gitlab.gnome.org/GNOME/epiphany/-/merge_requests/1488>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebKitGTK Bugs related to the Gtk API layer.
Projects
None yet
3 participants