Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(ext/node): Fix vm sandbox object panic #24985

Merged
merged 2 commits into from
Sep 24, 2024
Merged

fix(ext/node): Fix vm sandbox object panic #24985

merged 2 commits into from
Sep 24, 2024

Conversation

littledivy
Copy link
Member

Fixes #24412

@bartlomieju
Copy link
Member

Is there a small test we could add?

@devsnek
Copy link
Member

devsnek commented Aug 12, 2024

Yeah I am a bit concerned about this because I don't know how you would write code that hits this unwrap...

@marvinhagemeister
Copy link
Contributor

marvinhagemeister commented Aug 12, 2024 •
edited
Loading

I haven't been able to isolate the problem out of webpack's test suite yet. It looks to be a bit deeper burried in jest, maybe even tied to a specific version.

@littledivy
Copy link
Member Author

littledivy commented Aug 12, 2024 •
edited
Loading

Huh i cant find the code in jest that reproduces this.

It's a weird condition, the TracedReference to the sandbox object is empty and then somehow the interceptors are still called.

@marvinhagemeister
Copy link
Contributor

Looking at the webpack code, they are using the vm module themselves in their code. Maybe it occurs there. I haven't been able to tie the panic to a specific test case though.

@littledivy
Copy link
Member Author

Should we land this? I am not able to hit unwraps outside of the jest reproduction case.

@devsnek
Copy link
Member

devsnek commented Aug 21, 2024

maybe change them to throw a js exception, should make it easier to track down the cause

@littledivy
Copy link
Member Author

After throwing an exception:

    Invalid sandbox object

      at CacheBackend._runDecays (node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:451:47)
      at Timeout._onTimeout (node_modules/enhanced-resolve/lib/CachedInputFileSystem.js:484:9)
      at cb (ext:deno_node/internal/timers.mjs:64:31)
      at callback (ext:deno_web/02_timers.js:68:7)
      at eventLoopTick (ext:core/01_core.js:210:13)

Still pretty hard to get a small repro. Maybe we should do a npm_smoke_test for this instead? I want to merge this PR for Deno 2

littledivy added a commit to denoland/deno_core that referenced this pull request Sep 22, 2024
@littledivy
Fix panic in prepare_stack_trace_callback when global interceptor t…
e477741 
…hrows

Fix panic when a global interceptor is misconfigured or throws an
exception. Example
denoland/deno#24985 (comment)
@littledivy littledivy mentioned this pull request Sep 22, 2024
Merged
@littledivy littledivy merged commit 7d7e541 into denoland:main Sep 24, 2024
17 checks passed
@littledivy littledivy deleted the dont_panic_vm_sandbox_obj branch October 14, 2024 09:27
littledivy added a commit to denoland/deno_core that referenced this pull request Oct 14, 2024
@littledivy
Fix panic in prepare_stack_trace_callback when global interceptor t…
910d3c4 
…hrows

Fix panic when a global interceptor is misconfigured or throws an
exception. Example
denoland/deno#24985 (comment)
littledivy added a commit to denoland/deno_core that referenced this pull request Oct 14, 2024
@littledivy
Fix panic in prepare_stack_trace_callback when global interceptor t…
9dd5f80 
…hrows (#914)

Fix panic when a global interceptor is misconfigured or throws an
exception.

Fixes
denoland/deno#24985 (comment) and
denoland/deno#26240
@littledivy littledivy mentioned this pull request Oct 15, 2024
Merged
littledivy added a commit that referenced this pull request Oct 15, 2024
@littledivy
fix: panic in prepare_stack_trace_callback when global interceptor …
e4b52f5 
…throws (#26241)

Fixes #26240
Fixes
#24985 (comment)

Fix panic when a global interceptor is misconfigured or throws an
exception.

Updates deno_core to 0.313.0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

@devsnek devsnek Awaiting requested review from devsnek

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bug: Panic when running webpack's test suite
4 participants
@littledivy @bartlomieju @devsnek @marvinhagemeister