Skip to content

frontend: Add automatic crash log upload after crashes for Windows and macOS #11943

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

PatTheMav
Copy link
Member

@PatTheMav PatTheMav commented Mar 10, 2025

Description

This PR refactors the current safe mode implementation to allow automatic uploading of the most recent crash report upon a detected crash while avoiding duplicate uploads and also enables the feature on macOS.

Motivation and Context

The primary ask for this work was to enable the automatic upload of crash reports after OBS Studio has crashed and also enable this functionality to more platforms than just Windows.

Note

An even better solution towards this goal would be to use separate minimal process that uses platform-specific methods to "attach" to the main process and be able to detect and react to a crash and thus offer to relaunch the application and uploading a crash log from within it itself. That kind of solution was out of scope for this first iterative step.

To that end the following changes have been implemented:

  • Crash mode detection and crash log uploading have been moved out of the application code base into their own separate CrashHandler class.
  • Direct interaction between CrashHandler, OBSApp, and OBSBasic has been decoupled and has been replaced by Qt signals which code can connect to and react accordingly
  • Each crash sentinel now represents a unique application launch, which potentially allows the CrashHandler class to distinguish between a "live" sentinel for the currently running instance versus an "old" sentinel, which would represent an unclean prior shutdown
  • The most recent crash log file and corresponding upload URL are retained in the application config to avoid multiple uploads of the same crash log.
    • When the most recent crash log file has not changed, the "Upload" menu action will just yield the prior URL.
    • This also allows users to yield the URL of an automatic crash log upload
  • When an unclean shutdown (or "crash") is detected, the user is still provided with a choice to run OBS Studio in "Safe Mode". An additional checkbox to automatically upload the most recent crash is also provided. The automatic upload is opt-in by default.

Platform differences

On Windows the existing custom crash log files are checked and used for crash log upload.

Changes: The CrashHandler class will look for the most recent file by actual creation date and not simply by the time code in its file name anymore.

On macOS the diagnostics reports generated by the operating system are used. No custom crash files are involved. As macOS "retires" active crash reports after some time, the subdirectory for retired reports is also taken into account when looking for a "most recent" crash, with the file creation date used for sorting the results.

On Linux and FreeBSD no searches for crash reports take place, the class methods are implemented as stubs to ensure the class implementation is complete on these platforms.

How Has This Been Tested?

Created custom crash logs in the locations used for macOS and Windows and checked that most recent crashes are found and uploaded from these locations accordingly.

  • Checked behaviour when a new crash file was found (file was uploaded, new URL yielded)
  • Checked behaviour when no new crash file was found but URL was empty (file was uploaded, new URL was displayed to user)
  • Checked behaviour when file upload was unsuccessful (error message was displayed)
  • Checked behaviour when file upload was successful but response JSON does not contain url field (error message was displayed)
  • Checked behaviour when no new crash file was found with available URL (no upload took place, old URL was displayed to user)
  • Checked behaviour when crash sentinel with old UUID was placed in OBS Studio settings directory (prior crash was "detected", safe mode choice was presented)
  • Checked behaviour when automatic crash upload was selected in safe mode dialog (crash log was uploaded, no URL dialog was displayed)
  • Checked behaviour when crash upload was selected after automatic crash upload (existing crash log URL was displayed to user)
  • Checked behaviour when application was compiled in Debug configuration (no safe mode dialogs appeared, crash log behaviour was unchanged)

Types of changes

  • New feature (non-breaking change which adds functionality)
  • Code cleanup (non-breaking change which makes code smaller or more readable)

Checklist:

  • My code has been run through clang-format.
  • I have read the contributing document.
  • My code is not on the master branch.
  • The code has been tested.
  • All commit messages are properly formatted and commits squashed where appropriate.
  • I have included updates to all appropriate documentation.

@PatTheMav PatTheMav force-pushed the macos-crash-reporting branch 2 times, most recently from 11f0370 to e952d18 Compare March 10, 2025 20:01
@jcm93

This comment was marked as outdated.

@PatTheMav
Copy link
Member Author

After checking this box, I also do not seem to get a popup showing the URL of the uploaded report like I do if I manually select in the menu "Help->Crash Reports->Upload Previous Crash Report". Is this expected or a bug?

No that's intended - the requirement was that no further popups should be displayed to the user as we already have too many of those. Using the menu entry to get at that same URL was the proposed solution.

@jcm93
Copy link
Contributor

jcm93 commented Mar 10, 2025

So, if I check that box, and then I navigate to the aforementioned help menu item (Help->Crash Reports->Upload Previous Crash Report), does that not upload the same report twice? Or do we detect that the report was already uploaded and just show that URL?

Edit: I see this is covered in the original description; duplicate uploads don't happen if the 'most recent detected crash log' has not changed.

I do think it's a bit weird to have the redundancy of the checkbox if, no matter what, the Help menu is still required to yield the URL of the uploaded log.

@PatTheMav
Copy link
Member Author

I do think it's a bit weird to have the redundancy of the checkbox if, no matter what, the Help menu is still required to yield the URL of the uploaded log.

As I said, that's by design, maybe @Warchamp7 can chime in here as that was the solution we ended up with here.

The basic idea is to lower the barrier for crash reports to be uploaded, particularly as this would allow automatic analysis of "current crashes" server-side without the manual input by users.

@jcm93
Copy link
Contributor

jcm93 commented Mar 11, 2025

Got it; yeah, if part of the idea here is 'eventual server-side processing' then I think that makes sense enough to me.

@PatTheMav PatTheMav force-pushed the macos-crash-reporting branch from e952d18 to cc670f9 Compare March 11, 2025 17:06
@PatTheMav
Copy link
Member Author

Removed the intro text for every upload on macOS to upload the unchanged JSON contents of the crash log to the server. That way the file can be downloaded and parsed/opened directly (albeit with a possible change of file suffix) in Xcode or Console.

@PatTheMav
Copy link
Member Author

Added an experimental change for testers:

Instead of relying on the class destructors, both OBSApp's and CrashHandler's clean up code is now triggered by the aboutToQuit signal available to all sub-classes of QCoreApplication.

Per Qt, slots connected to signals are executed in the order they are added, so adding OBSApp's own handler first (and before constructing the CrashHandler) ensures that the application cleanup and libobos shutdown happen first, followed by the crash handler's cleanup and any crashes introduced by plugins will potentially leave the sentinel be.

@PatTheMav PatTheMav force-pushed the macos-crash-reporting branch 2 times, most recently from 63a6ee9 to 0de9c34 Compare March 13, 2025 18:32
@RytoEX RytoEX added the Seeking Testers Build artifacts on CI label Mar 13, 2025
@RytoEX
Copy link
Member

RytoEX commented Mar 13, 2025

I haven't had a chance to review this, but I might eventually prefer the migration to aboutToQuit in a separate PR, unless there is merit/value in keeping them together here.

@PatTheMav
Copy link
Member Author

I haven't had a chance to review this, but I might eventually prefer the migration to aboutToQuit in a separate PR, unless there is merit/value in keeping them together here.

Both have to be added at the same time. Otherwise plugin shutdown happens after the CrashHandler has been shut down and thus any 3rd party plugin could introduce an unclean shutdown and thus we could not provide the "safe mode" as a fix against that misbehaving 3rd party plugin.

So it's either both or none of the changes.

@WizardCM WizardCM added Windows Affects Windows macOS Affects macOS labels Mar 16, 2025
Copy link
Member

@Warchamp7 Warchamp7 left a comment

Choose a reason for hiding this comment

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

Looks good from my testing. One minor nit regarding wording.

Also discussed with Pat off-thread of having the upload checkbox hidden when the crash report in question has been previously uploaded.

@PatTheMav PatTheMav force-pushed the macos-crash-reporting branch 4 times, most recently from 8fe543c to c4505e1 Compare March 31, 2025 14:26
@PatTheMav
Copy link
Member Author

Made the following substantial changes based on feedback:

  • Log uploads (application logs as well as crash logs) now present a confirmation dialog with a link to the privacy policy first
  • Application log uploads are now hooked-up using the same events-based system as crash log uploads
  • Each commit is atomic, the project can be compiled without issue even with only the intermediate commits checked out. Only the final commit implements the actual changes.

The CrashHandler class encapsulates all functionality around unsafe
shutdown detection as well as crash log discovery and log upload.

Each (functional) CrashHandler should be initialized with an app launch
UUID, which enables the handler to disambiguate a sentinel file for the
current app launch from those of prior app launches.
This is an extended variant of the prior OBSLogReply class. The major
change is that the dialog encapsulates the interface workflow for log
uploads, including the confirmation for an upload and handling
successful or unsuccessful uploads.
Updates include:

* Use of CrashHandler to provide automatic uploads of the most recent
crash log if an unclean shutdown was detected and it has not been
uploaded yet.
* Detection and handling of unclean shutdowns is delegated entirely to
the CrashHandler class
* Use of OBSLogReply has been replaced with the LogUploadDialog, which
asks for confirmation before new uploads of log files (confirmation is
skipped for files with available upload URLs already - only available
for crash logs with this change)

Architectural changes:
* OBSApp is the layer responsible for application launch and shutdown
states, as well as crash logs and application logs
* The actual handling is delegated to purpose-made classes which OBSApp
owns instances of
* OBSBasic in turn refers to OBSApp for all this functionality, and can
subscribe/connect to appropriate events exposed by OBSApp to this
purpose
* Implementation details (like the existence of the CrashHandler class)
are not exposed to OBSBasic or the LogUploadDialog

The amount of changes for normal log file upload have been purposefully
limited. A proper refactoring of the application log file handling will
move this code out of OBSBasic as well.
@PatTheMav PatTheMav force-pushed the macos-crash-reporting branch from c4505e1 to 290bc5a Compare March 31, 2025 14:52
@RytoEX RytoEX requested a review from Warchamp7 April 28, 2025 21:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
macOS Affects macOS Seeking Testers Build artifacts on CI Windows Affects Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants