[POC] Pass GlobalFeatures from browser to renderer #7832

kjyoun · 2025-11-02T05:46:41Z

Bug: 1234

kjyoun · 2025-11-02T06:13:47Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a proof-of-concept for passing global features from the browser process to the renderer process using a new Mojo interface, CobaltSettings. While the overall approach of using Mojo for this IPC is sound, the current implementation has several critical and high-severity issues that must be addressed. These include a use-after-free vulnerability due to incorrect lifetime management of a Mojo remote, race conditions, and buggy logic in the renderer's initialization flow. I've provided detailed comments and suggestions for each issue.

media/mojo/services/gpu_mojo_media_client.cc

media/starboard/starboard_renderer.cc

cobalt/browser/cobalt_settings_impl.cc

media/starboard/starboard_renderer.cc

kjyoun · 2025-11-02T19:54:29Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a mechanism to pass settings from the browser process to the renderer process using a new Mojo interface, cobalt.mojom.CobaltSettings. This is used as a proof-of-concept to control the use_external_allocator flag in StarboardRenderer. The changes are well-structured and follow the intended goal. I have a couple of suggestions to improve robustness and safety, particularly around handling the setting value and ensuring callback contracts are met.

cobalt/browser/cobalt_settings_impl.cc

media/starboard/starboard_renderer.cc

kjyoun · 2025-11-02T22:44:24Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a mechanism to pass settings from the browser process to the renderer, which is a good step towards centralizing configuration. It adds a new CobaltSettings Mojo interface and also uses a command-line switch to pass the use_external_allocator setting for early renderer initialization. The changes are quite extensive, plumbing the new setting through the media stack.

My review identified a few areas for improvement. There are several leftover debugging logs that should be removed. There also appears to be a redundant Mojo interface binding for CobaltSettings that could be cleaned up. More importantly, the use_external_allocator setting is not yet used to configure the media player, as noted by a TODO in the code, which makes the feature incomplete. I've also suggested a small refactoring to improve code clarity. Overall, this is a solid proof-of-concept that needs a bit more work to be complete.

media/starboard/sbplayer_bridge.cc

cobalt/browser/cobalt_browser_interface_binders.cc

cobalt/browser/cobalt_content_browser_client.cc

$ cat ~/log.adb.txt | egrep "httpProtocal|external_allocator" 11-02 15:51:30.059 13764 13814 I cobalt : [13764:13814:1102/155130.059752:INFO:starboard_renderer_client_factory.cc(56)] StarboardRendererClientFactory constructed: use_external_allocator_=true 11-02 15:51:30.084 13764 13806 I cobalt : [13764:13806:1102/155130.084340:INFO:starboard_renderer.cc(132)] StarboardRenderer constructed: use_external_allocator_=true 11-02 15:51:30.084 13764 13806 I cobalt : [13764:13806:1102/155130.084562:INFO:starboard_renderer_wrapper.cc(49)] Creating StarboardRendererWrapper: traits.use_external_allocator=true 11-02 15:51:30.188 13764 13806 I cobalt : [13764:13806:1102/155130.188798:INFO:sbplayer_bridge.cc(761)] Use external_allocator=true 11-02 15:51:35.328 13764 13814 I cobalt : [13764:13814:1102/155135.328395:INFO:starboard_renderer_client_factory.cc(56)] StarboardRendererClientFactory constructed: use_external_allocator_=false 11-02 15:51:35.331 13764 13806 I cobalt : [13764:13806:1102/155135.331459:INFO:starboard_renderer.cc(132)] StarboardRenderer constructed: use_external_allocator_=false 11-02 15:51:35.332 13764 13806 I cobalt : [13764:13806:1102/155135.332739:INFO:starboard_renderer_wrapper.cc(49)] Creating StarboardRendererWrapper: traits.use_external_allocator=false 11-02 15:51:37.793 13764 13806 I cobalt : [13764:13806:1102/155137.793737:INFO:sbplayer_bridge.cc(761)] Use external_allocator=false

kjyoun · 2025-11-02T23:57:46Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new mojom interface, CobaltSettings, to pass global features from the browser process to the renderer process. This is a good architectural improvement. The implementation includes the mojom definition, the browser-side implementation, and plumbing for exposing the interface.

However, there are a few key issues. The most critical is that one usage in the renderer process directly accesses a browser-side singleton, violating process boundaries, instead of using the new mojom interface. Additionally, the new interface is exposed in two different ways, which is redundant. There are also some leftover debugging logs that should be removed.

My review includes suggestions to address these points to align the implementation with the intended architecture.

cobalt/renderer/cobalt_content_renderer_client.cc

cobalt/browser/cobalt_browser_interface_binders.cc

cobalt/browser/cobalt_content_browser_client.cc

cobalt/browser/cobalt_settings_impl.cc

cobalt/browser/cobalt_settings_impl.h

kjyoun · 2025-11-03T00:31:51Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a Mojo interface to pass settings from the browser to the renderer, which is a good approach. However, the implementation in the renderer process currently bypasses this interface and instead directly links against and calls browser-side code (GlobalFeatures). This is a significant architectural issue that breaks process isolation and must be corrected. The renderer should use the new cobalt.mojom.CobaltSettings interface to fetch settings. I've provided detailed suggestions on how to achieve this. Additionally, there are a few minor cleanups that can be done.

cobalt/renderer/BUILD.gn

cobalt/renderer/cobalt_content_renderer_client.cc

cobalt/browser/cobalt_settings_impl.cc

cobalt/browser/cobalt_settings_impl.h

@gemini-code-assist

cobalt/browser/cobalt_settings_impl.h Comment on lines +21 to +23 namespace content { class RenderFrameHost; } // namespace content Contributor @gemini-code-assist gemini-code-assist bot 1 minute ago medium The forward declaration for content::RenderFrameHost doesn't seem to be used in this header or the corresponding .cc file. It can be removed to keep the code clean.

kjyoun · 2025-11-03T00:45:28Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces a new Mojo interface CobaltSettings to pass settings from the browser process to the renderer process. This is used to pass the use_external_allocator setting. The changes include the new Mojo interface definition, its implementation in the browser process, and plumbing the new setting through the media stack.

While the overall approach of using a Mojo interface is sound, the current implementation in the renderer process uses a synchronous, blocking call to fetch the setting. This is a critical issue as it can lead to unresponsiveness and should be replaced with an asynchronous pattern. I've also included a couple of medium-severity suggestions to improve code maintainability and robustness.

gemini-code-assist · 2025-11-03T00:46:56Z

cobalt/renderer/cobalt_content_renderer_client.cc

+  if (cobalt_settings_remote.is_bound()) {
+    base::WaitableEvent event;
+    cobalt_settings_remote->GetSetting(
+        "use_external_allocator",
+        base::BindOnce(
+            [](media::RendererFactoryTraits* traits, base::WaitableEvent* event,
+               cobalt::mojom::SettingValuePtr value) {
+              if (value && value->is_string_value()) {
+                traits->use_external_allocator =
+                    (value->get_string_value() == "true");
+              }
+              event->Signal();
+            },
+            base::Unretained(renderer_factory_traits), &event));
+    event.Wait();
+  }


Blocking the renderer thread with event.Wait() is a serious performance and stability risk. This can lead to UI jank, unresponsiveness, and potentially trigger "Application Not Responding" (ANR) errors on some platforms. Even during initialization, this synchronous wait can significantly increase startup time and delay the rendering of the first frame.

While this may be acceptable for a Proof of Concept, this pattern should not be used in production code.

Please consider one of the following asynchronous approaches:

Pass settings via command-line switches: For settings that are known when the renderer process is created, passing them as command-line arguments is a common and efficient pattern in Chromium.

Asynchronous initialization: Refactor the initialization logic to be asynchronous. The component that depends on this setting (StarboardRenderer) could be initialized after the setting has been fetched asynchronously. This might involve a larger change but is a more robust solution for settings that can change dynamically.

For example, you could move the logic that depends on use_external_allocator into the callback itself or into a subsequent task posted from the callback.

gemini-code-assist · 2025-11-03T00:46:56Z

cobalt/renderer/cobalt_content_renderer_client.cc

+  if (cobalt_settings_remote.is_bound()) {
+    base::WaitableEvent event;
+    cobalt_settings_remote->GetSetting(
+        "use_external_allocator",


Using a magic string "use_external_allocator" for the setting key can be error-prone and makes the code harder to maintain. Consider defining this key as a named constant in a shared header file (e.g., with the mojom definition or in a new cobalt/browser/settings_keys.h). This will prevent typos and make it easier to find all usages of this key.

For example:

// In a shared header namespace cobalt { namespace settings { extern const char kUseExternalAllocator[]; } // namespace settings } // namespace cobalt // In a .cc file namespace cobalt { namespace settings { const char kUseExternalAllocator[] = "use_external_allocator"; } // namespace settings } // namespace cobalt

Then you can use cobalt::settings::kUseExternalAllocator here.

gemini-code-assist · 2025-11-03T00:46:56Z

cobalt/renderer/cobalt_content_renderer_client.cc

+              if (value && value->is_string_value()) {
+                traits->use_external_allocator =
+                    (value->get_string_value() == "true");
+              }


The current implementation only handles a string value for the setting. The SettingValue union in the mojom definition also supports int64_t. If the setting is ever sent as an integer (e.g., 1 for true), it will be silently ignored, and use_external_allocator will retain its default value.

To make this more robust, you could either:

Handle the integer case as well.

Add a DCHECK to ensure the type is a string, making the contract explicit.

if (value) { if (value->is_string_value()) { traits->use_external_allocator = (value->get_string_value() == "true"); } else if (value->is_int_value()) { traits->use_external_allocator = (value->get_int_value() != 0); } }

This reverts commit b666f1f.

Initial impl.

0cf872c

kjyoun force-pushed the global-features-connect branch from 91413b5 to 0cf872c Compare November 2, 2025 05:52

Complete picture

3b328b7

gemini-code-assist bot reviewed Nov 2, 2025

View reviewed changes

Gemini comment

2a5a93b

gemini-code-assist bot reviewed Nov 2, 2025

View reviewed changes

cobalt/browser/cobalt_settings_impl.cc Show resolved Hide resolved

media/starboard/starboard_renderer.cc Outdated Show resolved Hide resolved

gemini-code-assist bot reviewed Nov 2, 2025

View reviewed changes

kjyoun added 3 commits November 2, 2025 15:47

Another refactoring - Works

044ea80

Cleanup

52b417e

kjyoun force-pushed the global-features-connect branch from 4669bd1 to 52b417e Compare November 2, 2025 23:57

gemini-code-assist bot reviewed Nov 2, 2025

View reviewed changes

kjyoun added 2 commits November 2, 2025 16:17

fix build and ammend Gemini comments

64c2942

Remove bind_host_receiver_callback

97b6b62

gemini-code-assist bot reviewed Nov 3, 2025

View reviewed changes

cobalt/renderer/BUILD.gn Outdated Show resolved Hide resolved

cobalt/renderer/cobalt_content_renderer_client.cc Show resolved Hide resolved

cobalt/browser/cobalt_settings_impl.cc Show resolved Hide resolved

cobalt/browser/cobalt_settings_impl.h Outdated Show resolved Hide resolved

kjyoun added 3 commits November 2, 2025 16:35

Gemini comment

35a60a7

Gemini comment - 2

b666f1f

kjyoun force-pushed the global-features-connect branch from 41d623d to b666f1f Compare November 3, 2025 00:44

gemini-code-assist bot reviewed Nov 3, 2025

View reviewed changes

kjyoun added 2 commits November 2, 2025 16:55

Revert "Gemini comment - 2"

5707e8f

This reverts commit b666f1f.

Remove unnecessary deps

f0022f9

[POC] Pass GlobalFeatures from browser to renderer #7832

Are you sure you want to change the base?

[POC] Pass GlobalFeatures from browser to renderer #7832

Uh oh!

Conversation

kjyoun commented Nov 2, 2025

Uh oh!

kjyoun commented Nov 2, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kjyoun commented Nov 2, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

kjyoun commented Nov 2, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kjyoun commented Nov 2, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kjyoun commented Nov 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kjyoun commented Nov 3, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant