From 868d5278d942078aab2424916efcd81b479059c9 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Tue, 19 Oct 2021 15:10:22 +0000 Subject: [PATCH 1/2] Update Embind+GC integration Updates defunct integration from #8474 to latest version of the spec that renamed FinalizationGroup -> FinalizationRegistry, and changed the callback to take one value at a time instead of an iterator. Additionally, even though this functionality is now implemented in all major browsers, it has zero guarantees about reliability, and the callback might fire very late or not fire at all and still stay spec-compliant. As such, as suggested in #11436, the integration is used to not only free the found leaked instances, but also print a warning in ASSERTIONS mode that helps developer to find the original leaked instance and encourages to .delete() it manually. Resolves #11436. --- src/embind/embind.js | 50 +++++++++++++++++++++++++++----------------- 1 file changed, 31 insertions(+), 19 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index ad6409a5e8a3e..f789cb8be447f 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -17,7 +17,7 @@ /*global typeDependencies, flushPendingDeletes, getTypeName, getBasestPointer, throwBindingError, UnboundTypeError, _embind_repr, registeredInstances, registeredTypes, getShiftFromSize*/ /*global ensureOverloadTable, embind__requireFunction, awaitingDependencies, makeLegalFunctionName, embind_charCodes:true, registerType, createNamedFunction, RegisteredPointer, throwInternalError*/ /*global simpleReadValueFromPointer, floatReadValueFromPointer, integerReadValueFromPointer, enumReadValueFromPointer, replacePublicSymbol, craftInvokerFunction, tupleRegistrations*/ -/*global finalizationGroup, attachFinalizer, detachFinalizer, releaseClassHandle, runDestructor*/ +/*global finalizationRegistry, attachFinalizer, detachFinalizer, releaseClassHandle, runDestructor*/ /*global ClassHandle, makeClassHandle, structRegistrations, whenDependentTypesAreResolved, BindingError, deletionQueue, delayFunction:true, upcastPointer*/ /*global exposePublicSymbol, heap32VectorToArray, new_, RegisteredPointer_getPointee, RegisteredPointer_destructor, RegisteredPointer_deleteObject, char_0, char_9*/ /*global getInheritedInstanceCount, getLiveInheritedInstances, setDelayFunction, InternalError, runDestructors*/ @@ -1722,38 +1722,50 @@ var LibraryEmbind = { } }, - $finalizationGroup: false, + $finalizationRegistry: false, - $detachFinalizer_deps: ['$finalizationGroup'], + $detachFinalizer_deps: ['$finalizationRegistry'], $detachFinalizer: function(handle) {}, - $attachFinalizer__deps: ['$finalizationGroup', '$detachFinalizer', + $attachFinalizer__deps: ['$finalizationRegistry', '$detachFinalizer', '$releaseClassHandle'], $attachFinalizer: function(handle) { - if ('undefined' === typeof FinalizationGroup) { + if ('undefined' === typeof FinalizationRegistry) { attachFinalizer = function (handle) { return handle; }; return handle; } - // If the running environment has a FinalizationGroup (see + // If the running environment has a FinalizationRegistry (see // https://github.com/tc39/proposal-weakrefs), then attach finalizers - // for class handles. We check for the presence of FinalizationGroup + // for class handles. We check for the presence of FinalizationRegistry // at run-time, not build-time. - finalizationGroup = new FinalizationGroup(function (iter) { - for (var result = iter.next(); !result.done; result = iter.next()) { - var $$ = result.value; - if (!$$.ptr) { - console.warn('object already deleted: ' + $$.ptr); - } else { - releaseClassHandle($$); - } - } + finalizationRegistry = new FinalizationRegistry(function (info) { +#if ASSERTIONS + console.warn(info.leakWarning); +#endif + releaseClassHandle(info.$$); }); attachFinalizer = function(handle) { - finalizationGroup.register(handle, handle.$$, handle.$$); - return handle; + var $$ = handle.$$; + var info = { $$: $$ }; +#if ASSERTIONS + // Create a warning as an Error instance in advance so that we can store + // the current stacktrace and point to it when / if a leak is detected. + // This is more useful than the empty stacktrace of `FinalizationRegistry` + // callback. + var cls = $$.ptrType.registeredClass; + info.leakWarning = new Error("Embind found a leaked C++ instance " + cls.name + " <0x" + $$.ptr.toString(16) + ">.\n" + + "We'll free it automatically in this case, but this functionality is not reliable across various environments.\n" + + "Make sure to invoke .delete() manually once you're done with the instance instead.\n" + + "Check the stacktrace of this message for the original allocation."); + if ('captureStackTrace' in Error) { + Error.captureStackTrace(info.leakWarning, cls.constructor); + } +#endif + finalizationRegistry.register(handle, info, handle); + return handle; }; detachFinalizer = function(handle) { - finalizationGroup.unregister(handle.$$); + finalizationRegistry.unregister(handle); }; return attachFinalizer(handle); }, From 9906f80b00f0e7df76f8ae159df7da72e09b06a3 Mon Sep 17 00:00:00 2001 From: Ingvar Stepanyan Date: Wed, 20 Oct 2021 14:34:11 +0000 Subject: [PATCH 2/2] Tweak Embind leak warning message --- src/embind/embind.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/embind/embind.js b/src/embind/embind.js index f789cb8be447f..d9035d788e5af 100644 --- a/src/embind/embind.js +++ b/src/embind/embind.js @@ -1740,7 +1740,7 @@ var LibraryEmbind = { // at run-time, not build-time. finalizationRegistry = new FinalizationRegistry(function (info) { #if ASSERTIONS - console.warn(info.leakWarning); + console.warn(info.leakWarning.stack.replace(/^Error: /, '')); #endif releaseClassHandle(info.$$); }); @@ -1756,7 +1756,7 @@ var LibraryEmbind = { info.leakWarning = new Error("Embind found a leaked C++ instance " + cls.name + " <0x" + $$.ptr.toString(16) + ">.\n" + "We'll free it automatically in this case, but this functionality is not reliable across various environments.\n" + "Make sure to invoke .delete() manually once you're done with the instance instead.\n" + - "Check the stacktrace of this message for the original allocation."); + "Originally allocated"); // `.stack` will add "at ..." after this sentence if ('captureStackTrace' in Error) { Error.captureStackTrace(info.leakWarning, cls.constructor); }