CI: https://ci.nodejs.org/job/node-test-pull-request/25999/

@devsnek This PR changed a bit since I reviewed … shouldn’t HostCleanupFinalizationGroupCallback register a task to run the finalization hooks? Is it okay that the remainder of the list is not processed if there’s an exception from it?

@addaleax all the behaviour is at least within the specification. we can definitely tune it more as people start using weakrefs.

@devsnek Yeah, but then again so is not running the finalization calllbacks at all, as I understand it 🙃 I’m happy to provide a PR myself, but that this doesn’t run callbacks scheduled from a regular GC (i.e. not global.gc();) seems like a bug in this to me.

@addaleax I'm not sure what change you're looking for exactly

@devsnek As I’ve said, HostCleanupFinalizationGroupCallback should schedule a task to run on the event loop to run the finalization hooks (as e.g. the browser would do for this).

@addaleax these should run at the end of the microtask (and technically nextTick) queue, as they currently do.

@devsnek @addaleax, the behavior seems to be spec compliant and within the spirit of it.

From what I understand from the PR, if a cleanup callback throws, the rest of groups with pending finalization callbacks will simply be executed next tick. While that could cause delays, it's spec compliant and guarantees all callbacks are ultimately invoked.

I guess the only thing is the environment should avoid shutting down until all pending callbacks have been executed, if possible. I don't think anything prevents that right now.

Edit: I'll try to run my shim's test on the latest nightly.

I got a crash running my shim's test in the latest nightly:

FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope

https://github.com/mhofman/tc39-weakrefs-shim/tree/node12-experimental-weakrefs
run npm run test:node:esm:experimental

I managed to extract a reproduction. It's not simple and I'm not sure if a smaller repro is possible (I tried):

"use strict";

// Flags: --expose-gc --harmony-weak-refs

const common = require("../common");
const assert = require("assert");

const TRIGGER_CRASH = true;

function makeAsyncGc(gc, FinalizationGroup) {
    const finalizationGroup = new FinalizationGroup(holdings => {
        for (const holding of holdings) {
            holding.collected = true;
        }
    });

    return function asyncGc(target, trigger) {
        let checker = {};
        const checkerHolding = {
            collected: false,
        };
        const holding = target != null ? { collected: false } : checkerHolding;
        finalizationGroup.register(target || checker, holding, holding);
        finalizationGroup.register(checker, checkerHolding, checkerHolding);
        target = checker = undefined;
        return Promise.resolve(trigger)
            .then(() => {
                return gc();
            })
            .then(() => {
                try {
                    finalizationGroup.cleanupSome();
                    finalizationGroup.unregister(checkerHolding);
                    finalizationGroup.unregister(holding);
                } catch (err) {}
                if (!checkerHolding.collected) {
                    return Promise.reject(
                        new Error("Couldn't collect checker")
                    );
                }
                return holding.collected;
            });
    };
}

const asyncGc = makeAsyncGc(globalThis.gc, globalThis.FinalizationGroup);

let resolve;
const called = new Promise(r => {
    resolve = r;
});

let marker = {};

const fg = new globalThis.FinalizationGroup(
    common.mustCallAtLeast(i => {
        resolve();
    }, 1)
);

const collected = (function() {
    let obj = {};
    fg.register(obj, 42);
    const collected = asyncGc(obj);
    obj = undefined;
    return collected;
})();

collected
    .then(() => {
        globalThis.gc();
    })
    .then(() => called)
    .then(() => {
        if (!TRIGGER_CRASH) return;
        const collected = asyncGc(marker);
        marker = undefined;
        return collected;
    })
    .then(common.mustCall());

The crash seem to be a combination of resolving a promise in the callback and triggering a new gc() in the resolution chain of that promise.

@mhofman would you mind running lldb and seeing where the backtrace is from?

I don't have much of a native dev environment setup. I basically got the nightly running by building a the alpine-node docker image pulling in the nightly source instead of dist.

Looks like lldb isn't available on alpine 3.9. I'll see what I can do, but it might be faster to run the test I provided. I made sure it could run as part of node's test suite.

@devsnek, here is what I got running the test file above on the latest node 13.0.1:

node --expose-gc --harmony-weak-refs test/parallel/test-finalization-group-resolve.js 
FATAL ERROR: v8::HandleScope::CreateHandle() Cannot create a handle without a HandleScope
 1: 0x9e8320 node::Abort() [node]
 2: 0x9e9506 node::OnFatalError(char const*, char const*) [node]
 3: 0xb4310a v8::Utils::ReportApiFailure(char const*, char const*) [node]
 4: 0xcb86ba v8::internal::HandleScope::Extend(v8::internal::Isolate*) [node]
 5: 0xb448a1 v8::HandleScope::CreateHandle(v8::internal::Isolate*, unsigned long) [node]
 6: 0x98efac node::Environment::RunWeakRefCleanup() [node]
 7: 0x957c9a node::InternalCallbackScope::~InternalCallbackScope() [node]
 8: 0xa50d8e node::PerIsolatePlatformData::RunForegroundTask(std::unique_ptr<v8::Task, std::default_delete<v8::Task> >) [node]
 9: 0xa517e2 node::PerIsolatePlatformData::FlushForegroundTasksInternal() [node]
10: 0xa5352d node::NodePlatform::DrainTasks(v8::Isolate*) [node]
11: 0xa25acb node::NodeMainInstance::Run() [node]
12: 0x9b9ea1 node::Start(int, char**) [node]
13: 0x7cdcd7929b97 __libc_start_main [/lib/x86_64-linux-gnu/libc.so.6]
14: 0x956e65  [node]
Aborted (core dumped)

(lldb) bt
* thread #1, name = 'node', stop reason = signal SIGABRT
  * frame #0: 0x00007cdcd7946e97 libc.so.6`gsignal + 199
    frame #1: 0x00007cdcd7948801 libc.so.6`abort + 321
    frame #2: 0x00000000009e8331 node`node::Abort() + 33
    frame #3: 0x00000000009e9506 node`node::OnFatalError(char const*, char const*) + 166
    frame #4: 0x0000000000b4310a node`v8::Utils::ReportApiFailure(char const*, char const*) + 58
    frame #5: 0x0000000000cb86ba node`v8::internal::HandleScope::Extend(v8::internal::Isolate*) + 218
    frame #6: 0x0000000000b448a1 node`v8::HandleScope::CreateHandle(v8::internal::Isolate*, unsigned long) + 65
    frame #7: 0x000000000098efac node`node::Environment::RunWeakRefCleanup() + 140
    frame #8: 0x0000000000957c9a node`node::InternalCallbackScope::~InternalCallbackScope() + 538
    frame #9: 0x0000000000a50d8e node`node::PerIsolatePlatformData::RunForegroundTask(std::unique_ptr<v8::Task, std::default_delete<v8::Task> >) + 174
    frame #10: 0x0000000000a517e2 node`node::PerIsolatePlatformData::FlushForegroundTasksInternal() + 354
    frame #11: 0x0000000000a5352d node`node::NodePlatform::DrainTasks(v8::Isolate*) + 173
    frame #12: 0x0000000000a25acb node`node::NodeMainInstance::Run() + 827
    frame #13: 0x00000000009b9ea1 node`node::Start(int, char**) + 305
    frame #14: 0x00007cdcd7929b97 libc.so.6`__libc_start_main + 231
    frame #15: 0x0000000000956e65 node`_start + 41

Anything else I can provide ?

no that is enough, it seems that Environment::RunWeakRefCleanup is missing a HandleScope. I was considering rewriting this based on some feedback from anna anyway, so maybe it won't be a problem.

If v8 7.8 does ends up backported to 12 LTS (#30109), it'd be great if this PR and fix could also be included.

FYI, the pending integration changes for the chromium side can be found here: https://chromium-review.googlesource.com/c/chromium/src/+/1873754

Also is there any chance #30130 might fix this issue too?

no that's unrelated

Should only land on v12.x if #30109 does.