Enable support for CEF 6613+ (Chrome 128 to 147) and Chrome Runtime#523
Enable support for CEF 6613+ (Chrome 128 to 147) and Chrome Runtime#523WizardCM wants to merge 1 commit into
Conversation
f4f7da9 to
6087ad4
Compare
|
For reference, the only issues we've run into in Fedora as far as I remember have been:
So overall, nothing really relevant to this PR. That said, I should point out that Fedora never shipped nor tested the profile migration code. While Fedora did briefly ship the old obs-cef at one point in the past, that was abandoned as unmaintainable (making it build with newer compilers/libs became a nightmare), so there was a period where OBS had no CEF support in Fedora before this was reintroduced, and therefore no good reason to worry about profile migrations (it would only benefit people who had used the old version, to revive their old dormant profiles, which we figured wasn't really worth it). If anyone wants to build this PR in Fedora against system CEF, the only required changes should be:
Note that unlike the standard OBS build, this setup embeds the |
| { | ||
| std::string path = storage_path; | ||
| #if CHROME_VERSION_BUILD > 6533 | ||
| // This code may be temporary, ideally the change should be in OBS Studio itself, |
There was a problem hiding this comment.
Create a task issue and add a TODO comment with a link to that issue here (outlining the issue and what needs to be changed in that issue).
|
|
||
| static void MigrateToChromeRuntime() | ||
| { | ||
| BPtr<char> defaultProfile = obs_module_config_path("Default"); |
There was a problem hiding this comment.
This is entirely new C++ code, so I'd prefer if we'd use standard C++ library types and functions if available (e.g. std::string, std::string_view, std::filesystem, etc.) rather than custom OBS types and APIs and treating it as "C with some OOP".
(And even though std is included by default, this would be a violation of our C++ code style standards, so use std::string explicitly so this code does not need to be touched when using namespace std hopefully is excised at a later point).
Also be mindful of any libobs API that potentially returns nullptr instead of an actual char * and guard against that.
There was a problem hiding this comment.
Same question as above for how best to transition from obs_module_config_path here, as I don't think we have any existing examples in the OBS codebase.
Unfortunately, MigrateProfileConfigDir is used both for migrating the Default profile, and later to migrate individual Profile cookie directories, so it has to be reusable.
Good point on catching nullptrs. I'll revisit.
There was a problem hiding this comment.
I kinda agree with @notr1ch that we don't need to use nullptr checks if the occurrence of it would be a truly exceptional situation (as in: this should never happen and if it does, some developer messed up).
This would be in opposition to an API that could very well return a nullptr and that's part of its "normal operation".
The last consideration is whether even the exceptional situation is indeed "recoverable" or doing so would allow OBS to limp on as a program in a weird, undefined, state, which could lead to data corruption or data loss as a result.
Just crashing (to avoid that) is incredibly disruptive, but preferable to e.g. potentially overwriting a scene collection with garbage data.
| std::string dirs[3] = {"Local Storage", "Session Storage", "Network"}; | ||
|
|
||
| for (std::string oldDir : dirs) { | ||
| std::string newDir("Default/" + oldDir); | ||
| MigrateProfileConfigDir(parent + oldDir, parent + newDir, makeDirs); | ||
| } |
There was a problem hiding this comment.
Use constexpr for the constants:
constexpr std::array<std::string_view, 3> migrationDirectories {
"Local Storage",
"Session Storage",
"Network",
};
constexpr std::string_view defaultDirectoryPrefix {"Default/"};Which enables:
| std::string dirs[3] = {"Local Storage", "Session Storage", "Network"}; | |
| for (std::string oldDir : dirs) { | |
| std::string newDir("Default/" + oldDir); | |
| MigrateProfileConfigDir(parent + oldDir, parent + newDir, makeDirs); | |
| } | |
| for (std::string_view directory, migrationDirectories) { | |
| std::string oldDirectory {parent}; | |
| oldDirectory.append(directory); | |
| std::string newDirectory {parent}; | |
| newDirectory.append(defaultDirectoryPrefix); | |
| newDirectory.append(directory) | |
| MigrateProfileConfigDir(oldDirectory, newDirectory, makeDirs); | |
| } |
I don't know if I like the design with this function and MigrateProfileConfigDir rather than one single function. I'm kinda fine with using string concatenation at first rather than using std::filesystem::path objects as any string token would have to be converted using u8path first due to Windows using UTF-16 for paths internally.
There was a problem hiding this comment.
Done. Let me know if there are additional concerns outside of the lack of u8path handling due to C++20 deprecation warnings.
|
|
||
| try { | ||
| const auto copyOptions = std::filesystem::copy_options::recursive; | ||
| std::filesystem::copy(old_path, std::string(new_path_abs.Get()), copyOptions); |
There was a problem hiding this comment.
My hunch is that this might break in unforeseen ways as you pass in a std::string with UTF-8 bytes into a function that expects UTF-16 bytes instead.
There was a problem hiding this comment.
Hmm, probably. Do we have documentation on how best to transfer from obs_module_config_path to std::filesystem cleanly? I have tried and failed to find out. I'd guess the same way I did old_path a few lines earlier? Just seems a bit noisy going obs_module_config_path -> os_get_abs_path_ptr -> std::filesystem::u8path, unless the os_get_abs_path_ptr can be skipped?
There was a problem hiding this comment.
Without consulting a compiler, I'd probably do:
using fs = std::filesystem;
// This crashes if libobs returns a nullptr here, which could be seen as a
// critical failure that should never happen.
fs::path rootPath = fs:u8path(os_module_config_path());
fs::path oldPath = rootPath / fs:u8path(oldDir);
fs::path newPath = rootPath / fs:u8path(newDir);
if (makeDirs && !fs::exists(newPath)) {
fs::createDirectory(newPath);
}
[..]On macOS std::filesystem seems to handle absolute paths with relative path tokens just fine (so something like /Path/../SomePath works), otherwise std::filesystem::canonical can be used to resolve such a path completely, which you'd probably wrap the assignment to oldPath and newPath in - but I'm not sure that's necessary.
There was a problem hiding this comment.
Yep, that code worked pretty much out of the box - feel free to look over my changes. I have verified std::filesystem::canonical was not necessary (though it could be used to make error logging a bit clearer).
Changes made, outside of properly handing u8paths for now.
| CefRefPtr<CefRequestContext> rc = CefRequestContext::GetGlobalContext(); | ||
| CefString err; | ||
| CefRefPtr<CefValue> off = CefValue::Create(); | ||
| off->SetBool(false); | ||
| std::string opts[20] = {"autofill.credit_card_enabled", |
There was a problem hiding this comment.
Use constexpr for the constants:
constexpr std::array<std::string_view, 20> browserFeaturesToDisable {
[...]
};
constexpr std::array<std::string_view, 2> browserFeaturesToEnable {
[...]
};| CefRefPtr<CefRequestContext> rc = CefRequestContext::GetGlobalContext(); | |
| CefString err; | |
| CefRefPtr<CefValue> off = CefValue::Create(); | |
| off->SetBool(false); | |
| std::string opts[20] = {"autofill.credit_card_enabled", | |
| CefRefPtr<CefRequestContext> requestContext = CefRequestContext::GetGlobalContext(); | |
| CefString errorMessage; | |
| CefRefPtr<CefValue> optionValue = CefValue::Create(); | |
| optionValue->SetBool(false); | |
| for (std::string_view feature : browserFeaturesToDisable) { | |
| requestContext->SetPreference(feature.data(), optionValue.get(), errorMessage); | |
| } | |
| optionValue->SetBool(true); | |
| for (std::string_view feature: browserFeatuesToEnable) { | |
| requestContext->SetPreference(feature.data(), optionValue.get(), errorMessage); | |
| } |
I also wonder if this should maybe be wrapped in a lambda for each value, because any error while disabling this feature should be considered a critical failure (it would leave the browser with partially enabled Chromium runtime features which in turn could lead to unexpected behaviour and related support burden).
There was a problem hiding this comment.
Forgot to mention - as per our code style guidelines those constexpr would also have to use the k prefix (kBrowserFeaturesToDisable).
There was a problem hiding this comment.
Changes made outside of swapping to a lambda. I'll need to think on it some more, as majority of the features being disabled are mostly to avoid quirks. At the very least we could log the ones that fail to apply so we know to fix them. Happy to revisit.
|
how would I test this on arch? (sorry, really new to arch at the moment) |
|
It’s very nice to see this 🎉 Also – @hoshinolina: thanks for the CEF package in Fedora, it was very helpful when creating the Arch package ^^ @Phr3d13: I pushed |
6087ad4 to
8d898b5
Compare
tytan652
left a comment
There was a problem hiding this comment.
Issue met while building on Arch and Flatpak.
8d898b5 to
d6f85aa
Compare
| "translate", | ||
| "url_keyed_anonymized_data_collection.enabled"}; | ||
|
|
||
| constexpr std::array<std::string_view, 2> kBrowserFeaturesToEnable{"extensions.block_external_extensions", |
There was a problem hiding this comment.
I guess this is clang-format's doing? 🙈
| if (fs::exists(serviceProfiles)) { | ||
| // User has service integration cookies that also must be copied | ||
| for (auto const &dir_entry : std::filesystem::directory_iterator{serviceProfiles}) { | ||
| std::string name(dir_entry.path().filename().string()); |
There was a problem hiding this comment.
| std::string name(dir_entry.path().filename().string()); | |
| std::string name{dir_entry.path().filename().u8string()}; |
There was a problem hiding this comment.
no instance of constructor matches the argument list
argument types are: (std::basic_string<char8_t, std::char_traits<char8_t>, std::allocator<char8_t>>)
| std::string oldDir(prefix + "/" + name); | ||
| std::string newDir(prefix + "_" + name); | ||
| MigrateProfileConfigDir(oldDir, newDir, true); | ||
| // Subdirectories do not need to be migrated because it is not the Default profile |
There was a problem hiding this comment.
What does this comment relate to? The call to MigrateProfileConfigDir?
There was a problem hiding this comment.
It was a leftover comment with the intent of describing why this particular path doesn't manually copy Local Storage, Session Storage or Network, but thinking on it now it's unnecessary.
d6f85aa to
6acd268
Compare
6acd268 to
08a3aa7
Compare
Add a dummy Browser Client to introduce limitations into the Chrome UI windows that can be opened in uncontrolled edge cases. Chrome UI windows are unmanaged by default. By returning something other than null in GetDefaultClient() we can lock it down in various ways. OnBeforePopup, OnOpenURLFromTab can probably be removed as they don't get called. There's no direct way to stop the Chrome UI window from being opened, so instead close it immediately. Disable Various Chrome Settings in custom docks & browser sources via SetPreferences. For more, check `chrome/common/pref_names.h`. Chrome's default error display is now used by CEF, so on top of the existing override in browser docks, a similar override has been added to browser sources which just redirects to about:blank. Also blocks - Chrome Extensions, as they are largely untested and unpredictable - MediaRouter, which provides Cast.. functionality - CalculateNativeWinOcclusion, which lowers FPS for hidden pages - LiveCaption, which provides automatic captions - DocumentPictureInPictureAPI, which provides PiP widgets (YouTube) A data migration function is included. It moves some directories into a new 'Default' profile subdirectory, and performs the necessary renaming of certain files to ensure the new Profile loads. Additionally, the cookie directories for each service integration must be moved into the root config directory, as subdirectories in their current form (obs_profile_cookies/<cookie_id>) are not supported. This is intentional, and according to Marshall this was never a supported setup. cache_path *must* be a direct child of the root_cache_path. Invalid cache_path_ is silently treated as Incognito Mode and cookies are not stored.
08a3aa7 to
23ec123
Compare
Description
We made it! After a lot of work, burnout, and testing, support for CEF 6613 (Chrome 128) and the Chrome Runtime is ready to be reviewed!
This (finally) gives us the ability to safely migrate to the Chrome Runtime, the replacement to the Alloy Runtime that CEF has been using forever. CEF/Chrome versions from 6613/128 to 7727/147 (yes, current stable) are now fully supported and work out of the box.
Tip
Test builds of CEF can be downloaded from https://cef-builds.spotifycdn.com/index.html (use Minimal, Current Stable)
Add a dummy Browser Client to introduce limitations into the Chrome UI windows that can be opened in uncontrolled edge cases. Chrome UI windows are unmanaged by default. By returning something other than null in
GetDefaultClient()we can lock it down in various ways. https://magpcss.org/ceforum/viewtopic.php?f=6&t=19898OnBeforePopup,OnOpenURLFromTabin the Dummy Client can probably be removed as they don't get called. There's no direct way to stop the Chrome UI window from being opened, so instead close it immediately. However, I've kept them just in case.Disable Various Chrome Settings in custom docks & browser sources via
SetPreferences. For more, checkchrome/common/pref_names.h. We should actively keep an eye on this in future when updating CEF.Chrome's default error display is now used by CEF, so on top of the existing override in browser docks, a similar override has been added to browser sources which just redirects to about:blank.
Also blocks
A data migration function is included. It moves some directories into a new 'Default' profile subdirectory, and performs the necessary renaming of certain files to ensure the new Profile loads. This is necessary for user's data to be maintained, as the Alloy runtime used a bit of a custom structure which has been discarded in favour of Chrome Runtime's. CEF does not provide a migration path.
Additionally, the cookie directories for each service integration must be moved into the root config directory, as subdirectories in their current form (
obs_profile_cookies/<cookie_id>) are not supported. This is intentional, and according to Marshall this was never a supported setup. cache_path must be a direct child of the root_cache_path. Invalid cache_path_ is silently treated as Incognito Mode and cookies are not stored.cache_pathfor CefRequestContext no longer supported chromiumembedded/cef#3930To work around this, I've included some temporary code to translate the path received from the OBS frontend. When this PR is to be merged, I can submit a PR to obs-studio to match the intended behaviour.
Motivation and Context
Keeping CEF up to date is important - for security, reliability, support, and access to modern features.
How Has This Been Tested?
Tested on Windows 10 on CEF 147 with a
plugin_config/obs-browserconfig directory from OBS 32.1 to ensure data is migrated correctly. Launched again after migration to ensure data was still accessible.cef_binary_147.0.14+g76d2442+chromium-147.0.7727.138_windows64_minimalTested again with our current CEF 127 build to ensure we can use 6533 unchanged if CEF tests themselves backfire.
Additionally tested over the past few months with the following versions as they were released. No regressions were found. Mostly my fault for taking so long.
Additionally, @hoshinolina has done tremendous work testing this on Fedora, with users actively using the code in this PR since November 2025. As far as I know, users have had very few complaints.
Types of changes
Checklist: