Fix handling frontend JS events

RytoEX commented 7 months ago

Description

When handling frontend events, DispatchJSEvent expects JSON passed as a std::string. An unquoted string is not valid JSON. To get valid JSON, we can build a JSON object like in OBS_FRONTEND_EVENT_SCENE_CHANGED and pass the result of dump() to DispatchJSEvent.

When handling frontend events, DispatchJSEvent expects JSON passed as a std::string. An empty string will cause the event to never be dispatched. An empty nlohmann::json object converted to a std::string is represented by the string "null". Passing this exact string as the JSON string to DispatchJSEvent makes the events work. This is probably because an empty unquoted string by itself is not valid JSON. Two double-quotes with nothing inside them or the unquoted string "null" are considered valid JSON.

Motivation and Context

Fixes #428

How Has This Been Tested?

Tested locally on Windows 11.

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.

WizardCM commented 7 months ago

Thanks for digging into this. In my own testing I did track it down to the change away from json11, but couldn't dig down further.

I agree with the solution in https://github.com/obsproject/obs-browser/pull/429/commits/914368745e5ae70df699e5ba245d3b478eac9695 as it's the right change.

For the second commit, I'd like to propose an alternative:

diff --git a/browser-app.cpp b/browser-app.cpp
index 582b1c6e..71975ad0 100644
--- a/browser-app.cpp
+++ b/browser-app.cpp
@@ -328,8 +328,12 @@ bool BrowserApp::OnProcessMessageReceived(CefRefPtr<CefBrowser> browser,
                ExecuteJSFunction(browser, "onActiveChange", arguments);

        } else if (message->GetName() == "DispatchJSEvent") {
-               nlohmann::json payloadJson = nlohmann::json::parse(
-                       args->GetString(1).ToString(), nullptr, false);
+               CefString data = args->GetString(1);
+               if (data.empty())
+                       data = "null";
+
+               nlohmann::json payloadJson =
+                       nlohmann::json::parse(data.ToString(), nullptr, false);

                nlohmann::json wrapperJson;
                if (args->GetSize() > 1)

My method of tracking down exactly how best to solve this was by checking payloadJson.is_discarded() which returns true if the string passed to nlohmann is invalid JSON.

You are correct that we should be setting the JSON to null, as that is consistent with events pre-30 (verified in OBS 29.1). However, for overall readability, I'd like to stick with passing an empty string to DispatchJSEvent.

RytoEX commented 7 months ago

See my previous comment on current thoughts.

I'm a bit torn. On the one hand, your proposal catches all possible invocations of DispatchJSEvent. On the other hand, it does introduce some extra work on processing events (though it's probably negligible), and I personally prefer that we just pass valid JSON to DispatchJSEvent when calling it because I think it's more explicit, though I understand the appeal of catching unexpected cases like this. Ultimately, I don't have a strong argument for one proposal over the other.

BarryCarlyon commented 7 months ago

I would not consider it a "breaking change" to change to a new behaviour in OBS 30. (passing an empty string)

Since the documentation for the events that (will now) have an empty string, people are not (or shouldn't) be expecting to have anything passed to them anyway on those events. And even if you are JSON parsing blinding you'd be wrapping in a try/catch in your browser source HTML.

So I'd throw a vote in for the new behaviour (empty string), then we can get this shipped in the OBS hotfix.

We just might need also consider a tweak to the documentation about nulls/empty strings if you are trying to capture and parse a return value to the listener for an event where one isn't expected anyway. For "bad devs" (used loosley) a empty string is still as "falsey" as a null

RytoEX commented 7 months ago

To prevent this from sitting longer, I'm going to merge this as-is, since this is the code I have already tested. If there are further changes we wish to make later, feel free to open a PR.

obsproject / obs-browser