second pass phantom callback leads to infinite recursion #27577
Comments
|
I'm wondering about this myself as well. My impression is that the second pass callback is safe to execute JS. But that could easily cause another GC, which would lead to this recursion. |
|
I just cleared it up with the GC folks. The second pass callback is safe. It should run asynchronously, so that it would not lead to recursion. Interestingly I can't reproduce this issue anymore after a clean rebuild. |
|
@hashseed I think this is a real bug, and not necessarily related to your GN build. I don’t know if it’s related to GC, but I can reproduce this with a standard Node.js debug build, but only a) consistently when run under valgrind or b) 2 out of 100 runs without valgrind. |
addaleax
added
confirmed-bug
|
Fully deterministic reproduction using a debug build: // --gc-interval=10 --expose-gc
'use strict';
const vm = require('vm');
const inspector = require('inspector');
const session = new inspector.Session();
session.connect();
session.post('Runtime.enable');
const kContextCount = 3; // kContextCount = 2 fails differently
let contexts = [];
for (let i = 0; i < kContextCount; i++)
contexts.push(vm.runInNewContext('Object'));
contexts = null;
setImmediate(() => {
global.gc();
});I’ll try to work on a solution somewhere along these lines (but Debug builds reaaaally take forever for me so it might take a while just to confirm that it solves the issue and everything): diff --git a/deps/v8/src/global-handles.cc b/deps/v8/src/global-handles.cc
index 350380b23ce6..58987926dc13 100644
--- a/deps/v8/src/global-handles.cc
+++ b/deps/v8/src/global-handles.cc
@@ -995,11 +995,19 @@ void GlobalHandles::InvokeSecondPassPhantomCallbacksFromTask() {
}
void GlobalHandles::InvokeSecondPassPhantomCallbacks() {
+ // The callbacks may execute JS, which in turn may lead to another GC run.
+ // If we are already processing the callbacks, we do not want to start over
+ // from within the inner GC.
+ if (running_second_pass_callbacks_) return;
+ running_second_pass_callbacks_ = true;
+
+ AllowJavascriptExecution allow_js(isolate());
while (!second_pass_callbacks_.empty()) {
auto callback = second_pass_callbacks_.back();
second_pass_callbacks_.pop_back();
callback.Invoke(isolate(), PendingPhantomCallback::kSecondPass);
}
+ running_second_pass_callbacks_ = false;
}
size_t GlobalHandles::PostScavengeProcessing(unsigned post_processing_count) {
diff --git a/deps/v8/src/global-handles.h b/deps/v8/src/global-handles.h
index 8caa3c33ce72..f1abc9326974 100644
--- a/deps/v8/src/global-handles.h
+++ b/deps/v8/src/global-handles.h
@@ -231,6 +231,7 @@ class GlobalHandles final {
traced_pending_phantom_callbacks_;
std::vector<PendingPhantomCallback> second_pass_callbacks_;
bool second_pass_callbacks_task_posted_ = false;
+ bool running_second_pass_callbacks_ = false;
// Counter for recursive garbage collections during callback processing.
unsigned post_gc_processing_count_ = 0; |
|
That solution look alright to me :) |
|
Finally got around to writing regression tests that reproduce this, CL is open at https://chromium-review.googlesource.com/c/v8/v8/+/1607762 :) |
|
It looks like this was released in V8 Going to go ahead and close this, but feel free to re-open if closing is premature. Thanks! |
In my GN build, I'm observing this failure:
./node test/parallel/test-repl-tab-complete.jsThe stack trace I get is this
I think the assertion failure is actually a red herring.
What actually happens is that during GC, we trigger a second-pass-phantom-callback. That sends a notification to the inspector, which in
inspector_js_api.cctries to allocate a string to pass that along. Only, allocating the string fails, because we are not done with GC yet. So we perform GC.That triggers a recursion until we run out of stack, in which case things start to behave weirdly and we get this assertion failure. This weirdness does not reproduce on the regular Node build. The following patch helps with reproducing this on the regular Node build though:
The text was updated successfully, but these errors were encountered: