WIP: Windows: Show popup after crash #129974

Germano Cavalcante · 2024-11-07T15:45:06+01:00

2024-11-07 15:45:06 +01:00

Implements a crash dialog for Windows, showing what was discussed in
#128621

Implements a crash dialog for Windows, showing what was discussed in #128621 --- <img width="346" alt="image" src="attachments/142227f6-3c83-41ce-8e93-4e0669daece1">

image.png

18 KiB

❤️ 1 🎉 4

Germano Cavalcante added 1 commit 2024-11-07 15:45:15 +01:00

Windows: Show popup after crash 40be2fddd3

Germano Cavalcante changed title from ~~Windows: Show popup after crash~~ to WIP: Windows: Show popup after crash

2024-11-07 15:45:29 +01:00

Germano Cavalcante requested review from Ray molenkamp 2024-11-07 15:45:41 +01:00

Ray molenkamp requested changes 2024-11-07 17:08:23 +01:00

source/blender/blenlib/BLI_system.h Outdated

						
				@ -48,2 +48,3 @@

				 */

				void BLI_windows_handle_exception(void *exception);

				void BLI_windows_exception_capture(void *exception);

				void bli_windows_exception_message_get(char r_message[512]);

Ray molenkamp commented

2024-11-07 16:42:53 +01:00

lower case functions do not go in public headers they are reserved for static local functions.

mano-wii marked this conversation as resolved

source/blender/blenlib/CMakeLists.txt Outdated

						
				@ -479,6 +479,7 @@ if(WIN32)

				  )

				  list(APPEND SRC

				    intern/system_win32.c

				    intern/system_win32.cc

Ray molenkamp commented

2024-11-07 16:43:59 +01:00

best to convert system_win32.c to C++ in a separate commit/PR.

👍 1

Germano Cavalcante commented

2024-11-07 17:49:59 +01:00

Well, that's one point, in this case I created the two files because, for some reason, it was not possible to merge them into just system_win32.cc. There are linking errors. I could convert system_win32.c to C++, but the new functions do not get along well if added to the same file.

Well, that's one point, in this case I created the two files because, for some reason, it was not possible to merge them into just `system_win32.cc`. There are linking errors. I could convert `system_win32.c` to C++, but the new functions do not get along well if added to the same file.

mano-wii marked this conversation as resolved

source/blender/blenlib/intern/system_win32.c Outdated

						
				@ -384,6 +384,7 @@ void BLI_system_backtrace(FILE *fp)

				  SymInitialize(GetCurrentProcess(), NULL, TRUE);

				  bli_load_symbols();

				  if (current_exception) {

				    /* `BLI_windows_exception_log_start` should have been called before. */

Ray molenkamp commented

2024-11-07 16:44:24 +01:00

BLI_windows_exception_log_start doesn't exist? you likely mean BLI_windows_exception_capture?

`BLI_windows_exception_log_start` doesn't exist? you likely mean `BLI_windows_exception_capture`?

mano-wii marked this conversation as resolved

source/blender/blenlib/intern/system_win32.c Outdated

						
				@ -395,3 +396,3 @@

				}

				void BLI_windows_handle_exception(EXCEPTION_POINTERS *exception)

				void BLI_windows_exception_capture(void *exception)

Ray molenkamp commented

2024-11-07 16:37:57 +01:00

Not a fan of this pattern, it gives the opportunity of use after frees that normally wouldn't happen if you just passed the exception as an argument to the functions that require it. Also having a function called bli_windows_exception_message_get not have an exception as input parameter is really strange.

Not a fan of this pattern, it gives the opportunity of use after frees that normally wouldn't happen if you just passed the exception as an argument to the functions that require it. Also having a function called `bli_windows_exception_message_get` not have an exception as input parameter is _really_ strange.

mano-wii marked this conversation as resolved

source/blender/blenlib/intern/system_win32.c Outdated

						
				@ -414,1 +401,3 @@

				  fflush(stderr);

				}

				void bli_windows_exception_message_get(char r_message[512])

Ray molenkamp commented

2024-11-07 16:40:32 +01:00

lower case bli_ prefix, but not a static function

lower case `bli_` prefix, but not a static function

mano-wii marked this conversation as resolved

