Add full support for CEF 4638

PatTheMav commented 2 years ago

Description

Introduces necessary fixes for compatibility with CEF 4638 due to API changes within CEF.

Note: This PR stays a draft for now as we need a proper solution to the upstream changes regarding web security disabling for local file support.

Motivation and Context

Make obs-browser compatible with CEF 4638 which is required for M1 support.

How Has This Been Tested?

Tested on Apple M1 machine

Types of changes

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

Checklist:

[x] My code has been run through clang-format.
[x] I have read the contributing document.
[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.

PatTheMav commented 2 years ago

Added RequestHandler and ResourceRequestHandler callbacks to intercept requests made by a browser source. While this allows us to "detect" CORS requests from local files (via the null Origin header), in my tests I've been able to change other headers and even the URL inside OnBeforeResourceLoad but any changes to the Origin header were ignored.

PatTheMav commented 2 years ago

After discussion with the team, we came to the following conclusion:

Loading local media files for playback works as intended
Loading external JS files via <script> works as intended
CORS requests from servers that enforce strict CORS policies will fail due to null origin for local URLs

As the latter point mirrors behaviour of current browsers, we deem it a "won't fix" coupled with updated user guidance once we update.

tt2468 commented 2 years ago

Ubuntu 20.04 (Xorg)
This branch
Latest master from obs-studio

Issue A: Building on Ubuntu 20.04 with 4280 fails, with a few errors (See Discord)

Issue B: Every instance of a browser source spams this message to the log, and there is no video: [1120/215034.001000:WARNING:task_impl.cc(31)] No task runner for threadId 0

The more browsers that are added, the more spam there is. The browser can be removed, stopping the spam, but it starts up again when you go to close OBS, and requires killing.

OBS functions as expected when no browser stuff is ever loaded.

PatTheMav commented 2 years ago

That's weird given that most changes are guarded by a version check for newer CEF versions only, the changes that affect every version is proper use of nullptr and Dillon's change to use cef_window_open_disposition_t from what I could see..

tt2468 commented 2 years ago

@PatTheMav I can try using a fresh dev environment in the morning, just to make sure that there isn't some random issue.

tt2468 commented 2 years ago

Fresh ENV - Building with 4280 still errors, and I also discovered that I was only able to build with the spotify 4638 CEF because of previous cmake cache. I need a download of 4638 with the correct CEF wrapper.

tt2468 commented 2 years ago

Using the latest version:

CEF 4280 builds and works as expected

CEF 4638 builds and works as expected, with the bonus that it doesn't throw any threadrunner errors.

Big thing to note: At least on Ubuntu 20.04, in order to use 4638 you need to use a fresh build environment. For example, having a 4280-based environment then reconfiguring with cmake will cause tons of wacky errors on runtime, and the browser source will simply not work.

Perhaps there is some way to detect an old version of cef in cmakelists, to throw an explicit configuration error if the version changes?

WizardCM commented 2 years ago

Remaining things to test/fix:

[x] AJAX local files in "Local file" mode
[x] Verify 3770 still works on Windows with these changes

PatTheMav commented 2 years ago

Using the latest version:

CEF 4280 builds and works as expected

CEF 4638 builds and works as expected, with the bonus that it doesn't throw any threadrunner errors.

Big thing to note: At least on Ubuntu 20.04, in order to use 4638 you need to use a fresh build environment. For example, having a 4280-based environment then reconfiguring with cmake will cause tons of wacky errors on runtime, and the browser source will simply not work.

Perhaps there is some way to detect an old version of cef in cmakelists, to throw an explicit configuration error if the version changes?

From what I could tell that is by design - CEF_ROOT_DIR only specifies a hint for FindCEF to look for its required files - once they are found, these values are set as cache variables and changing the root directory won't have any effect, as the library is already found.

The intended way is to delete the cache variables associated with CEF (except for the root directory) and re-run CMake again (so FindCEF will find the "new" library and headers).

As for a way to fix this: CEF would have to produce a proper CMake package with version and target configuration - then you could do find_package(CEF 95.xxx.xxx) and specify the exact version you want. As long as that isn't the case you'd have to do version management by hand.

phedders commented 2 years ago

On Wed, 2021-11-24 at 04:35 -0800, Patrick Heyer wrote:

The intended way is to delete the cache

Would you be able to 'remind' (:D) me on the correct or most efficient way to do that? Many thanks!

PatTheMav commented 2 years ago

On Wed, 2021-11-24 at 04:35 -0800, Patrick Heyer wrote: The intended way is to delete the cache Would you be able to 'remind' (:D) me on the correct or most efficient way to do that? Many thanks!

Either run ccmake -S . -B [YOUR_BUILD_DIR], then press t to see the advanced entries and delete every entry starting with CEF_ but CEF_ROOT_DIR.

Or edit CMakeCache.txt in your build directory directly and also remove those entries.

WizardCM commented 2 years ago

This seems to fix AJAX calls to local files in local file mode, but may break large media files.

diff --git a/browser-scheme.hpp b/browser-scheme.hpp
index 62b4d75e..dd28614f 100644
--- a/browser-scheme.hpp
+++ b/browser-scheme.hpp
@@ -22,7 +22,7 @@
 #include <string>
 #include <fstream>

-#if CHROME_VERSION_BUILD >= 3440
+#if CHROME_VERSION_BUILD >= 3440 && CHROME_VERSION_BUILD < 4638
 #define ENABLE_LOCAL_FILE_URL_SCHEME 1
 #else
 #define ENABLE_LOCAL_FILE_URL_SCHEME 0

Edit: Verified this breaks large media files. But only sometimes?

PatTheMav commented 2 years ago

Large media file playback seems fine to me, I checked local CORS with your test suite, both XHR and Fetch seem to work fine. Pointing to a non-existing video source led to a hard crash though, as we don't check for file existence before creating a CefStreamReader it seems.

Loading remote will still be blocked by every host that doesn't use Access-Control-Allow-Origin: * but that's kind-of obvious.

Edit: Added a fix for the missing file issue - how did you recreate the large media file issue? I managed to watch an entire TV episode via browser source without issue just now.

WizardCM commented 2 years ago

For some reason, the diff below fixes the browser panel resize issue on Linux. If we end up using this fix temporarily, I recommend we scope it to 4638.

diff --git a/panel/browser-panel.cpp b/panel/browser-panel.cpp
index e56565b29..55af1a1bf 100644
--- a/panel/browser-panel.cpp
+++ b/panel/browser-panel.cpp
@@ -427,6 +427,8 @@ void QCefWidgetInternal::Resize()
                changes.height = size.height();
                XConfigureWindow(xDisplay, (Window)handle,
                                 CWX | CWY | CWHeight | CWWidth, &changes);
+               XSync(xDisplay, false);
 #endif
        });

This is probably not the best solution, but it does work.

PatTheMav commented 2 years ago

Confirmed the fix, added as additional commit.

obsproject / obs-browser