[rust] Publish Selenium Manager as native shared library #15368

nvborisenko · 2025-03-03T19:21:35Z

User description

This is a PR for ongoing collaboration to make Selenium Manager be shipped as native library (C-compatible dynamic library).

Motivation and Context

Implements #15355

Checklist

Using cargo build should produce 3 output types (executable, rust lib, native shared lib)
Update bazel where it is required
Update github workflow to publish native libs to github artifacts
Export rust function for external consumers (like we do sm.exe --browser chrome, but now as a function)

Types of changes

Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)

Checklist

I have read the contributing document.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have added tests to cover my changes.
All new and existing tests passed.

PR Type

Enhancement

Description

Added support for building Selenium Manager as a shared library.
Introduced a new rust_shared_library target in Bazel build files.
Exported a sample Rust function (get_test) for external usage.
Updated Cargo.toml to define library crate types.

Changes walkthrough 📝

Relevant files

Configuration changes

defs.bzl Introduced `rust_shared_library` in Bazel definitions rust/defs.bzl Added `rust_shared_library` to the list of available Rust build targets.	+2/-0
rustfmt_wrapper.bzl Added `rust_shared_library` to formatting wrapper rust/private/rustfmt_wrapper.bzl Added `rust_shared_library` to the wrapper functions. Ensured formatting tests are applied to shared libraries.	+5/-0
BUILD.bazel Defined `rust_shared_library` target in Bazel rust/BUILD.bazel Added a new `rust_shared_library` target for Selenium Manager. Configured the shared library to include necessary dependencies.	+13/-1
Cargo.toml `Updated Cargo.toml for shared library support` rust/Cargo.toml Defined library crate types (`cdylib` and `rlib`). Added configuration for the Selenium Manager binary and library.	+7/-0

Enhancement

lib.rs `Added exported function for shared library usage` rust/src/lib.rs Added an exported function `get_test` for external usage. Demonstrated how to expose Rust functions for C compatibility.	+13/-0

Need help?
Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
Check out the documentation for more information.

qodo-merge-pro · 2025-03-03T19:22:03Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 Security concerns Memory safety: The exposed C interface in `get_test()` function returns a raw pointer without proper memory management guidelines or deallocation function, which could lead to memory corruption or crashes if not handled correctly by external consumers.
⚡ Recommended focus areas for review Memory Leak The `get_test()` function returns a raw pointer but doesn't provide a way for the caller to free the allocated memory. This could lead to memory leaks when called from external code. pub extern "C" fn get_test() -> mut std::os::raw::c_char { let sm = get_manager_by_browser("chrome".to_string()).unwrap(); let dp = std::ffi::CString::new(sm.get_driver_path_in_cache().unwrap().display().to_string()).unwrap(); dp.into_raw() // Transfer ownership to C } Error Handling* The `get_test()` function uses unwrap() on potential failure points which could cause crashes in production. Should implement proper error handling for external API. let sm = get_manager_by_browser("chrome".to_string()).unwrap(); let dp = std::ffi::CString::new(sm.get_driver_path_in_cache().unwrap().display().to_string()).unwrap();

qodo-merge-pro · 2025-03-03T19:22:42Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	✅ ~~Fix memory leak in FFI~~ Suggestion Impact: The commit completely rewrote the FFI interface but implemented the core suggestion by adding a proper memory freeing function. The new implementation uses a struct to return results and includes a dedicated free_webdriver_path_result function that properly frees memory allocated for C strings, addressing the memory leak concern raised in the suggestion. code diff: +#[no_mangle] +pub extern "C" fn free_webdriver_path_result(result: mut WebDriverPathResult) { + if result.is_null() { + return; + } + unsafe { + let ffi_result = &mut result; + if !ffi_result.driver_path.is_null() { + // Reconstruct CString to drop it and free memory + let _ = std::ffi::CString::from_raw(ffi_result.driver_path); + } + if !ffi_result.error.is_null() { + // Reconstruct CString to drop it and free memory + let _ = std::ffi::CString::from_raw(ffi_result.error); + } + } +} The raw pointer returned by `get_test()` is never freed, which will cause memory leaks. Add a complementary function to free the memory from the C side. rust/src/lib.rs [1683-1689] #[no_mangle] pub extern "C" fn get_test() -> mut std::os::raw::c_char { let sm = get_manager_by_browser("chrome".to_string()).unwrap(); let dp = std::ffi::CString::new(sm.get_driver_path_in_cache().unwrap().display().to_string()).unwrap(); dp.into_raw() // Transfer ownership to C } +#[no_mangle] +pub extern "C" fn free_test_string(s: mut std::os::raw::c_char) { + unsafe { + if !s.is_null() { + let _ = CString::from_raw(s); + } + } +} + `[Suggestion has been applied]` Suggestion importance[1-10]: 9 __ Why: Memory leaks in FFI code are critical issues that can cause resource exhaustion. Adding a complementary function to free the allocated memory is essential for proper memory management when interacting with C code.	High
Possible issue	✅ ~~Handle errors in FFI code~~ Suggestion Impact: The commit completely redesigned the FFI interface, replacing the simple get_test() function with a more robust approach using a WebDriverPathResult struct that properly handles errors. While not implementing the suggestion directly, it addresses the same underlying issue by providing proper error handling in FFI code. code diff: +#[repr(C)] +pub struct WebDriverPathResult { + success: bool, + driver_path: mut std::ffi::c_char, + error: mut std::ffi::c_char, +} + +// this is just an example how to expose function for external usage #[no_mangle] -pub extern "C" fn get_test() -> mut std::os::raw::c_char { - let sm = get_manager_by_browser("chrome".to_string()).unwrap(); - - let dp = std::ffi::CString::new(sm.get_driver_path_in_cache().unwrap().display().to_string()).unwrap(); - dp.into_raw() // Transfer ownership to C +pub extern "C" fn get_dummy_webdriver_path(driver_name: const std::ffi::c_char, log: LogCallback) -> WebDriverPathResult { + for i in 1..6 { + //let message = std::ffi::CString::new("Hello, I am logging message").unwrap(); + //log(i, message.as_ptr()); + } + + let driver = unsafe { std::ffi::CStr::from_ptr(driver_name).to_str().unwrap() }; + + return WebDriverPathResult { + success: true, + driver_path: std::ffi::CString::new("This is dummy driver path for ".to_owned() + driver).unwrap().into_raw(), + //driver_path: std::ffi::CString::new(String::from("A").repeat(10_000_000)).unwrap().into_raw(), + error: std::ptr::null_mut(), + } } The function unwraps results without proper error handling, which could cause crashes in C code. Implement proper error handling and return a null pointer on error. rust/src/lib.rs [1684-1689] pub extern "C" fn get_test() -> *mut std::os::raw::c_char { - let sm = get_manager_by_browser("chrome".to_string()).unwrap(); - - let dp = std::ffi::CString::new(sm.get_driver_path_in_cache().unwrap().display().to_string()).unwrap(); - dp.into_raw() // Transfer ownership to C + let sm = match get_manager_by_browser("chrome".to_string()) { + Ok(m) => m, + Err(_) => return std::ptr::null_mut(), + }; + + let path = match sm.get_driver_path_in_cache() { + Ok(p) => p, + Err(_) => return std::ptr::null_mut(), + }; + + match std::ffi::CString::new(path.display().to_string()) { + Ok(s) => s.into_raw(), + Err(_) => std::ptr::null_mut(), + } } `[Suggestion has been applied]` Suggestion importance[1-10]: 8 __ Why: Using unwrap() in FFI code can cause program crashes when errors occur. Proper error handling with null pointer returns is crucial for maintaining stability and safety in cross-language interfaces.	Medium
Update

nvborisenko · 2025-03-03T19:31:07Z

rust/src/lib.rs

+// Exported functions
+// ----------------------------------------------------------
+
+// this just an example how to expose function for external usage


@bonigarcia can you please help to implement this function in rust? This function should be equivalent of selenium-manager.exe --browser chrome --browser-version 130 --browser-path /some/path --proxy some_value.

This is a major change, so there should be a project consensus on this feature first. Personally, I am not sure about it. Are the other bindings (in addition to .Net) following this approach?

titusfortner · 2025-03-05T18:05:55Z

@nvborisenko since this will be easiest for .NET, what would those bindings need to do to use this? What would be next steps/timeframe?

nvborisenko · 2025-03-05T18:34:58Z

I have prepared a checklist:

Using cargo build should produce 3 output types (executable, rust lib, native shared lib) - DONE?
Update bazel where it is required - I am not sure it is even required. Probably if Boni wants to create new unit test
Update github workflow to publish native libs to github artifacts - Here you Titus can help. We expect sm.so, sm.dll, and sm.dylib published as a release to https://github.com/SeleniumHQ/selenium_manager_artifacts besides regular sm, sm.exe. Naming convention should be the same as for binary (excepting file extension), reflecting OS architecture where it can be ran (literally as is).
Export rust function for external consumers (like we do sm.exe --browser chrome, but now as a function) - And here it is big question. When we call SM, what we want to get? I see only driver_path and browser_path. In any case @bonigarcia might help or not, we should: 1) understand how to avoid memory leaking (when rust allocates memory, and who is release). 2) Agree on method signature: input/output parameters.

nvborisenko · 2025-03-05T18:40:05Z

@nvborisenko since this will be easiest for .NET, what would those bindings need to do to use this? What would be next steps/timeframe?

I asked big brain, seems it is easy to do for all bindings. But, yes, .NET is the easiest.

titusfortner · 2025-03-06T04:51:44Z

I think this is what it would look like, and probably pretty straightforward:

src/
├── main.rs # CLI entry point (minimal, calls into lib.rs)
├── lib.rs # Core logic (shared between CLI and FFI)
├── ffi.rs # FFI functions (pub extern "C")

Move the implementation details from main.rs to lib.rs
Put the external functions in ffi.rs
Both main.rs and ffi.rs manage their respective inputs, execute the functions in lib.rs and manage their respective outputs

I'm not sure how .NET handles streaming the logs via ffi, and I think we're also supposed to call in to free the memory once we've gotten the paths and stuff, but should all be doable.

titusfortner · 2025-03-06T05:40:11Z

All the logistics for building/storing should be just updating this one file - https://github.com/SeleniumHQ/selenium/blob/trunk/.github/workflows/ci-rust.yml

FFI needs a get_paths function that accepts a JSON object and returns a JSON object. We need both browser path and driver path to send back to the bindings. Service class starts the driver at the provided path, then the driver needs to reference the matching browser location when sending capabilities to the driver

nvborisenko · 2025-03-06T16:47:13Z

FFI needs a get_paths function that accepts a JSON object and returns a JSON object.

No json objects in native functions, please. We have primitive strings, numbers, booleans, delegates (callback functions).

I'm not sure how .NET handles streaming the logs via ffi

To "stream" logs in runtime, rust will just call this delegate (pointer to function), provided by bindings.

titusfortner · 2025-03-06T17:10:01Z

We need to send key value pairs back and forth, there's no primitive for a map, and everything works with json

nvborisenko · 2025-03-06T17:12:37Z

Structures.

nvborisenko · 2025-03-06T19:23:03Z

Streaming log messages is resolved (I am not care about memory leaking yet). Verified, works good.

About returning of driver_path and browser_path. Do we really need it? Can it be 2 separate functions? I am asking because of I remember the logic in bindings when SM might return empty browser_path, and then binding decide something. Now we have a great chance to revisit what exactly we want to get from SM and when. Seems it might be 2 separate functions?

titusfortner · 2025-03-06T22:56:46Z

Streaming log messages is resolved (I am not care about memory leaking yet). Verified, works good.

Excellent!

Can it be 2 separate functions?

I was going to say no, but we might be able to do it in 2 method calls...
The main thing is that the driver method requires running the browser method, so they definitely can't be run async, and the rust functions don't maintain state between calls... But you could use the output from the browser method as an argument to the driver method. I'm just not sure how that makes the code better

Structures

Eesh, I thought the point was to decrease the code we needed in the bindings? 😂 JSON would be a lot simpler, wouldn't require managing memory, and the overhead is going to be a lot less than what we have now. But I'm open to it.

nvborisenko · 2025-03-07T18:32:57Z

Memory management is resolved. Rust part is here.

C# full example:

using System;
using System.Runtime.InteropServices;

namespace ClassLibrary1
{
    public static partial class SeleniumManager
    {
        [UnmanagedFunctionPointer(CallingConvention.Cdecl)]
        public delegate void LogCallback(int level, string message);

        private static readonly LogCallback _callback;

        static SeleniumManager()
        {
            // Define the callback logic
            _callback = (result, message) =>
            {
                Console.WriteLine($"Result: {result}, Message: {message}");
            };
        }

        public static string GetDummyDriverPath(string driverName)
        {
            // Convert the delegate to a function pointer
            IntPtr _callbackPtr = Marshal.GetFunctionPointerForDelegate(_callback);

            var result = get_dummy_webdriver_path(driverName, _callbackPtr);

            try
            {
                return Marshal.PtrToStringAnsi(result.DriverPath);
            }
            finally
            {
                free_webdriver_path_result(ref result);
            }
        }

        [StructLayout(LayoutKind.Sequential)]
        struct WebDriverPathResult
        {
            public bool IsSuccess;
            public IntPtr DriverPath;
            public IntPtr Error;
        }

        [DllImport("selenium_manager", CallingConvention = CallingConvention.Cdecl)]
        static extern WebDriverPathResult get_dummy_webdriver_path(string driverName, IntPtr callback);

        [DllImport("selenium_manager")]
        static extern void free_webdriver_path_result(ref WebDriverPathResult result);
    }
}

And usage:

Console.WriteLine(SeleniumManager.GetDummyDriverPath("chrome").DriverPath)

Next step: resolve how rust will share errors (even unhandled).

rust/src/lib.rs

nvborisenko · 2025-03-07T20:15:56Z

@titusfortner about ffi.rs module. We can move all extern functions into this separate module, but we also should re-declare these functions in lib.rs anyway. As I understand correctly lib.rs is a compilation unit. BTW moving the implementations of functions to ffi.rs is a good idea, despite that we also need to duplicate function "signatures" in lib.rs. I think we still want doing it.

nvborisenko · 2025-03-07T21:01:43Z

Moved to ffi.rs.

Now, as a .NET/C# consumer, I am ready to try out real function instead of "dummy".

How I see further steps:

I can continue to adjust GitHub Workflow to publish native library
We should finalize function signature (what to return and what to accept)
@bonigarcia (or anybody else) will replace "dummy placeholder" with real implementation (in ffi.rs)

nvborisenko · 2025-03-07T21:29:00Z

rust/src/ffi.rs

+            log(i, message.as_ptr());
+        }
+
+        //panic!("Intentional panic for testing");


@bonigarcia can you help me here? We want to handle all panics in this block and propagate it in returning result. Seems I do it incorrectly.

Sorry, I don't have bandwidth for the following weeks. In any case, this deserves to be discussed in the following Selenium Dev Summit in Valencia (24-25 March).

nvborisenko · 2025-03-07T21:36:17Z

Earlier benchmark results, where AsProcess current behavior (cached), AsLibrary - "dummy" SM implementation.

Method	Mean	Error	StdDev	Gen0	Allocated
AsProcess	266,338,900.0 ns	4,967,127.39 ns	4,878,383.27 ns	-	50608 B
AsLibrary	992.8 ns	8.97 ns	7.95 ns	0.0782	496 B

Mean : Arithmetic mean of all measurements
Error : Half of 99.9% confidence interval
StdDev : Standard deviation of all measurements
Gen0 : GC Generation 0 collects per 1000 operations
Allocated : Allocated memory per single operation (managed only, inclusive, 1KB = 1024B)
1 ns : 1 Nanosecond (0.000000001 sec)

titusfortner · 2025-03-09T20:22:28Z

So, for Windows you'd update the ci-rust.yml with something like this. It should all end up in selenium_manager_artifacts release as expected.

  - name: "Build shared library"
    run: cargo build --release --crate-type=cdylib
    working-directory: rust

  - name: "Rename binary"
    run: |
      mv rust/target/release/selenium-manager.exe selenium-manager-windows.exe
      mv rust/target/release/selenium_manager.dll selenium-manager-windows.dll

  - name: "Upload release binary"
    uses: actions/upload-artifact@v4
    with:
      name: selenium-manager-windows
      path: |
        selenium-manager-windows.exe
        selenium-manager-windows.dll
      retention-days: 6

In Release Job

      linux_lib_sha=$(shasum -a 256 artifacts/selenium-manager-linux/selenium-manager-linux.so | awk '{print $1}')
      macos_lib_sha=$(shasum -a 256 artifacts/selenium-manager-macos/selenium-manager-macos.dylib | awk '{print $1}')
      windows_lib_sha=$(shasum -a 256 artifacts/selenium-manager-windows/selenium-manager-windows.dll | awk '{print $1}')

      echo "{\"macos\": \"$macos_sha\", \"macos_lib\": \"$macos_lib_sha\", \"windows\": \"$windows_sha\", \"windows_lib\": \"$windows_lib_sha\", \"linux\": \"$linux_sha\", \"linux_lib\": \"$linux_lib_sha\"}" > latest.json


      files: |
        artifacts/selenium-manager-linux/selenium-manager-linux
        artifacts/selenium-manager-linux/selenium-manager-linux.so
        artifacts/selenium-manager-macos/selenium-manager-macos
        artifacts/selenium-manager-macos/selenium-manager-macos.dylib
        artifacts/selenium-manager-windows/selenium-manager-windows.exe
        artifacts/selenium-manager-windows/selenium-manager-windows.dll
        artifacts/selenium-manager-linux-debug/selenium-manager-linux-debug.tar
        artifacts/selenium-manager-macos-debug/selenium-manager-macos-debug.tar
        artifacts/selenium-manager-windows-debug/selenium-manager-windows-debug.exe

titusfortner · 2025-03-09T22:26:44Z

Doing more research...

pub use ffi --> this is only needed if we expect other rust applications to need access. We can add it later if we need it, but it isn't necessary so long as ffi file is compiled.
Structs --> it looks like it is an especially good fit for .NET, but I'm not very elegant for other languages due to how optional values must be handled. Not important right now, but we will probably want a second rust method in ffi.rs that converts JSON to the struct when we implement in other languages and just take the small serialization performance hit.
Configurations --> We probably need to make ManagerConfig the source of truth (merge it with Cli struct), so main is like:

use clap::Parser;
use selenium_manager::{ManagerConfig, manage_selenium, SeleniumPaths};
use std::process::exit;

fn main() {
    let config = ManagerConfig::parse();

    match manage_paths(config) {
        Ok(SeleniumPaths { driver_path, browser_path }) => {
            exit(0);
        }
        Err(e) => {
            eprintln!("Error: {}", e);
            exit(1);
        }
    }
}

ffi is like:

use selenium_manager::{ManagerConfig, manage_selenium, SeleniumPaths};

#[no_mangle]
pub extern "C" fn run_selenium_manager(config: ManagerConfig) -> i32 {
    match manage_paths(config) {
        Ok(SeleniumPaths { driver_path, browser_path }) => {
            0
        }
        Err(_) => {
            1
        }
    }
}

Then everything that is in main now needs to get put into manage_paths.
We could break out logging and clear cache/metadata if we want to as well.

nvborisenko added 2 commits March 1, 2025 16:45

Build shared library

4c944e3

Define cargo targets

69e5405

qodo-merge-pro bot added the Review effort 2/5 label Mar 3, 2025

nvborisenko marked this pull request as draft March 3, 2025 19:22

nvborisenko requested a review from bonigarcia March 3, 2025 19:22

nvborisenko commented Mar 3, 2025

View reviewed changes

Added example of Log Callback

775ab44

nvborisenko added 2 commits March 7, 2025 17:26

Example how to free fn result

15559d0

Verify memory leakage for log events

eb287c1

nvborisenko added 2 commits March 7, 2025 22:14

Handle panic

1205f52

Intentional panic

5c06ba7

nvborisenko commented Mar 7, 2025

View reviewed changes

rust/src/lib.rs Outdated Show resolved Hide resolved

Move extern function to ffi.rs

6d4be86

nvborisenko commented Mar 7, 2025

View reviewed changes

Simplify exporting of ffi module

0bc4ab4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[rust] Publish Selenium Manager as native shared library #15368

[rust] Publish Selenium Manager as native shared library #15368

nvborisenko commented Mar 3, 2025 •

edited

Loading

qodo-merge-pro bot commented Mar 3, 2025

qodo-merge-pro bot commented Mar 3, 2025 •

edited

Loading

nvborisenko Mar 3, 2025

bonigarcia Mar 3, 2025

titusfortner commented Mar 5, 2025

nvborisenko commented Mar 5, 2025

nvborisenko commented Mar 5, 2025

titusfortner commented Mar 6, 2025 •

edited

Loading

titusfortner commented Mar 6, 2025

nvborisenko commented Mar 6, 2025

titusfortner commented Mar 6, 2025

nvborisenko commented Mar 6, 2025

nvborisenko commented Mar 6, 2025

titusfortner commented Mar 6, 2025

nvborisenko commented Mar 7, 2025

nvborisenko commented Mar 7, 2025 •

edited

Loading

nvborisenko commented Mar 7, 2025

nvborisenko Mar 7, 2025

bonigarcia Mar 12, 2025

nvborisenko commented Mar 7, 2025

titusfortner commented Mar 9, 2025

titusfortner commented Mar 9, 2025

[rust] Publish Selenium Manager as native shared library #15368

Are you sure you want to change the base?

[rust] Publish Selenium Manager as native shared library #15368

Conversation

nvborisenko commented Mar 3, 2025 • edited Loading

User description

Motivation and Context

Checklist

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

qodo-merge-pro bot commented Mar 3, 2025

PR Reviewer Guide 🔍

qodo-merge-pro bot commented Mar 3, 2025 • edited Loading

PR Code Suggestions ✨

nvborisenko Mar 3, 2025

Choose a reason for hiding this comment

bonigarcia Mar 3, 2025

Choose a reason for hiding this comment

titusfortner commented Mar 5, 2025

nvborisenko commented Mar 5, 2025

nvborisenko commented Mar 5, 2025

titusfortner commented Mar 6, 2025 • edited Loading

titusfortner commented Mar 6, 2025

nvborisenko commented Mar 6, 2025

titusfortner commented Mar 6, 2025

nvborisenko commented Mar 6, 2025

nvborisenko commented Mar 6, 2025

titusfortner commented Mar 6, 2025

nvborisenko commented Mar 7, 2025

nvborisenko commented Mar 7, 2025 • edited Loading

nvborisenko commented Mar 7, 2025

nvborisenko Mar 7, 2025

Choose a reason for hiding this comment

bonigarcia Mar 12, 2025

Choose a reason for hiding this comment

nvborisenko commented Mar 7, 2025

titusfortner commented Mar 9, 2025

titusfortner commented Mar 9, 2025

nvborisenko commented Mar 3, 2025 •

edited

Loading

qodo-merge-pro bot commented Mar 3, 2025 •

edited

Loading

titusfortner commented Mar 6, 2025 •

edited

Loading

nvborisenko commented Mar 7, 2025 •

edited

Loading