Skip to main content
Topic: Spider Monkey Panel (foo_spider_monkey_panel) (Read 25198 times) previous topic - next topic
0 Members and 1 Guest are viewing this topic.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #225
Something like
Code: [Select]
window.TriggerSmpGcAsap()
I'll try to think of a better way to solve your problem (hoping there is one).
I (whyonearth) was banned for not really considering forum rules. I hope now I'm complying with them. Sorry for breaking the rules and for confusion with replying from new account.

Anyway, that OOM-on-repeated-script-reload error I was talking about was not an OOM error at all. Nevertheless, it's still error of resource exhaustion type.

So, I noted that there was some free memory at the time of crash. This is when I thought I might be wrong in my conclusions. Then I somehow did look more thoroughly at foobar2000 crash report and accidentally 93MB of .rar files extracted meaningful information (crash report is trimmed here):
Spoiler (click to show/hide)

So, code tried to access address zero, when last win32 error was error with code 1114 (ERROR_DLL_INIT_FAILED: A dynamic link library (DLL) initialization routine failed).

I examined AutoHotkey_H sources, then downloaded OllyDbg and tried to trace AutoHotkey DLL execution. Starting point was in DllGetClassObject. Here's what I found.
Spoiler (click to show/hide)

Relevant links:Spoiler (click to show/hide)

So, when I naively chose Win32w_MT variant of AutoHotkey.dll from release distribution, I chose statically linked library (which I didn't know at the time) and this brought some fixed limits.

Good news, in recent version of Windows 10 this limit is raised: read here and here. Bad news (for me), I'm not using the recent version of Windows 10.

So, while I can instead use dynamically linked AutoHotkey.dll (from Win32w directory), which is linked with vcruntime140.dll, just as foobar2000, and this replacement seems to fix crashes, I thought, maybe if SMP component would just unload COM DLL on unloading (i.e. reloading) SMP script, this also should fix my problem?

While, looking at the SMP code, I can find CoFreeUnusedLibraries, I believe, in real usage component doesn't always work as expected.

Trying to prove that normally used CoFreeUnusedLibraries certainly should free FLS indexes allocated by COM DLL (at least, by AutoHotkey.dll), I did some experiments:
Spoiler (click to show/hide)

So, I think this is where SMP can be improved.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #226
@thisbanisnotsettoexpire : that's quite a research you did there =)
I'm not sure if this is SMP bug, since it seems that AHK reserves new FLS on every execution, but as I said I will add some GC on panel unload.

Not sure when will it happen though, since MS released a broken Visual Studio update, which made it impossible to compile SMP (and I just can't find enough motivation to go through the whole VS reinstallation process to get the working VS back).
<rant>This f***ing s***-show makes me wonder if they even go through QA before making a release...</rant>
<rant intensifies>And don't even get me started on this f***ing disgusting compact title-bar...</rant>

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #227
@thisbanisnotsettoexpire : that's quite a research you did there =)
I'm not sure if this is SMP bug, since it seems that AHK reserves new FLS on every execution, but as I said I will add some GC on panel unload.

Not sure when will it happen though, since MS released a broken Visual Studio update, which made it impossible to compile SMP (and I just can't find enough motivation to go through the whole VS reinstallation process to get the working VS back).
<rant>This f***ing s***-show makes me wonder if they even go through QA before making a release...</rant>
<rant intensifies>And don't even get me started on this f***ing disgusting compact title-bar...</rant>
Is this VS2019 - 16.2.0 you're talking about?

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #228
As far as I can understand from sources, SMP allows to pass JS function as argument to COM object method. Passed function is wrapped in IDispatch-able gateway and this function could be invoked from COM object side by using gateway's ExecuteValue() method.

AutoHotkey_H COM object can assign value to variable in AutoHotkey script with ahkassign method. Though documentation says that only string value can be passed to that method and assigned to variable, in fact, as AutoHotkey_H sources show, CoCOMServer::ahkassign recognizes different COM types (calling AssignVariant, then VariantToToken). By passing VT_DISPATCH COM value to ahkassign, it's possible to assign AutoHotkey object to AutoHotkey script variable and then invoke IDispatch methods from AutoHotkey side by using same-named object methods.

I successfully tested this functionality of AutoHotkey_H COM object with Python script:
Spoiler (click to show/hide)

But when I used AutoHotkey_H COM object in SMP JS script in the same way as in Python script, trying to call JS function from AutoHotkey script side, it produced access violation error on ExecuteValue() call from AutoHotkey script.

JS script code:Spoiler (click to show/hide)

In tests I used following versions: Spoiler (click to show/hide)

I tried to trace SMP code with OllyDbg, found that ExecuteValue() is really called on SMP component side and (if my understanding is right) in this line
Code: [Select]
JS::RootedObject jsGlobal( pJsCtx_, heapMgr.Get( globalId_ ).toObjectOrNull() );
heapMgr.Get( globalId_ ) returned 0.

If I'm right, can it be fixed? (Just asking about possibility, not pushing to do it ASAP.)

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #229
While, looking at the SMP code, I can find CoFreeUnusedLibraries, I believe, in real usage component doesn't always work as expected.

Trying to prove that normally used CoFreeUnusedLibraries certainly should free FLS indexes allocated by COM DLL (at least, by AutoHotkey.dll), I did some experiments:

So, I think this is where SMP can be improved.
In that experiments I used CoUninitialize and it was critical for successful results, but, while it's OK to use this function in the end of execution of standalone script, now I believe (basing on CoUninitialize documentation), that using it in SMP is either not allowed at all, or at least would require cooperation between author of foobar2000, authors of all other components and author of SMP (i.e. it's impractical).

Turns out, just using CoFreeUnusedLibraries wasn't enough to produce noticable releasing of FLS indexes, because
Quote
CoFreeUnusedLibraries does not immediately release DLLs that have no active object. There is a 10-minute delay for multithreaded apartments (MTAs) and neutral apartments (NAs). For single-threaded apartments (STAs), there is no delay.

No, it's not a quote from CoFreeUnusedLibraries documentation, it's a quote from CoFreeUnusedLibrariesEx documentation. Thanks a lot, Microsoft.

CoFreeUnusedLibrariesEx is specifically provided to set explicit delay for unloading DLLs with a way to force the unloading of DLLs without any delay, though remarks in CoFreeUnusedLibrariesEx documentation warn against forcing immediate unloading of DLLs.

With using CoFreeUnusedLibrariesEx I can consistently release FLS indexes in test script without CoUninitialize:
Spoiler (click to show/hide)

The problem is, with any reasonable delay (even reasonably small delay), there probably would still be no useful change in my original SMP's usage scenario (in the concrete case of using statically linked AutoHotkey DLL), because in reloading script new AutoHotkey COM object would be created by reloaded script in a time smaller than that delay and then AutoHotkey DLL wouldn't be unloaded and allocated FLS indexes wouldn't be released and foobar2000 will still crash after repeated script reloading.

But anyway, it looks like this particular mysterious mystery of strange mystery is resolved and, with this information, maybe TheQwertiest will come up with some heuristic to use the possibility of setting COM DLLs unload delay.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #230
Is this VS2019 - 16.2.0 you're talking about?
Yup

I tried to trace SMP code with OllyDbg, found that ExecuteValue() is really called on SMP component side and (if my understanding is right) in this line
Code: [Select]
JS::RootedObject jsGlobal( pJsCtx_, heapMgr.Get( globalId_ ).toObjectOrNull() );
heapMgr.Get( globalId_ ) returned 0.

If I'm right, can it be fixed? (Just asking about possibility, not pushing to do it ASAP.)
This should not ever happen - it's a bug.
Can you try the nightly build (see OP for the link) and check if the bug persists?

PS: Thanks for the detailed repro steps!

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #231
I tried to trace SMP code with OllyDbg, found that ExecuteValue() is really called on SMP component side and (if my understanding is right) in this line
Code: [Select]
JS::RootedObject jsGlobal( pJsCtx_, heapMgr.Get( globalId_ ).toObjectOrNull() );
heapMgr.Get( globalId_ ) returned 0.
I changed debugger from OllyDbg to x64dbg (it has 32-bit version and can debug 32-bit code, despite the name) and, coincidentally, place of error also changed. I didn't try to debug with OllyDbg again, so I can't say whether I was just plainly wrong in quoted bug report, or there was some misunderstanding on my side. However, after several attempts at debugging with x64dbg, I successfully found bugfix.

I patched foo_spider_monkey_panel v1.2.1 DLL at following offsets (from the beginning of file):
OffsetOriginal valuePatched value
0x3F7790xFF0x90
0x3F77A0xD70x90

Originally I used runtime patching ability of x64dbg and exported patch in .1337 format. It looks like offsets in this patch are not from the beginning of the file, so it wasn't useful for me per se, but for completeness I paste it here:Spoiler (click to show/hide)

Original values are assembled as CALL EDI instruction and patch replaces it with NOPs.

In terms of C++ sources, patch removes this line in ActiveXObject::Invoke method:
Code: [Select]
VariantClear( &args[i] );

After patching, I can execute JS script from quoted post without any errors, but with producing expected result of appending an entry to foobar2000 console. As ExecuteValue method is the default member of SMP's COM object (i.e. it is identified by DISPID_VALUE), it can also be invoked from AutoHotkey side by using following syntax: Spoiler (click to show/hide)

If I'm right, can it be fixed? (Just asking about possibility, not pushing to do it ASAP.)
This should not ever happen - it's a bug.
Can you try the nightly build (see OP for the link) and check if the bug persists?
After replacing foo_spider_monkey_panel v1.2.1 to v1.2.2-beta+8a02533a I observe Access Violation error on the ahk.ahkExec('f.ExecuteValue()'); line. Though I can't say that it's the same bug. In absence of .pdb for nightly and having working stable DLL, I didn't try to debug nightly to find the cause of error.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #232
@thisbanisnotsettoexpire : that's some extensive debugging!
It's really weird though that `VariantClear` causes errors: it just cleans up arguments that were generated for function call (and it should be safe for well-formed COM types). Not doing this will cause memory leaks.

Nevertheless, I can't test nor debug this (nor continue the component development) since I still don't have a working Visual Studio installation: current version (16.2) is broken and I couldn't find a way to download the previous release...

PS: Corresponding .pdb symbols (as well as a debug build) are available on the appveyor agent - https://ci.appveyor.com/project/TheQwertiest/foo-spider-monkey-panel/builds/26070987/job/nxn00otulu6tyrl7/artifacts

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #233
It's really weird though that `VariantClear` causes errors: it just cleans up arguments that were generated for function call (and it should be safe for well-formed COM types). Not doing this will cause memory leaks.
Thanks for hint. I guess then it's working, but wrong solution. VariantClear:
Quote
clears a VARIANTARG by setting the vt field to VT_EMPTY. The current contents of the VARIANTARG are released first. [...] If the vtfield is VT_DISPATCH, the object is released.

So I've took an attempt to come up with solution from other side and patched AutoHotkey_H DLL v2.0-a100-H104:
Spoiler (click to show/hide)

It changes 0x6A 0x00 to 0x6A 0x01, i.e. PUSH 0 to PUSH 1.

In terms of original source, it modifies function CoCOMServer::ahkassign by changing argument false in function call to true:
Code: [Select]
AssignVariant(*var, value, false);

It means that AssignVariant is called with argument aRetainVar = true, then VariantToToken is called with argument aRetainVar = true, then reference count for an interface pointer to a COM object is incremented.

I'm not sure that it's correct bugfix (is AutoHotkey_H really must be responsible for incrementing reference counter?), but it also works with Spider Monkey Panel v1.2.2-beta+8a02533a.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #234
After some incredible shenanigans I've managed to get myself a working VS2019 16.1 installation (no thanks to Microsoft)!
Anyways...

@thisbanisnotsettoexpire : I've managed to reproduce your crash - it seems that the COM object passed to `ahkassign` does not have it's counter incremented.
Code: [Select]
...
function f();
...
ahk.assign('f', f);
...
SMP wraps function `f` in a COM object and passes it to `ahk.assign`.
So, we have the following scenario:
- COM object is created from JS object. It has a usage counter equal to 1.
- COM object is passed to ahk.assign. Usage counter was not incremented in ahk.assign.
- Local (to SMP) COM object is destroyed. Usage counter is decremented to zero thus the VARIANT is destroyed as well.
- If ahk tries to use said (destroyed) COM object it will invoke UB and most likely crash.

Quote
    is AutoHotkey_H really must be responsible for incrementing reference counter?
Yes, because the caller does not how the passed object will be utilized. COM object handling is the same as shared_ptr.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #235
@thisbanisnotsettoexpire : I've tested with the debug build of AHK and my guess was proven true - AHK assign does not increase ref count. This is the code which assigns my COM object to internal ASH variable: https://github.com/HotKeyIt/ahkdll/blob/7e6d355db20d6faf59d8a15fcc908b52ff7b5d80/source/script_com.cpp#L731
There is no reference increments going on unless aRetainVar was set to true (which is not the case for `ahkassign`).

[EDIT] Moreover, it seems that `ahkassign` will actually DECREASE refcount of the incoming object WITHOUT incrementing it in some cases (invoking UB if the caller didn't increase refcount) : e.g. https://github.com/HotKeyIt/ahkdll/blob/7e6d355db20d6faf59d8a15fcc908b52ff7b5d80/source/script_com.cpp#L698
This code is flawed and I can't find any sensible scenario when it would be a correct approach. Your AHK patch *is* the correct fix.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #236
Well f**k... VS 2019 was updated to 16.2 on the build agent, so now SMP can't be built there... And there is still no answer from Microsoft on this issue...

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #237
It's not an easy problem. You can try to look at the code of the Biography script by Wilb, there, it calls a VBScript in order to download images (well his VBScript can actually download any kind of file).

Thank you Ottidix and TheQwertiest. It works great. It was finally quite simple but I did no get the idea by myself !
Another question ; I have an error 'can't access dead object' !
Any suggestion on why this error is raised ?
I will check the livecycle of my object, but if you have any clue (I reset SMP parameters in 'Advanced / Tools / SMP settings, but not sufficient)

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #238
Another question ; I have an error 'can't access dead object' !
Any suggestion on why this error is raised ?

It completely depend of your code, there is a lot of possible scenarios. One common mistake : if you send a object from another panel using window.NotifyOthers(name,info), you need to clone the object inside the corresponding callback on_notify_data(name,info) of the receiver panel, you can't just assign by reference to another variable. Because the "info" object is destructed at the end of this function.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #239
@cerbaire , Ottodix's suggestion is entirely correct. You can find more info about it in the documentation of `on_notify_data` callback (you can access HTML documentation via 'Configure...'>'Help'>'View Help').

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #240
@cerbaire , Ottodix's suggestion is entirely correct. You can find more info about it in the documentation of `on_notify_data` callback (you can access HTML documentation via 'Configure...'>'Help'>'View Help').

Thanks for your responses. I understand the problem, and I think I am not in the scenario of a on_notify_data call.
My object is a FbMetadbHandle stored in a list. I will check on which event I am binded, but I think I instantiate my object directly in the panel script so it is not binded to any documented SMP event. I check that tonight and let you know.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #241
Then it's really impossible to help without the code, you can debug it yourself though. You can use a function like:

Code: [Select]
function debug_dead_item(metadb_list,index){
    console.log("desired index: "+index+", list length: "+metadb_list.length);
    console.log(metadb_list[index].RawPath);
}

Put checkpoints in your code with this function in order to trace when/where this object was available and when/where it's not anymore, and then you'll probably understand what's going on. This function will crash if the list or if the specified index doesn't exist, but it doesn't matter, the crash message will be as good an information than the console messages.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #242
Thanks for your responses. I understand the problem, and I think I am not in the scenario of a on_notify_data call.
My object is a FbMetadbHandle stored in a list. I will check on which event I am binded, but I think I instantiate my object directly in the panel script so it is not binded to any documented SMP event. I check that tonight and let you know.
The problem might be delayed: e.g.
Code: [Select]
// Pseudo code

let arr = []
on_notify_data(data) {
  // no error
  arr.push_back(data);
}

on_mouse_click() {
  // error
  console.log(arr[0].Path);
}
On another note: what SMP version are you using?

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #243
Thanks for your responses. I understand the problem, and I think I am not in the scenario of a on_notify_data call.
My object is a FbMetadbHandle stored in a list. I will check on which event I am binded, but I think I instantiate my object directly in the panel script so it is not binded to any documented SMP event. I check that tonight and let you know.
The problem might be delayed: e.g.
Code: [Select]
// Pseudo code

let arr = []
on_notify_data(data) {
  // no error
  arr.push_back(data);
}

on_mouse_click() {
  // error
  console.log(arr[0].Path);
}
On another note: what SMP version are you using?

I am using last nightly build available.
In the panel code, I call GetPlaylistItems(-1) in a setTimeout() and store index and FbMetadbHandle returned by GetPlaylistItems. Later, in a on_mouse_lbtn_up, I use the previous stored FbMetadbHandle, and it fail.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #244
I am using last nightly build available.
In the panel code, I call GetPlaylistItems(-1) in a setTimeout() and store index and FbMetadbHandle returned by GetPlaylistItems. Later, in a on_mouse_lbtn_up, I use the previous stored FbMetadbHandle, and it fail.
That's really weird. Can you provide a minimal repro scenario? So that I could reproduce it locally.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #245
I am using last nightly build available.
In the panel code, I call GetPlaylistItems(-1) in a setTimeout() and store index and FbMetadbHandle returned by GetPlaylistItems. Later, in a on_mouse_lbtn_up, I use the previous stored FbMetadbHandle, and it fail.
That's really weird. Can you provide a minimal repro scenario? So that I could reproduce it locally.
You're right ! I was storing FbMetadbHandle in a function called from on_notify_data, and using that handle after the callback end.
I replace the handle by the RawPath property and it works better!
Thank you for putting me in the right direction.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #246
You're right ! I was storing FbMetadbHandle in a function called from on_notify_data, and using that handle after the callback end.
I replace the handle by the RawPath property and it works better!
Thank you for putting me in the right direction.
No! You are using it wrong! Please, read documentation for the `on_notify_data`...
Your usage (for both FbMetadbHandle and FbMetadbHandle.RawPath) is covered explicitly in the method description.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #247
You're right ! I was storing FbMetadbHandle in a function called from on_notify_data, and using that handle after the callback end.
I replace the handle by the RawPath property and it works better!
Thank you for putting me in the right direction.
No! You are using it wrong! Please, read documentation for the `on_notify_data`...
Your usage (for both FbMetadbHandle and FbMetadbHandle.RawPath) is covered explicitly in the method description.
Right! I now use JSON.parse(JSON.stringify(metadb)) to store a copy of my handle and it works lke a charm. Thanks.

Re: Spider Monkey Panel (foo_spider_monkey_panel)

Reply #248
Right! I now use JSON.parse(JSON.stringify(metadb)) to store a copy of my handle and it works lke a charm. Thanks.
Uhm... no...

Code: [Select]
...
 If you want to store the data from `info` you have to perform a deep copy:
- `String(info)` for strings.
...
So if you want to store `metadb.RawPath`, you store it like this:
Code: [Select]
// Pseudo code
g_storage = []
on_notify_data(metadb)
{
  // `String` performs a deep copy
  g_storage.push_back(String(metadb.RawPath))
}

You can't serialize `metadb` since it's not a serializable object (i.e. it will be restored into a simple struct instead after deserialization).
To clone metadb you'll have to clone metadbhandlelist instead (metadb does not have a copy constructor):
Code: [Select]
// Pseudo code
g_storage = []
on_notify_data(metadb)
{
  // FbMetadbHandleList constructor always performs a deep copy
  let tmpHandleList = new FbMetadbHandleList(metadb);
  g_storage.push_back(tmpHandleList[0]);
}

 
SimplePortal 1.0.0 RC1 © 2008-2019