-
Notifications
You must be signed in to change notification settings - Fork 223
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling frontend JS events #429
Conversation
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.
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 9143687 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 You are correct that we should be setting the JSON to |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
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 |
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. |
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
Checklist: