Skip to content

frontend: Skip saveAll after closeWindow#13346

Draft
umireon wants to merge 2 commits intoobsproject:masterfrom
umireon:umireon/guard-for-saveall-on-commitdata
Draft

frontend: Skip saveAll after closeWindow#13346
umireon wants to merge 2 commits intoobsproject:masterfrom
umireon:umireon/guard-for-saveall-on-commitdata

Conversation

@umireon
Copy link
Copy Markdown

@umireon umireon commented Apr 21, 2026

Description

On the exit of OBS frontend, commitData can be called after closeWindow. Since closeWindow releases OBSBasic's resources, saveAll on commitData may write corrupted configuration.

This change adds a guard to prevent commitData from saving all configuration when the instance of OBSBasic is already released. This is safe because closeWindow is also responsible for saveAll.

Motivation and Context

Creating a profile, connecting to YouTube, and closing the OBS frontend immediately results in removing Auth.Type in the profile's basic.ini. This is problematic for users because they cannot persist their connection with some specific procedure.

How Has This Been Tested?

I have built it on my head branch on macOS Tahoe (arm64). I confirmed that create, connect, close flow saves correct configuration, and it is fixed to persist YouTube connection between runs.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

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

On the exit of OBS frontend, commitData can be called after
closeWindow. Since closeWindow releases OBSBasic's resources, saveAll
on commitData may write corrupted configuration.

For instance, creating a profile, connecting to YouTube, and closing
the OBS frontend immediately results in removing Auth.Type in
the profile's basic.ini. This is problematic for users because they
cannot persist their connection with some specific procedure.

This change adds a guard to prevent commitData from saving
all configuration when the instance of OBSBasic is already released.
This is safe because closeWindow is also responsible for saveAll.

Signed-off-by: Kaito Udagawa <umireon@kaito.tokyo>
Copilot AI review requested due to automatic review settings April 21, 2026 00:32

This comment was marked as spam.

@Fenrirthviti
Copy link
Copy Markdown
Member

Please do not request AI reviews on our repo.

This calls in to question that this change was also AI-generated. Can you please confirm that this PR is not AI-assisted?

@umireon
Copy link
Copy Markdown
Author

umireon commented Apr 21, 2026

@Fenrirthviti I'm sorry, my user profile on GitHub is misconfigured and AI review is automatically requested. I've just turned off that settings.

This calls in to question that this change was also AI-generated. Can you please confirm that this PR is not AI-assisted?

I confirm that the whole content of this Pull Request including code, PR description, and commit message are written by me and contains no AI-generated contents.

I apologize to behave in the way not aligning the project policy, but this is not intentional and I won't do this again.

Thanks,

Copy link
Copy Markdown
Member

@notr1ch notr1ch left a comment

Choose a reason for hiding this comment

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

Makes sense to me.

@PatTheMav
Copy link
Copy Markdown
Member

PatTheMav commented Apr 27, 2026

The issue is correctly identified, mainly that the duplicate call to saveAll at different points during the shutdown corrupts data, but the fix actually makes matters worse.

Any saving of user data or application state should take place in commitData, because that's the intended event for this per Qt's application architecture. The event is triggered by the session manager which is automatically invoked when an application receives a termination request, which also allows a user to cancel the termination.

Only when the user does not cancel it (or the application does not intend the user to have a say in this) is a Quit event triggered, which then takes care of closing all windows.

So what happens on macOS is that commitData correctly saved the application state, but then at some non-deterministic point during shutdown closeWindow will overwrite this correct data with incomplete data.

The core issue here is that OBS treats the main window as "the application" and ties up a whole lot other state to its lifetime even though that's not how Qt is designed to handle it rather than simply storing its internal window state before simply emitting the Quit event and letting the application handle saving application state.


So what happens currently is that when you:

  • Quit OBS via the menu bar item
  • Quit OBS via the CMD+Q shortcut
  • Quit OBS via the dock
  • Let macOS quit OBS automatically due to a shutdown/restart or other event

Then closeWindow will corrupt the data.

Only if you quit OBS by closing the main window will closeWindow potentially save correct data and commitData might potentially write incomplete data.

Indeed it's the design of closeWindow that's the problem here, because all other "native" shutdown variants would achieve the right thing.

@PatTheMav
Copy link
Copy Markdown
Member

@umireon Would you be willing to test a different "hypothesis" if you will?

@Warchamp7 and I discussed the different wrinkles of shutdown on Windows and macOS and the "trigger" for saveAll will be different:

  • On macOS it will be triggered by commitData by anything except for closing the main window.
  • On Windows it would potentially only be triggered by commitData when the OS is shutting down, otherwise closeWindow on OBSBasic will usually be the trigger.

My hypothesis is that we only want one trigger to succeed, but it also needs to be the first caller that succeeds. So we could potentially add a guard inside saveAll to bail out early if data was saved once before.

If you could implement that and test it with your use case (as you got the environment in which it goes "wrong" available already), that'd be very helpful.

@umireon umireon marked this pull request as draft April 28, 2026 22:33
@umireon
Copy link
Copy Markdown
Author

umireon commented Apr 28, 2026

@PatTheMav Thank you for your great feedback! I have both a macOS and Windows environment and I can implement and verify your idea works. Once I finish, I will push and make this PR ready.

@WizardCM WizardCM added the kind/bug Categorizes issue or PR as related to a bug. label May 1, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind/bug Categorizes issue or PR as related to a bug.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants