OutSystems / CefGlue

.NET binding for The Chromium Embedded Framework (CEF)
MIT License
277 stars 41 forks source link

CefGlue throws NullRefException #180

Open adcorduneanu opened 2 weeks ago

adcorduneanu commented 2 weeks ago

Hi,

I recently attempted to update our version of Xilium.CefGlue from CEF 102 to the latest release from this repository. Unfortunately, this has introduced some significant issues with our application's callback handling.

Our application relies on registered callbacks, shared across multiple tabs, to handle various operations such as inter-process communication (IPC), application updates, network signal listeners, and more.

We implement these callbacks by extending CefV8Handler and registering functions that take a JavaScript function as an argument. Once specific business conditions are met, these callbacks are triggered from our C# code.

However, after the update, I noticed that all objects using these contexts, including the registered callbacks and execution contexts, are unexpectedly disposed of. This behavior is breaking our functionality.

After investigating, I didn’t find any related issues in this repository, but I did come across two similar issues on Xilium's GitLab page:

Issue #102 – This closely resembles what we are experiencing: GitLab Issue #102

Issue #88 – This provides further context and may be the root cause of our problem: GitLab Issue #88 Unfortunately, this issue remains unresolved.

Additionally, it appears that this regression is linked to the following commit, which may have introduced the side effects we’re observing: Commit 0d922bcd165e54bc991970f6d441a91ec4594a89

Any assistance or guidance on resolving this would be greatly appreciated.

Thank you in advance for your attention to this matter!

joaompneves commented 2 weeks ago

Hi,

We've been consistently using this code in our product for long time (several years), without any relevant issues. The mentioned changes aimed at resolving memory leaks that were caused by objects not being disposed, once they are not needed anymore. Usually, in normal circumstances, these objects are short lived, and the Object tracker, gets rid (disposes) of them once the cef callback finishes executing. I don't know the details of your scenario, but it might be the case that you're grabbing references for something you shouldn't, your beyond the point that you should do, which might lead to memory leaks.

adcorduneanu commented 2 weeks ago

I'll try to add some context to clarify my use case, which I imagine may differ from others.

My approach uniquely leverages CEF Glue's native capabilities, using native code to inject JavaScript functions that execute when network events change, for example.

I've attached code samples that closely resemble our implementation.

The issue arises in PublishConnectionState - specifically, callbackContext and onNetworkChangeCallback are already being disposed, even though they should persist as long-running tasks.

At one point, I attempted to prevent automatic disposal by using Untrack on callbackContext and onNetworkChangeCallback during callback registration, but this didn't resolve the issue.

If I comment out calls to StartTracking, StopTracking, Track, and Untrack in CefObjectTracker, the callbacks work as expected. However, this introduces a memory management issue.

const onConnectionStateChanged = (signalStrength, hasWifiConnection, ssid, localIpAddress) => {
    // some javascript logic here
};

registerConnectionStateListener(onConnectionStateChanged); // register the onConnectionStateChanged on the registerConnectionStateListener, so when a network event is triggered the javascript is notified
internal class InvokeJavascriptMethodTask : CefTask
{
    private CefV8Context callbackContext;
    private CefV8Value method;
    private object[] args;

    public InvokeJavascriptMethodTask(CefV8Context callbackContext, CefV8Value method, params object[] args)
    {
        this.callbackContext = callbackContext;
        this.method = method;
        this.args = args;
    }

    protected override void Execute()
    {
        try
        {
            List<CefV8Value> valueArgs = new List<CefV8Value>();
            foreach (object arg in args)
            {
                if (arg == null)
                    valueArgs.Add(CefV8Value.CreateNull());
                else if (arg is bool)
                    valueArgs.Add(CefV8Value.CreateBool((bool)arg));
                else if (arg is string)
                    valueArgs.Add(CefV8Value.CreateString((string)arg));
                else if (arg is DateTime)
                    valueArgs.Add(CefV8Value.CreateDate(new CefBaseTime(((DateTime)arg).Ticks)));
                else if (arg is double)
                    valueArgs.Add(CefV8Value.CreateDouble((double)arg));
                else if (arg is int)
                    valueArgs.Add(CefV8Value.CreateInt((int)arg));
                else if (arg is float)
                    valueArgs.Add(CefV8Value.CreateDouble((double)(float)arg));
            }

            CefV8Value result = method.ExecuteFunctionWithContext(callbackContext, null, valueArgs.ToArray());
            if (result == null)
            {
                // Incorrect method call, or exception occurred
            }
        }
        catch (Exception ex)
        {
            LogManager.GetCurrentClassLogger().Error(ex);
        }
    }
}

public class NetworkMonitorHandler : CefV8Handler
{
    private CefV8Context callbackContext;
    private bool networkChangeEventsAttached;
    private CefV8Value onNetworkChangeCallback;
    private WlanApiClient wlanClient;

    public void Cleanup()
    {
        try
        {
            if (wlanClient != null)
            {
                wlanClient.SignalQualityChanged -= wlanClient_SignalQualityChanged;
                wlanClient.Dispose();
            }

            if (networkChangeEventsAttached)
            {
                NetworkChange.NetworkAvailabilityChanged -= NetworkChange_NetworkAvailabilityChanged;
                NetworkChange.NetworkAddressChanged -= NetworkChange_NetworkAddressChanged;
            }

            callbackContext?.Dispose();
        }
        catch
        {
            // logger here
        }
    }

    protected override bool Execute(
        string name,
        CefV8Value obj,
        CefV8Value[] arguments,
        out CefV8Value returnValue,
        out string exception
    )
    {
        if (arguments.Length == 1 && arguments[0].IsFunction)
        {
            onNetworkChangeCallback = arguments[0];
            callbackContext = CefV8Context.GetCurrentContext();

            StartMonitoringConnectionState();

            returnValue = CefV8Value.CreateBool(true);
            exception = null;
            return true;
        }

        returnValue = CefV8Value.CreateBool(false);
        exception = null;
        return false;
    }

    private string GetIpAddress()
    {
         return "some IP here";
    }

    private void NetworkChange_NetworkAddressChanged(object sender, EventArgs e)
    {
        try
        {
            PublishConnectionState();
        }
        catch (Exception ex)
        {
            // logger here
        }
    }

    private void NetworkChange_NetworkAvailabilityChanged(object sender, NetworkAvailabilityEventArgs e)
    {
        try
        {
            PublishConnectionState();
        }
        catch (Exception ex)
        {
            // logger here
        }
    }

    private void PublishConnectionState()
    {
        var localIpAddress = GetIpAddress();
        var state = wlanClient.GetState();

        // we didn't changed this code since 10 years ago
        // at this point the callbackContext, and onNetworkChangeCallback are disposed, which is root of our issues

        if (state != null)
        {
            callbackContext.GetTaskRunner().PostTask(
                new InvokeJavascriptMethodTask(
                    callbackContext,
                    onNetworkChangeCallback,
                    state.SignalStrength,
                    state.HasWifiConnection,
                    state.Ssid,
                    localIpAddress
                )
            );
        }
        else
        {
            var hasConnection = !string.IsNullOrEmpty(localIpAddress);
            state = new WlanClientState
            {
                HasWifiConnection = hasConnection,
                SignalStrength = hasConnection ? 100 : 0,
                Ssid = string.Empty
            };
            callbackContext.GetTaskRunner().PostTask(
                new InvokeJavascriptMethodTask(
                    callbackContext,
                    onNetworkChangeCallback,
                    state.SignalStrength,
                    state.HasWifiConnection,
                    state.Ssid,
                    localIpAddress
                )
            );
        }
    }

    private void StartMonitoringConnectionState()
    {
        wlanClient = new WlanApiClient();
        wlanClient.Open();

        wlanClient.SignalQualityChanged += wlanClient_SignalQualityChanged;
        NetworkChange.NetworkAddressChanged += NetworkChange_NetworkAddressChanged;
        NetworkChange.NetworkAvailabilityChanged += NetworkChange_NetworkAvailabilityChanged;

        PublishConnectionState();
        networkChangeEventsAttached = true;
    }

    private void wlanClient_SignalQualityChanged(object sender, SignalQualityChangedEventArgs e)
    {
        try
        {
            PublishConnectionState();
        }
        catch (Exception ex)
        {
            // logger here
        }
    }
}

public sealed class CustomRenderProcessHandler : CefRenderProcessHandler
{
    private NetworkMonitorHandler networkMonitorHandler;

    protected override void OnContextCreated(CefBrowser browser, CefFrame frame, CefV8Context context)
    {
        try
        {
            if (!context.Enter()) return;

            using (CefV8Value global = context.GetGlobal())
            {
                networkMonitorHandler = new NetworkMonitorHandler();
                global.SetValue("registerConnectionStateListener",
                    CefV8Value.CreateFunction("registerConnectionStateListener", networkMonitorHandler),
                    CefV8PropertyAttribute.None);
            }

            base.OnContextCreated(browser, frame, context);
        }
        catch (Exception ex)
        {
            // logger here
        }
        finally
        {
            context.Exit();
        }
    }

    protected override void OnContextReleased(CefBrowser browser, CefFrame frame, CefV8Context context)
    {
        if (browser.IsPopup || browser.FrameCount > 1)
        {
            base.OnContextReleased(browser, frame, context);
            return;
        }

        try
        {
            networkMonitorHandler?.Cleanup();
            base.OnContextReleased(browser, frame, context);
        }
        catch (Exception ex)
        {
            // logger here
        }
    }
}
joaompneves commented 2 weeks ago

Well, I think that what you're doing can be simplified alot with CefGlue existing capabilities.

Create a dotnet object that exposes the native functionality and call that method from the JS side. Its a pooling mechanism but without an active wait, because it relies on the promise infra-structure.

Suggestion:


class ConnectionState
{
    public int Signal;
    public string Ip;
}

class NetworkConnectionChangeHandler
{
    private TaskCompletionSource<ConnectionState> tcs = new();

    public async Task<ConnectionState> CheckConnectionStateChanges()
    {
        try
        {
            return await tcs.Task;
        }
        finally
        {
            tcs = new();
        }
    }

    public void SignalChanges(ConnectionState state) => tcs.SetResult(state);
}

...

const string DotnetObjectName = "ConnectionStateMonitor";

var handler = new NetworkConnectionChangeHandler();
browser.RegisterJavascriptObject(handler, DotnetObjectName);

browser.ExecuteJavaScript(
        "window['registerNetworkConnectionStateListener'] = (function(dotnetHandler) {" +
        "    const callbacks = [];" +
        "    async function startMonitoring() {" +
        "        console.log('will start monitoring');" +
        "        while(true) {" +
        "            const details = await window[dotnetHandler].checkConnectionStateChanges();" +
        "            callbacks.forEach(c => c(details)); " +
        "        }" +
        "    }" +
        "    return function(callback) { " +
        "        if (callbacks.length === 0) startMonitoring();" +
        "        callbacks.push(callback); " +
        "    };" +
        $"}})('{DotnetObjectName}');");

Task.Run(async () =>
{
    await Task.Delay(1000);
    browser.ExecuteJavaScript("registerNetworkConnectionStateListener(d => console.log(d))");
    await Task.Delay(1000);
    handler.SignalChanges(new ConnectionState() { Signal = 50, Ip = "127.0.0.1" });
    await Task.Delay(1000);
    handler.SignalChanges(new ConnectionState() { Signal = 100, Ip = "127.0.0.1" });
});