src: release context frame in AsyncWrap::EmitDestroy

[Flarna]

Release the async context frame in AsyncWrap::EmitDestroy to allow gc to collect it.

This is in special relevant for reused resources like HTTPParser otherwise they might keep ALS stores alive.

Fixes: #61882
Refs: #48528

[codecov]

Codecov Report

❌ Patch coverage is 85.71429% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 89.73%. Comparing base (488a854) to head (c458399).
⚠️ Report is 14 commits behind head on main.

Files with missing lines	Patch %	Lines
src/async_wrap.cc	85.71%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #61995      +/-   ##
==========================================
- Coverage   89.77%   89.73%   -0.05%     
==========================================
  Files         674      676       +2     
  Lines      205705   206074     +369     
  Branches    39449    39514      +65     
==========================================
+ Hits       184670   184918     +248     
- Misses      13280    13311      +31     
- Partials     7755     7845      +90     

Files with missing lines	Coverage Δ
src/async_wrap.cc	84.30% <85.71%> (-0.11%)	⬇️

... and 54 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[addaleax]

Not for this PR, but do you know how it comes this is a v8::Global to begin with and not an internal field on object()?

[Flarna]

Sorry, don't know. Maybe @Qard can chime in.

[addaleax]

Sorry, just saw your comments on the DCHECK (hidden because of the resolved comment) – that's exactly why the DCHECK should be in place, we should not be able to MakeCallback() when there isn't a corresponding context frame to be entered.

I can try to find some time to dig into those failures.

[nodejs-github-bot]

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

[addaleax]

Yeah, the only error I'm getting is this one:

Path: parallel/test-http-req-close-robust-from-tampering
#  out/Release/node[96212]: MaybeLocal<Value> node::AsyncWrap::MakeCallback(const Local<Function>, int, Local<Value> *) at ../src/async_wrap.cc:665
  #  Assertion failed: !context_frame_.IsEmpty()

----- Native stack trace -----

 1: 0x104c41c8c node::DumpNativeBacktrace(__sFILE*) [[...]/src/node/out/Release/node]
 2: 0x104dbf8b8 node::Assert(node::AssertionInfo const&) [[...]/src/node/out/Release/node]
 3: 0x104bb7630 node::AsyncWrap::MakeCallback(v8::Local<v8::Function>, int, v8::Local<v8::Value>*) [[...]/src/node/out/Release/node]
 4: 0x104dfe5f0 node::http_parser::Parser::on_body(char const*, unsigned long) [[...]/src/node/out/Release/node]
 5: 0x104df637c node::http_parser::Parser::Proxy<int (node::http_parser::Parser::*)(char const*, unsigned long), &node::http_parser::Parser::on_body(char const*, unsigned long)>::Raw(llhttp__internal_s*, char const*, unsigned long) [[...]/src/node/out/Release/node]
 6: 0x106ba6300 llhttp__on_body [[...]/src/node/out/Release/node]
 7: 0x106ba3f68 llhttp__internal__run [[...]/src/node/out/Release/node]
 8: 0x106b9e160 llhttp__internal_execute [[...]/src/node/out/Release/node]
 9: 0x104dfb064 node::http_parser::Parser::Execute(char const*, unsigned long) [[...]/src/node/out/Release/node]
10: 0x104dfa188 node::http_parser::Parser::OnStreamRead(long, uv_buf_t const&) [[...]/src/node/out/Release/node]
11: 0x104ccc25c node::StreamResource::EmitRead(long, uv_buf_t const&) [[...]/src/node/out/Release/node]
12: 0x105033a58 node::LibuvStreamWrap::OnUvRead(long, uv_buf_t const*) [[...]/src/node/out/Release/node]
13: 0x105034ba0 node::LibuvStreamWrap::ReadStart()::$_1::operator()(uv_stream_s*, long, uv_buf_t const*) const [[...]/src/node/out/Release/node]
14: 0x105034b30 node::LibuvStreamWrap::ReadStart()::$_1::__invoke(uv_stream_s*, long, uv_buf_t const*) [[...]/src/node/out/Release/node]
15: 0x1065023c4 uv__stream_io [[...]/src/node/out/Release/node]
16: 0x10650a980 uv__io_poll [[...]/src/node/out/Release/node]
17: 0x1064f6e88 uv_run [[...]/src/node/out/Release/node]
[...]
Command: out/Release/node [...]/src/node/test/parallel/test-http-req-close-robust-from-tampering.js
--- CRASHED (Signal: 6) ---

That's the test from #37958, which was added to complete coverage and intentionally uses internal APIs.

We could skip the DCHECK for now, but it does catch a valid issue here I'd say.

[Flarna]

Setting undefined in AsyncWrap::EmitDestroy() should be fine as it fulfills the contract to have a Local<Value> in MakeCallback. If no context frame was ever set (e.g. no ALS user) the context frame it will be also undefined.

The failing test shows actually a race in HTTP:

1. The http server socket receives the complete HTTP message in a single TCP package.
2. HTTP parser parses headers resulting in emiting request event.
3. During the request event the parser is freeded.
4. once the function returns back to the llhttp parser that one happily continues to process the tcp packet and emits the body - on the already freed parser.

Adding parser[kOnBody] = null; here in cleanParser() avoids the call to onBody but the race that a freed parser is still used is not solved.

But well, thats an unrelated issue for an http expert.

Do you want me to remove the DCHECK? I think it doesn't harm.