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

Show a GUI alert when crashing on desktop platforms #61906

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Calinou
Copy link
Member

@Calinou Calinou commented Jun 10, 2022

This makes it possible to know that Godot has crashed without looking at the console (which may not be visible to the user).

  • Change the window title while dumping the backtrace, as this can take a while to finish.
  • If file logging is enabled, print path to log file to standard error and the GUI alert.
  • Clicking OK will open the log file in the default program associated with .log files. Notepad has native associations for .log on Windows, so this will work on Windows too (once this PR is ported to Windows).

Preview

Linux

simplescreenrecorder-2022-06-10_18.24.36.mp4

Windows

image

macOS

Screenshot_20240206_195358 Screenshot_20240206_195417

TODO

  • Consider adding a headless handler for OS::alert(), so we don't have to hack around it here. Make this handler print the string with some special formatting around it, but without the blocking behavior (as this behavior is undesired in headless mode).
  • Port changes to Windows. Everything seems to work there 🙂
  • Port changes to macOS. This is now functional thanks to @bruvzg, but [macOS] Add SIGTRAP to the crash handler. #87842 needs to be merged if we want it to work with OS.crash() (the method used for testing this crash handler).
  • Redo for 3.x once everything is OK.

Production edit: closes godotengine/godot-roadmap#5

@qarmin
Copy link
Contributor

qarmin commented Jun 11, 2022

I think that there should be an option to disable this(of course if it will be enabled by default).
Not always is possible to use in CI headless build, so using normal build will just freeze entire CI job after crash

@Zireael07
Copy link
Contributor

Will this apply to Android editor port?

@Calinou
Copy link
Member Author

Calinou commented May 21, 2023

Will this apply to Android editor port?

No, as Android/iOS/HTML5 currently lack a crash handler.

@bruvzg
Copy link
Member

bruvzg commented May 22, 2023

Port changes to macOS and Windows' crash handlers.

Probably won't be straightforward to do, new windows won't show (or at least won't be responsive) since OS event loop is not running while in the crash handler.

@Calinou
Copy link
Member Author

Calinou commented Nov 18, 2023

Probably won't be straightforward to do, new windows won't show (or at least won't be responsive) since OS event loop is not running while in the crash handler.

By the way, it looks like the crash backtrace for the running project appears in the editor Output panel on macOS: #79194 (comment)

I've never seen this occur on Windows and Linux though. This could be a decent workaround for projects run from the editor if we can't make the GUI alert dialog show up on Windows and macOS.

Another alternative is to open the log file directly using OS::shell_open() on crash after the backtrace is done writing to that file.

@TheFoxKnocks
Copy link

Hey, is this currently implemented into Godot or still being worked on? I would find this very useful.

@Zireael07
Copy link
Contributor

This PR hasn't been merged so it isn't implemented in Godot yet.

@bruvzg
Copy link
Member

bruvzg commented Feb 2, 2024

To avoid issues with non-responsive or missing GUI window, I would probably do something like this instead:

  • Add a new --crash-handler crashed_process_pid log_path command line argument that will display a window (a proper UI, not OS::alert) with the info.
  • Spawn a new process as soon as crash handler is triggered (since trace generation can take a few seconds, and spawned precess can display a responsive generating backtrace message while waiting for the original process to exit).

@Calinou
Copy link
Member Author

Calinou commented Feb 6, 2024

To avoid issues with non-responsive or missing GUI window, I would probably do something like this instead:

I agree it's the way to go, but it's a lot of effort to do in a polished manner. I wouldn't be able to finish for 4.3… so maybe I should just make it open a log file on macOS and be done with it. Windows/Linux have functional GUI crash handlers already, though I don't see a way to do it for Android either (not with the suggested approach either).

Crashing without any visible GUI feedback is pretty bad, so we should do something about it as soon as possible even if the implementation isn't ideal.

@bruvzg
Copy link
Member

bruvzg commented Feb 6, 2024

As a workaround it's possible to display dialog via Apple script, shell_open also won't work outside normal app and should be replaced with script call as well.

 		fprintf(stderr, "\nFind the log file for this session at:\n%s\n\n", log_path.utf8().get_data());
 
 		if (DisplayServer::get_singleton()->get_name() != "headless") {
-			OS::get_singleton()->alert(vformat("%s: Program crashed with signal %d.\n\nFind the log file for this session at:\n%s\n\nClicking OK will open this log file. Please include the this log file's contents in bug reports.", __FUNCTION__, sig, log_path));
-			OS::get_singleton()->shell_open(log_path.utf8().get_data());
+			List<String> args;
+			args.push_back("-e");
+			args.push_back(vformat("display alert \"Crash\" message \"%s: Program crashed with signal %d.\\n\\nFind the log file for this session at:\\n%s\\n\\nClicking OK will open this log file. Please include the this log file's contents in bug reports.\"", __FUNCTION__, sig, log_path.c_escape()));
+			OS::get_singleton()->execute("osascript", args);
+			args.clear();
+			args.push_back(log_path);
+			OS::get_singleton()->execute("open", args);
 		}
 	} else if (DisplayServer::get_singleton()->get_name() != "headless") {
-		OS::get_singleton()->alert(vformat("%s: Program crashed with signal %d.", __FUNCTION__, sig));
+		List<String> args;
+		args.push_back("-e");
+		args.push_back(vformat("display alert \"Crash\" message \"%s: Program crashed with signal %d.\"", __FUNCTION__, sig));
+		OS::get_singleton()->execute("osascript", args);
 	}
 
 	// Abort to pass the error to the OS

@Calinou Calinou force-pushed the crash-handler-show-gui-alert branch from a204b63 to 3e9cf34 Compare February 6, 2024 19:18
@Calinou
Copy link
Member Author

Calinou commented Feb 6, 2024

It seems I accidentally force-pushed changes that removed Windows support last week, so I'll need to restore them before this PR can be merged.
Edit: Fixed.

As a workaround it's possible to display dialog via Apple script, shell_open also won't work outside normal app and should be replaced with script call as well.

It works great, thanks 🙂 I've added you as a co-author.

@TheFoxKnocks
Copy link

Crashing without any visible GUI feedback is pretty bad, so we should do something about it as soon as possible even if the implementation isn't ideal.

Just wanted to say I completely agree. Coming from GameMaker, the lack of a crash report popup was a surprise and has made tracking down bugs people find significantly more challenging, whereas GameMaker would point to the script where it crashed, and even if it's not always 100% accurate, it's still good guidance for tracking down the problem.

@Calinou
Copy link
Member Author

Calinou commented Feb 6, 2024

whereas GameMaker would point to the script where it crashed, and even if it's not always 100% accurate, it's still good guidance for tracking down the problem.

Note that like the existing command line crash handler, this GUI crash handler will only point you to the cause of the error if you're using a build with debug symbols. Official binaries don't include debugging symbols for binary size reasons (and they currently can't be downloaded separately).

Also, even with debug symbols, it will only point you to the C++ source of the error, not your script line. Pointing to the script line would require something like godotengine/godot-proposals#963.

@Calinou Calinou force-pushed the crash-handler-show-gui-alert branch 2 times, most recently from d48a34d to 8b29f7d Compare February 6, 2024 21:10
@Calinou Calinou marked this pull request as ready for review February 6, 2024 23:12
@Calinou Calinou requested review from a team as code owners February 6, 2024 23:12
@Riteo
Copy link
Contributor

Riteo commented Feb 6, 2024

Wowzie, this stuff is great! No idea how I missed it, sorry.

Anyways, I wonder, what happens if any of the intermediate steps causes an exception on its own? Would it just stop there, recall the handler and cause an infinite loop, or something else?

@Calinou
Copy link
Member Author

Calinou commented Feb 7, 2024

Anyways, I wonder, what happens if any of the intermediate steps causes an exception on its own? Would it just stop there, recall the handler and cause an infinite loop, or something else?

I've never seen the crash handler call itself, so I think it'll just fall back to OS behavior and exit in this case.

@bruvzg
Copy link
Member

bruvzg commented Feb 7, 2024

Anyways, I wonder, what happens if any of the intermediate steps causes an exception on its own? Would it just stop there, recall the handler and cause an infinite loop, or something else?

The first thing our crash handler do, is restoring default signal handlers. So if it crashes, it would behave as if there is no custom crash handler.

Copy link
Contributor

@kitbdev kitbdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Windows.

This doesn't show an alert in some cases. Crashing the running project with #84983 it does not show, but using #81535 it does.

Edit:
Also, clicking the X instead of the 'Ok' should not open the log file, users should not have to open it.

platform/windows/crash_handler_windows.cpp Outdated Show resolved Hide resolved
@Calinou
Copy link
Member Author

Calinou commented Mar 19, 2024

Also, clicking the X instead of the 'Ok' should not open the log file, users should not have to open it.

Unfortunately, we have no control over this because alert() returns void. It doesn't return whether the OK button was pressed or whether it was closed through other means.

Making alert() return another type would break compatibility, so a new method would need to be added.

This makes it possible to know that Godot has crashed without looking
at the console (which may not be visible to the user).

- Change the window title while dumping the backtrace, as this can
  take a while to finish.
- If file logging is enabled, print path to log file to standard error
  and the GUI alert. Clicking OK will open the log file in the default
  program associated with `.log` files. Notepad has native associations
  for `.log` on Windows, so this works on Windows too.

Co-authored-by: bruvzg <7645683+bruvzg@users.noreply.github.com>
@Calinou
Copy link
Member Author

Calinou commented Oct 4, 2024

Rebased and tested again on Linux, it works as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a GUI dialog that pops up when the engine crashes
8 participants