source/blender/blenlib/intern/system_win32.c Outdated

						
				@ -415,0 +403,4 @@

				void bli_windows_exception_message_get(char r_message[512])

				{

				  if (!current_exception) {

				    /* `BLI_windows_exception_log_start` should have been called before. */

Ray molenkamp commented

2024-11-07 16:38:23 +01:00

BLI_windows_exception_log_start doesn't exist? you likely mean BLI_windows_exception_capture?

`BLI_windows_exception_log_start` doesn't exist? you likely mean `BLI_windows_exception_capture`?

mano-wii marked this conversation as resolved

source/blender/blenlib/intern/system_win32.c Outdated

						
				@ -415,0 +417,4 @@

				  snprintf(r_message,

				           512,

				           "Error   : %s\n"

Ray molenkamp commented

2024-11-07 16:51:47 +01:00

This is more of a UI issue, who is this data for?

If it's for us developers, i'd probably want the ExceptionFlags and any Parameters to be included.

If it's for the end user, there's no need to have the Address and TreadID fields here, as there's literally nothing useful the user can do with that information.

This is more of a UI issue, who is this data for? If it's for us developers, i'd probably want the ExceptionFlags and any Parameters to be included. If it's for the end user, there's no need to have the Address and TreadID fields here, as there's literally nothing useful the user can do with that information.

Germano Cavalcante commented

2024-11-09 01:07:13 +01:00

I decided to remove this information from the popup. It wasn't part of the mockup. It was an artistic liberty but it ended up looking ugly.

mano-wii marked this conversation as resolved

source/blender/blenlib/intern/system_win32.cc Outdated

						
				@ -0,0 +19,4 @@

				/** \name showMessageBox

				 * \{ */

				static std::string url_encode(const char *str)

Ray molenkamp commented

2024-11-07 16:53:03 +01:00

This should probably live elsewhere, seems like a useful function outside of system_win32 if we're keeping it here, why not just call the UrlEscape API? input a const char*, output a string feels strange

This should probably live elsewhere, seems like a useful function outside of `system_win32` if we're keeping it here, why not just call the `UrlEscape` API? input a const char*, output a string feels strange

Germano Cavalcante commented

2024-11-11 13:42:23 +01:00

...why not just call the UrlEscape API?

I don't know what I'm doing wrong, but it's not working:

static std::string url_encode(const std::string &str)
{
  /* Query required buffer size. */
  DWORD size = 0;
  UrlEscapeA(str.c_str(), nullptr, &size, URL_ESCAPE_PERCENT);

  /* Perform actual URL encoding. */
  std::vector<char> buffer(size + 1);
  UrlEscapeA(str.c_str(), buffer.data(), &size, URL_ESCAPE_PERCENT);
  return std::string(buffer.data());
}

This code is a bit difficult to debug, so I didn't investigate it, but even so, I have a slight preference for a solution that doesn't use the Windows API since perhaps this code could be exposed later in some BLI header.

> ...why not just call the `UrlEscape` API? I don't know what I'm doing wrong, but it's not working: ```cpp static std::string url_encode(const std::string &str) { /* Query required buffer size. */ DWORD size = 0; UrlEscapeA(str.c_str(), nullptr, &size, URL_ESCAPE_PERCENT); /* Perform actual URL encoding. */ std::vector<char> buffer(size + 1); UrlEscapeA(str.c_str(), buffer.data(), &size, URL_ESCAPE_PERCENT); return std::string(buffer.data()); } ``` This code is a bit difficult to debug, so I didn't investigate it, but even so, I have a slight preference for a solution that doesn't use the Windows API since perhaps this code could be exposed later in some BLI header.

source/blender/blenlib/intern/system_win32.cc Outdated

						
				@ -0,0 +85,4 @@

				 * Displays a crash popup with options to open the crash log and report a bug.

				 * This is based on the `showMessageBox` function in `GHOST_SystemWin32.cc`.

				 */

				static void showMessageBox(const char *message,

Ray molenkamp commented

2024-11-07 17:00:05 +01:00

missing bli_ prefix

missing `bli_` prefix

mano-wii marked this conversation as resolved

source/creator/creator_signals.cc

						
				@ -82,3 +85,3 @@

				}

				static void sig_handle_crash_backtrace(FILE *fp)

				static void crashlog_filepath_get(char filepath[FILE_MAX])

Ray molenkamp commented

2024-11-07 17:05:26 +01:00

Not convinced this belongs in this file, all other methods are either MAIN_ or sig_ this feels a bit out of place? @ideasman42 do you have any strong feelings about this?

Not convinced this belongs in this file, all other methods are either `MAIN_` or `sig_` this feels a bit out of place? @ideasman42 do you have any strong feelings about this?

source/creator/creator_signals.cc Outdated

						
				@ -147,2 +148,4 @@

				  }

				}

				static void signal_cleanup_and_terminate(int signum)

Ray molenkamp commented

2024-11-07 17:01:37 +01:00

all other code around here uses sig_

all other code around here uses `sig_`

mano-wii marked this conversation as resolved

Ray molenkamp requested review from Harley Acheson 2024-11-07 17:08:53 +01:00

Ray molenkamp commented

2024-11-07 17:09:17 +01:00

I'd like @Harley to sign off on the user facing changes, so added him as reviewer.

Germano Cavalcante added 4 commits 2024-11-09 01:04:12 +01:00

Merge branch 'main' into crash_popup c63226bf05

Avoid using the global variable current_exception 7337fa428d

Merge remote-tracking branch 'origin/main' into crash_popup ec1a07b172

Load last saved blend file if crash 67d48796c1

Germano Cavalcante added 1 commit 2024-11-09 01:10:19 +01:00

Cleanup rename functions 79ce98401d

Germano Cavalcante added 1 commit 2024-11-09 01:16:25 +01:00

Avoid reopening a .blend file that was not saved during the session. This can cause an infinite loop if the crash occurs when opening Blender.

buildbot/vexp-code-patch-lint Build done.

Details

buildbot/vexp-code-patch-darwin-x86_64 Build done.

Details

buildbot/vexp-code-patch-linux-x86_64 Build done.

Details

buildbot/vexp-code-patch-darwin-arm64 Build done.

Details

buildbot/vexp-code-patch-windows-amd64 Build done.

Details

buildbot/vexp-code-patch-coordinator Build done.

Details

4b141088e7

Germano Cavalcante commented

2024-11-09 01:23:43 +01:00

I implemented the solution of opening the last saved .blend. (For this I just stored the directory in G.filepath_last_blend whenever a .blend file is written for autosave or for save).

(This is not something that has been discussed much, it can be reversed or postponed if fells unsafe).

@blender-bot build

I implemented the solution of opening the last saved .blend. (For this I just stored the directory in `G.filepath_last_blend` whenever a .blend file is written for autosave or for save). (This is not something that has been discussed much, it can be reversed or postponed if fells unsafe). @blender-bot build

Ray molenkamp commented

2024-11-09 01:31:56 +01:00

I'll leave that to the UI team, at face value, it is a little strange that a button labelled "close" would open a new blender

Jesse Yurkovich commented

2024-11-09 01:48:05 +01:00

I think you're going to have to process the filepath differently, converting directly to wstring like that won't work. This is what results when crashing a file called こんにちは.blend :

I think you'll need to the UTF16_ENCODE and UTF16_UN_ENCODE macros.

I think you're going to have to process the filepath differently, converting directly to `wstring` like that won't work. This is what results when crashing a file called `こんにちは.blend` : <img width="275" alt="image" src="attachments/8f98925d-2ce4-4f90-9a23-85c8c72deeaa"> I think you'll need to the `UTF16_ENCODE` and `UTF16_UN_ENCODE` macros.

image.png

11 KiB

Germano Cavalcante added 4 commits 2024-11-11 13:38:19 +01:00

Cleanup: use ELEM 33e8daef96

Merge remote-tracking branch 'origin/main' into crash_popup 69b225185e

Fix non-ASCII characters case 42e05ccd6a

Use string instead char* in url_encode 4cc56d8d0b

Germano Cavalcante commented

2024-11-11 13:43:56 +01:00

Well noted @deadpin, fixed :)
(I preferred to use the same solution seen in the original showMessageBox).

Well noted @deadpin, fixed :) (I preferred to use the same solution seen in the original `showMessageBox`).

This pull request is marked as a work in progress.