panel: Update browser shutdown code to ensure proper sequence of events #506

Merged

PatTheMav merged 1 commit from browser-close-fix into master

2025-10-17 07:00:45 -05:00

PatTheMav commented

2025-10-10 14:23:08 -05:00

(Migrated from github.com)

Description

Rewrites the browser shutdown code to be more explicit about its functionality and the sequence of events that should take place in a serial order to ensure that the browser process is properly shut down before the associated Qt widgetry is destroyed.

Motivation and Context

When the underlying CEF browser instance is required to close (either because the widget itself is being destroyed or because the associated function was called explicitly) a specific sequence of events needs to be ensured and also needs to block the thread it was called on.

This is necessary to ensure that CEF's own shutdown code can run to completion before Qt continues with its own widget destruction and cleanup code.

This change splits the procedure into 3 separate steps, each scheduled to run in-order on the local event loop to simulate a "serial dispatch queue":

Detach the window used by CEF from the Qt view hierarchy to prevent CEF from closing the main OBS Studio window
Initiate the actual browser closure
Reset the Qt widget reference kept by the browser client and quit the local event loop.

Step 3 is either executed after the "OnBeforeClose" lifetime event is signaled by CEF or automatically after 1000 ms to limit the maximum of time the UI is allowed to keep the user waiting.

The wrong order of events can lead to race conditions and has already led to obscure crashes that occur because the state of the underlying objects while the closeBrowser function is executed have been in semi-invalid states and pointers have become invalid seemingly at random on users' machines.

How Has This Been Tested?

Tested on Windows and macOS, successfully establishing service connections and aborting the attempt to establish one, checking that the associated OAuth panels are correctly closed up.

Also checked with custom browser panels.

This change also hardens the code responsible to remove the NSView from the window hierarchy before the browser is closed by explicitly checking if the instance the ObjC messages are being sent to still has the required isa pointer to make it an ObjC class instance.

The associated code has been moved into a separate function as it should be implemented in a platform-specific ObjC file instead (so ObjC functions can be called directly) instead of being interweaved with code for other platforms via preprocessor conditions.

Types of changes

Bug fix (non-breaking change which fixes an issue)
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.

### Description Rewrites the browser shutdown code to be more explicit about its functionality and the sequence of events that should take place in a serial order to ensure that the browser process is properly shut down before the associated Qt widgetry is destroyed. ### Motivation and Context When the underlying CEF browser instance is required to close (either because the widget itself is being destroyed or because the associated function was called explicitly) a specific sequence of events needs to be ensured and also needs to block the thread it was called on. This is necessary to ensure that CEF's own shutdown code can run to completion before Qt continues with its own widget destruction and cleanup code. This change splits the procedure into 3 separate steps, each scheduled to run in-order on the local event loop to simulate a "serial dispatch queue": 1. Detach the window used by CEF from the Qt view hierarchy to prevent CEF from closing the main OBS Studio window 2. Initiate the actual browser closure 3. Reset the Qt widget reference kept by the browser client and quit the local event loop. Step 3 is either executed after the "OnBeforeClose" lifetime event is signaled by CEF or automatically after 1000 ms to limit the maximum of time the UI is allowed to keep the user waiting. The wrong order of events can lead to race conditions and has already led to obscure crashes that occur because the state of the underlying objects while the `closeBrowser` function is executed have been in semi-invalid states and pointers have become invalid seemingly at random on users' machines. ### How Has This Been Tested? Tested on Windows and macOS, successfully establishing service connections and aborting the attempt to establish one, checking that the associated OAuth panels are correctly closed up. Also checked with custom browser panels. This change also hardens the code responsible to remove the `NSView` from the window hierarchy before the browser is closed by explicitly checking if the instance the ObjC messages are being sent to still has the required `isa` pointer to make it an ObjC class instance. The associated code has been moved into a separate function as it should be implemented in a platform-specific ObjC file instead (so ObjC functions can be called directly) instead of being interweaved with code for other platforms via preprocessor conditions. ### Types of changes - Bug fix (non-breaking change which fixes an issue) - Code cleanup (non-breaking change which makes code smaller or more readable) ### Checklist: - [x] My code has been run through [clang-format](https://github.com/obsproject/obs-studio/blob/master/.clang-format). - [x] I have read the [**contributing** document](https://github.com/obsproject/obs-studio/blob/master/CONTRIBUTING.rst). - [x] My code is not on the master branch. - [x] The code has been tested. - [x] All commit messages are properly formatted and commits squashed where appropriate. - [x] I have included updates to all appropriate documentation.

👍 2

PatTheMav commented

2025-10-11 07:11:39 -05:00

(Migrated from github.com)

Replaced the reinterpret_cast used by original code with the safer, and more correct, static_cast variant.

While dynamic_cast would be the absolutely correct choice as it will just return a nullptr if the pointer to the base class cannot correctly be downcast to the derived class, there is currently no way the pointer returned by host->GetClient() in this part of the code will return a client that is not a QCefBrowserClient. Thus static_cast is good-enough.

Replaced the `reinterpret_cast` used by original code with the safer, and more correct, `static_cast` variant. While `dynamic_cast` would be the absolutely correct choice as it will just return a `nullptr` if the pointer to the base class cannot correctly be downcast to the derived class, there is currently no way the pointer returned by `host->GetClient()` in this part of the code will return a client that is not a `QCefBrowserClient`. Thus `static_cast` is good-enough.

No reviewers

No milestone

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference

gh-obsproject/obs-browser!506

No description provided.

Rows
Columns