Skip to content

src: lock the isolate properly in IsolateData destructor#57031

Merged
nodejs-github-bot merged 2 commits intonodejs:mainfrom
joyeecheung:isolate-data-locker
Feb 15, 2025
Merged

src: lock the isolate properly in IsolateData destructor#57031
nodejs-github-bot merged 2 commits intonodejs:mainfrom
joyeecheung:isolate-data-locker

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Feb 13, 2025

Otherwise it may fail the DCHECK that uses the locked thread as a fast path to get the current thread.

@joyeecheung
src: lock the isolate properly in IsolateData destructor
a597b98 
Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.
@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. needs-ci PRs that need a full CI run. labels Feb 13, 2025
@joyeecheung joyeecheung mentioned this pull request Feb 13, 2025
Closed
@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 13, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 13, 2025

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

@targos
Copy link
Member

targos commented Feb 13, 2025

Looks like we can't do that on the main branch. Feel free to push the commit to #56959

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 13, 2025

It seems it also fails in the V8 13.3 branch in the cctest, I suspect that this is also exposing that we aren't using HandleScope properly throughout the codebase.

@joyeecheung
Copy link
Member Author

joyeecheung commented Feb 13, 2025 •
edited
Loading

Looks like it's the cctests not locking threads and the exposure kinda spreads once someone is locking the threads. The second commits fix them locally for me.

@targos
Copy link
Member

targos commented Feb 13, 2025

@nodejs/cpp-reviewers

@codecov
Copy link

codecov bot commented Feb 13, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.11%. Comparing base (85f5a6c) to head (7bfd05c).
Report is 21 commits behind head on main.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main   #57031   +/-   ##
=======================================
  Coverage   89.11%   89.11%           
=======================================
  Files         665      665           
  Lines      193193   193194    +1     
  Branches    37212    37212           
=======================================
+ Hits       172158   172168   +10     
+ Misses      13775    13774    -1     
+ Partials     7260     7252    -8
Files with missing lines Coverage Δ
src/env.cc 85.89% <100.00%> (-0.16%) ⬇️

... and 29 files with indirect coverage changes

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 14, 2025
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 14, 2025 •
edited by jasnell
Loading

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

@joyeecheung joyeecheung added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Feb 15, 2025
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 15, 2025
@nodejs-github-bot nodejs-github-bot merged commit d384151 into nodejs:main Feb 15, 2025
61 checks passed
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Feb 15, 2025

Landed in d384151

targos pushed a commit that referenced this pull request Feb 17, 2025
@joyeecheung @targos
src: lock the isolate properly in IsolateData destructor
fa26f83 
Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.

PR-URL: #57031
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
acidiney pushed a commit to acidiney/node that referenced this pull request Feb 23, 2025
@joyeecheung @acidiney
src: lock the isolate properly in IsolateData destructor
35f5de2 
Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.

PR-URL: nodejs#57031
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@github-actions github-actions bot mentioned this pull request Feb 25, 2025
Merged
aduh95 pushed a commit that referenced this pull request Apr 2, 2025
@joyeecheung @aduh95
src: lock the isolate properly in IsolateData destructor
6732f3d 
Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.

PR-URL: #57031
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
aduh95 pushed a commit that referenced this pull request Apr 3, 2025
@joyeecheung @aduh95
src: lock the isolate properly in IsolateData destructor
b2c3d8d 
Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.

PR-URL: #57031
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
@aduh95 aduh95 mentioned this pull request Apr 11, 2025
Merged
RafaelGSS pushed a commit that referenced this pull request Apr 16, 2025
@joyeecheung @RafaelGSS
src: lock the isolate properly in IsolateData destructor
60a645f 
Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.

PR-URL: #57031
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
RafaelGSS pushed a commit that referenced this pull request Apr 17, 2025
@joyeecheung @RafaelGSS
src: lock the isolate properly in IsolateData destructor
619e52c 
Otherwise it may fail the DCHECK that uses the locked thread
as a fast path to get the current thread.

PR-URL: #57031
Reviewed-By: Chengzhong Wu <legendecas@gmail.com>
Reviewed-By: Yagiz Nizipli <yagiz@nizipli.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Rafael Gonzaga <rafael.nunu@hotmail.com>
codebytere added a commit to electron/electron that referenced this pull request Apr 25, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
073a033 
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request Apr 25, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
e7a8909 
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 1, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
0db4eb1 
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 5, 2025
@electron-roller @codebytere
chore: bump node to v22.15.0 (main) (#46742)
cb445b3 
* chore: bump node in DEPS to v22.15.0

* inspector: fix GN build

nodejs/node#56798

* test: search cctest files

nodejs/node#56791

* crypto: fix missing OPENSSL_NO_ENGINE guard

nodejs/node#57012

* test,crypto: make tests work for BoringSSL

nodejs/node#57021

* module: use synchronous hooks for preparsing in import(cjs)

nodejs/node#55698

* deps: update simdjson to 3.12.0

nodejs/node#56874

* build: remove explicit linker call to libm on macOS

nodejs/node#56901

* test: make eval snapshot comparison more flexible

nodejs/node#57020

* src: allow embedder customization of OOMErrorHandler

nodejs/node#57325

* src: do not pass nullptr to std::string ctor

nodejs/node#57354

* src: lock the isolate properly in IsolateData destructor

nodejs/node#57031

* chore: shrink --trace-atomics-wait patch

* chore: fixup patch indices

* build: fix GN build failure

nodejs/node#57013

* crypto: expose security levels

nodejs/node#56601

* zlib: add zstd support

nodejs/node#52100

* test: move crypto related common utilities in common/crypto

nodejs/node#56714

* cli: move --trace-atomics-wait to eol

nodejs/node#52747

* test: disable test-https-client-renegotiation-limit

BoringSSL doesn't support caller-initiated renegotiation - see
https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/ssl/ssl_lib.cc;l=1627-1631

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
codebytere added a commit to electron/electron that referenced this pull request May 5, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
dbdefef 
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 6, 2025
@codebytere
src: lock the isolate properly in IsolateData destructor
079024e 
nodejs/node#57031
codebytere added a commit to electron/electron that referenced this pull request May 6, 2025
@electron-roller @codebytere
chore: bump node to v22.15.0 (37-x-y) (#46869)
cd023b5 
* chore: bump node in DEPS to v22.15.0

* inspector: fix GN build

nodejs/node#56798

* test: search cctest files

nodejs/node#56791

* crypto: fix missing OPENSSL_NO_ENGINE guard

nodejs/node#57012

* test,crypto: make tests work for BoringSSL

nodejs/node#57021

* module: use synchronous hooks for preparsing in import(cjs)

nodejs/node#55698

* deps: update simdjson to 3.12.0

nodejs/node#56874

* build: remove explicit linker call to libm on macOS

nodejs/node#56901

* test: make eval snapshot comparison more flexible

nodejs/node#57020

* src: allow embedder customization of OOMErrorHandler

nodejs/node#57325

* src: do not pass nullptr to std::string ctor

nodejs/node#57354

* src: lock the isolate properly in IsolateData destructor

nodejs/node#57031

* chore: shrink --trace-atomics-wait patch

* chore: fixup patch indices

* build: fix GN build failure

nodejs/node#57013

* crypto: expose security levels

nodejs/node#56601

* zlib: add zstd support

nodejs/node#52100

* test: move crypto related common utilities in common/crypto

nodejs/node#56714

* cli: move --trace-atomics-wait to eol

nodejs/node#52747

* test: disable test-https-client-renegotiation-limit

BoringSSL doesn't support caller-initiated renegotiation - see
https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/ssl/ssl_lib.cc;l=1627-1631

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
kigh-ota pushed a commit to kigh-ota/electron that referenced this pull request Sep 30, 2025
@electron-roller @codebytere @kigh-ota
chore: bump node to v22.15.0 (main) (electron#46742)
2f58fe0 
* chore: bump node in DEPS to v22.15.0

* inspector: fix GN build

nodejs/node#56798

* test: search cctest files

nodejs/node#56791

* crypto: fix missing OPENSSL_NO_ENGINE guard

nodejs/node#57012

* test,crypto: make tests work for BoringSSL

nodejs/node#57021

* module: use synchronous hooks for preparsing in import(cjs)

nodejs/node#55698

* deps: update simdjson to 3.12.0

nodejs/node#56874

* build: remove explicit linker call to libm on macOS

nodejs/node#56901

* test: make eval snapshot comparison more flexible

nodejs/node#57020

* src: allow embedder customization of OOMErrorHandler

nodejs/node#57325

* src: do not pass nullptr to std::string ctor

nodejs/node#57354

* src: lock the isolate properly in IsolateData destructor

nodejs/node#57031

* chore: shrink --trace-atomics-wait patch

* chore: fixup patch indices

* build: fix GN build failure

nodejs/node#57013

* crypto: expose security levels

nodejs/node#56601

* zlib: add zstd support

nodejs/node#52100

* test: move crypto related common utilities in common/crypto

nodejs/node#56714

* cli: move --trace-atomics-wait to eol

nodejs/node#52747

* test: disable test-https-client-renegotiation-limit

BoringSSL doesn't support caller-initiated renegotiation - see
https://source.chromium.org/chromium/chromium/src/+/main:third_party/boringssl/src/ssl/ssl_lib.cc;l=1627-1631

---------

Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com>
Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@anonrig anonrig anonrig approved these changes

@legendecas legendecas legendecas approved these changes

@RafaelGSS RafaelGSS RafaelGSS approved these changes

Assignees

No one assigned

Labels

c++ Issues and PRs that require attention from people who are familiar with C++. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@joyeecheung @nodejs-github-bot @targos @jasnell @addaleax @anonrig @legendecas @RafaelGSS