microsoft / ClearScript

A library for adding scripting to .NET applications. Supports V8 (Windows, Linux, macOS) and JScript/VBScript (Windows).
https://microsoft.github.io/ClearScript/
MIT License
1.74k stars 148 forks source link

Possible memory leak in ScriptEngine.AddCOMType (V8ScriptEngine) #510

Closed rflsouza closed 1 year ago

rflsouza commented 1 year ago

Version: Microsoft.ClearScript.V8 7.3.7 Framework: .Net Framework 4.8

I haven't found a way to unload a COM. If it does not exist manually, I believe there is a leak problem.

I made a little example code to show:

for (var i = 0; i <= 100; i++)
{
   using (var engine = new V8ScriptEngine(flags))
   {
       engine.AddCOMType("XMLHttpRequest", "MSXML2.XMLHTTP");
       engine.AddHostType("Console", typeof(Console));

       Console.WriteLine("executing script");

       engine.Execute($@"  
           Console.WriteLine(""Sample Http Request with XMLHTTP"");

           function get(url) {{
               var xhr = new XMLHttpRequest();
               xhr.open('GET', url, false);

               xhr.send();
               if (xhr.status == 200)
                   return xhr.responseText;
               throw new Error('Request failed: ' + xhr.status);
           }}                    
           Console.WriteLine(get(""https://viacep.com.br/ws/01001000/json/""));
           delete xhr;
       ");                   

        // Attempt to release the com      
       engine.CollectGarbage(true);
       engine.Dispose();

       GC.Collect();
       GC.WaitForPendingFinalizers();
   }

With the program running it is possible to monitor and see the objects being added, causing the memory problem.

MSXML2

I was using MSXML2.XMLHTTP, to have a single script to test in the browser or in my application. That's how I came across the problem.

ClearScriptLib commented 1 year ago

Hi @rflsouza,

How are you monitoring the objects?

Thanks!

rflsouza commented 1 year ago

Hi,

The fastest way is to use Process Explorer (Sysinternals).

Sysinternals - Process Explorer >> Properties of Process >> .Net Assemblies

image

Thanks.

ClearScriptLib commented 1 year ago

Hi @rflsouza,

Thanks for your help. This is a bug that affects some COM scenarios on .NET Framework only. We'll have a fix in the next release.

Thanks again!

rflsouza commented 1 year ago

Good news! looking forward to the new release :)

I'm new here in the script, is there any way to make using COM faster?

When I use it to load it takes about 4 to 6 seconds sometimes, I was even thinking of changing the use to HttpClient (HostType), because it goes in milliseconds. The only point was the compatibility of writing a script that works in several places "browser test" for example.

ClearScriptLib commented 1 year ago

Hi again,

is there any way to make using COM faster?

The bug causes both the memory leak and the performance issue. The problem is at this JavaScript line:

var xhr = new XMLHttpRequest();

After ClearScript creates the object, it must examine its type information in order to expose its members to the script engine. For a normal managed object, obtaining type information is trivial, but for a COM object it's much more complicated.

COM objects come in many varieties, with different ways of exposing their type information (if they choose to expose it at all). In this case – on .NET Framework only – ClearScript ends up calling Marshal.GetTypeForITypeInfo, which dynamically constructs a Type based on COM type information. That's a very expensive operation, but the killer is that the Type is reconstructed every time. That's the performance issue, and since Type instances aren't discardable, there's the memory leak.

We intend to fix this by caching the COM-derived Type objects. In the meantime, you can work around the problem by hiding the COM object from the script engine. One way is to hide it behind a managed facade:

public class XMLHttpRequest {
    private static readonly Type _type = Type.GetTypeFromProgID("MSXML2.XMLHTTP");
    private readonly dynamic _impl = Activator.CreateInstance(_type);
    public object open(string method, string url, bool async = true) => _impl.open(method, url, async);
    public object send(object body = null) => _impl.send(body);
    public int status => _impl.status;
    public string responseText => _impl.responseText;
    // ... add other members as required
}

Here's the program we used to verify this workaround:

public class Program {
    public static void Main() {
        for (var count = 1;; count++) {
            Test();
            using (var process = Process.GetCurrentProcess()) {
                Console.WriteLine("{0:#,#} after {1:#,#} iterations", process.PrivateMemorySize64, count);
            }
        }
    }
    private static void Test() {
        for (var i = 0; i < 100; i++) {
            using (var engine = new V8ScriptEngine()) {
                engine.AddHostTypes(typeof(XMLHttpRequest), typeof(Console));
                engine.Execute(@"
                    var xhr = new XMLHttpRequest();
                    xhr.open('GET', 'https://viacep.com.br/ws/01001000/json/', false);
                    //xhr.send();
                    //Console.WriteLine(xhr.status === 200 ? xhr.responseText : 'Request failed: ' + xhr.status);
                ");
            }
        }
    }
}

Note that we commented out the send call to avoid bombarding the ViaCEP site. That's not where the problem is anyway.

Cheers!

rflsouza commented 1 year ago

Hi,

Perfect, I'm going to do the Facade pattern performance test, so it will map better!

About making the cache it will be great, because the mapping of object information will be done only once, and in the future the need for Facede.

About the bug, I understood the problem perfectly, there are some lines to follow in the correction, such as working with Factory and controlling the list of objects. it can still track the cache implementation.

I'm waiting for the next version.

Thanks for the explanations and help!

ClearScriptLib commented 1 year ago

Fixed in Version 7.4.2.