[v6.x backport] node internals' postmortem metadata (#14901, #18530, #18653)

[mmarchini]

Backport of #14901, #18530 and #18653 to v6.x.

I had some problems to backporting the tests, and I'm not sure if the current changes on test/cctest/node_test_fixture.h are the best approach for v6.x. If someone could review the changes in this file I'd really appreciate.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[mmarchini]

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

[mmarchini]

Failures are related. I'll take a look at it later.

[joaocgreis]

I aborted a node-test-binary-windows that had been running for 8 hours, apparently just stuck on cctest. This might also be related. Since there are other failures I will not restart the job now.

[MylesBorins]

If we wanted to land this it should come as semver-minor. unsure if we want to do another minor before v6.x goes into maintenance mode

/cc @nodejs/lts

[joyeecheung]

@MylesBorins My opinion would be yes, it's worth it. This helps diagnose various kinds of bugs in v6.x should there be any when it's in maintenance mode.

[mmarchini]

I agree with @joyeecheung.

Regarding the failing tests: test/cctest/test_node_postmortem_metadata.cc was heavily inspired on test/cctest/test_environment.cc and it uses the EnvironmentTestFixture (which was created for test_environment.cc). There's no test_environment.cc in v6.x, and I'm having a hard time backporting EnvironmentTestFixture to v6.x. If anyone have some I would really appreciate: https://github.com/mmarchini/node/blob/c7040a17af9f25dbc967bad9c2ad71da34387ff8/test/cctest/node_test_fixture.h#L64-L144

Alternatively, we could remove tests for this or write simpler ones in v6.x:

TEST_F(DebugSymbolsTest, BaseObjectPersistentHandle) {
  EXPECT_EQ(nodedbg_offset_BaseObject__persistent_handle___v8_Persistent_v8_Object,
            OffsetOf(&BaseObject::persistent_handle_));
}

FWIW the metadata is working on nodejs/llnode#122 (Demo: https://asciinema.org/a/qgfHneUACrD0iGXwmFc37rXC4)

[gibfahn]

If we wanted to land this it should come as semver-minor. unsure if we want to do another minor before v6.x goes into maintenance mode

If we're going to consider this we should look at all the possible minors and make a decision based on that. We did a minor quite recently (Feb 13th), and we don't want to do a minor after we go into maintenance if we can avoid it, so we'd need to make the decision quite quickly.

I'm thinking an issue in nodejs/Release, and a Release meeting in the next week or two would be good.

[MylesBorins]

Let’s get a meeting together for next week. If we are going to do it we will need to make the next v6.x release a minor

…

On Mar 22, 2018, at 4:15 PM, Gibson Fahnestock ***@***.***> wrote: If we wanted to land this it should come as semver-minor. unsure if we want to do another minor before v6.x goes into maintenance mode If we're going to consider this we should look at all the possible minors and make a decision based on that. We did a minor quite recently (Feb 13th), and we don't want to do a minor after we go into maintenance if we can avoid it, so we'd need to make the decision quite quickly. I'm thinking an issue in nodejs/Release, and a Release meeting in the next week or two would be good. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#19323 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV3Z4tKBsIT3E9Atw0CvmQVQWR0x-ks5tg849gaJpZM4So_a8>.

[MylesBorins]

It is not looking like we are going to be doing another feature related minor on 6.x. Should we close this?

[mmarchini]

No problem, I'll close this then.