-
Notifications
You must be signed in to change notification settings - Fork 71
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
failing test-trace-events-dynamic-enable #78
Comments
I'll take a look. I am working on relanding the original change upstream. I had to fix some thread-safety issues (unrelated to the nodejs/node#22114, but exposed by it) that caused the revert. The upstream fixes that have landed: v8/v8@602aeb4, and v8/v8@f964846. I am still working on the reland: https://chromium-review.googlesource.com/c/v8/v8/+/1188529 |
Thanks for the update! |
Any progress? I see there was some trouble with the reland CL |
@targos I'm looking at this now. |
The ASAN issues in V8 CQ were legitimate, and should be fixed by this fix: https://chromium-review.googlesource.com/c/v8/v8/+/1228419. Here are the steps that need to happen:
Until then, we can either revert the change that introduced the test in Node, or disable it, like you have already. I'll let you decide on the approach. |
Both changes have landed upstream. |
Nice! Can we try to have the changes backported upstream? |
LMK if I should open an issue for the merge request |
Merge request opened: https://bugs.chromium.org/p/v8/issues/detail?id=8199 |
This test was added in nodejs/node#22114
That PR backported https://chromium-review.googlesource.com/1161518 from V8.
The problem is that the CL was reverted in https://chromium-review.googlesource.com/1162122 and never relanded.
/cc @ofrobots
The text was updated successfully, but these errors were encountered: