Skip to content
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

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

Closed
rflsouza opened this issue May 10, 2023 · 7 comments
Closed

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

rflsouza opened this issue May 10, 2023 · 7 comments
Assignees

Comments

@rflsouza
Copy link

rflsouza commented May 10, 2023

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
Copy link
Collaborator

Hi @rflsouza,

How are you monitoring the objects?

Thanks!

@rflsouza
Copy link
Author

Hi,

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

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

image

Thanks.

@ClearScriptLib
Copy link
Collaborator

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
Copy link
Author

rflsouza commented May 11, 2023

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
Copy link
Collaborator

ClearScriptLib commented May 11, 2023

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
Copy link
Author

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 added a commit that referenced this issue Jun 1, 2023
…tors (mentioned in GitHub Issue #444); fixed COM-related memory leak on .NET Framework (GitHub Issue #510); enabled multidimensional array manipulation via VBScript indexing syntax (GitHub Issue #511); improved stability on Apple Silicon devices. Tested with V8 11.4.183.17.
@ClearScriptLib
Copy link
Collaborator

Fixed in Version 7.4.2.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants