Do not try to create the browser before init#519
Do not try to create the browser before init#519hoshinolina wants to merge 1 commit intoobsproject:masterfrom
Conversation
|
What's the actual root cause of the race? At first glance the "browser manager thread" seems like a culprit, because it decouples the actual initialisation of CEF from the main thread on Windows and Linux (but not on macOS), so CEF will only be initialised "eventually". On macOS |
|
Yes, it's that. In particular, the graphics thread will call tick on the source once it's added to the scene, and if CEF init isn't complete yet by then bad things happen.
|
PatTheMav
left a comment
PatTheMav
left a comment
There was a problem hiding this comment.
Annoying that it's necessary, but seems like the most elegant fix for this race condition.
Please amend the commit description per our contribution guidelines to explain the "why" of the fix (explaining the race condition, it's root cause, and why this fixes it).
CEF is initialized in a browser manager thread on Windows and Linux, which happens asynchronously. The graphics tick function may be called before CEF initialization completes, which leads to asserts and other issues. Ensure this does not happen by checking for CEF init completion.
hoshinolina
force-pushed
the
fix-init-race
branch
from
8bea62a to
ec4be1a
Compare
|
Done. |
| if (os_event_try(cef_started_event) != 0) | ||
| return; |
There was a problem hiding this comment.
| if (os_event_try(cef_started_event) != 0) | |
| return; | |
| if (os_event_try(cef_started_event) != 0) { | |
| return; | |
| } |
Sorry, should've picked up on this in my earlier preview. Per our code style guidelines, except for C code, we prefer all conditional and loop blocks to be explicitly enclosed with curlies.
There was a problem hiding this comment.
I mean... sure but... nothing else in the file does this, including the rest of the function.
I think if this is desired, someone should add InsertBraces: true to .clang-format and fix the whole codebase. Enforcing things like this in review is essentially a waste of everyone's time when clang-format can do it for you.
6 participants
Description
Should fix the random asserts on startup (not related to the recent consistent startup failures), I think.
Motivation and Context
Fixes a race.
How Has This Been Tested?
Quick smoke test.
Types of changes
Checklist: