JsEngine member eventCallbacks is a self-reference and can generate memory leaks

[eric@adblockplus.org]

Problem

Class member JsEngine::eventCallbacks is a container type that uses JsValueList, which holds a JsEnginePtr back to the engine. This creates a self-reference when eventCallbacks is not empty. In that case, when an instance of JsEnginePtr goes out of scope, the reference count is still above zero and the destructor of JsEngine is not called, stranding the instance and leaking memory.

Temporary Solution

Provide a way to explicitly clear all the callbacks when the user is done with the JsEngine instance.

Permanent Solution

Change the type of eventCallbacks to use v8 Persistent objects directly, avoiding completely all the redundant engine pointers.

[eric@adblockplus.org]

After some investigation, I've removed the present issue as blocking for #3593. We still have a problem, but because the way these callbacks are used, they're not much of a problem in practice. It's still the case that it requires proper use by clients in order not to create a leak, and that such use is neither obvious nor documented.

[sergz]

I find the cause and effect in the description incorrect.

Class member JsEngine::eventCallbacks is a container type that uses JsValueList, which holds a JsEnginePtr back to the engine.
...
Permanent Solution
Change the type of eventCallbacks to use v8 Persistent objects directly, avoiding completely all the redundant engine pointers.

EventCallback is a function std::function<void(const JsValueList& params)> which accepts JsValueList as argument, so it does not keep a reference to JsEngine through JsValueList. However, the function object can keep a reference to JsEngine and there are several ways to do it, e.g.

jsEngine->SetEventCallback("someOneTimeEvent", [jsEngine](const JsValueList& params) {
  jsEngine->RemoveEventCallback("someOneTimeEvent");
});

where jsEngine is std::shared_ptr<JsEngine>. For instance, another, maybe a bit less obvious, way to do it is to add an intermediate object which keeps JsEngine, e.g.

std::shared_ptr<FilterEngine> filterEngine = ....;
filterEngine->GetJsEngine()->SetEventCallback([filterEngine](const JsValueList& params){});

One can also capture JsValueList and in this case JsEngine will be kept through JsValueList.

The proposed solution to use v8 Persistent objects maybe helps in come edge cases, when JsValue is captured (directly or indirectly via JsValueList or something else), but generally there is no relation to JsValueList and it is not an actual solution.

Therefore since a programmer can always do it somehow I would rather give recommended constraints on EventCallback in the comments in the code, namely don't keep a strong reference to JsEngine in the function closure and of course ensure that your objects are still not destroyed (#1849).

[oleksandr]

Looks like #1849 is a substitute for this one. Can we close this?

[sergz]

Replying to oleksandr:

Looks like #1849 is a substitute for this one. Can we close this?

I find them different. The key moment of #1849 is the usage of a dangling pointer but this issue is rather about circular references.

I have not closed it because I would like the following part to be addressed

Therefore since a programmer can always do it somehow I would rather give recommended constraints on EventCallback in the comments in the code, namely don't keep a strong reference to JsEngine in the function closure and of course ensure that your objects are still not destroyed (#1849).