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.

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

deno compile stack trace printing causes panics #8578

Closed
lucacasonato opened this issue Dec 1, 2020 · 6 comments · Fixed by #8581
Closed

deno compile stack trace printing causes panics #8578

lucacasonato opened this issue Dec 1, 2020 · 6 comments · Fixed by #8581
Labels
bug Something isn't working correctly cli related to cli/ dir
Milestone

Comments

@lucacasonato
Copy link
Member

~/p/g/d/deno ❯❯❯ cat test.js
throw new Error("foo");
~/p/g/d/deno ❯❯❯ deno compile --unstable test.js
Bundle file:///mnt/f9/Projects/github.com/denoland/deno/test.js
Compile file:///mnt/f9/Projects/github.com/denoland/deno/test.js
Emit test
~/p/g/d/deno ❯❯❯ env RUST_BACKTRACE=1 ./test
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: ()', cli/disk_cache.rs:72:39
stack backtrace:
   0: rust_begin_unwind
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/std/src/panicking.rs:483
   1: core::panicking::panic_fmt
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/core/src/panicking.rs:85
   2: core::option::expect_none_failed
             at /rustc/7eac88abb2e57e752f3302f02be5f3ce3d7adfb4/library/core/src/option.rs:1234
   3: core::result::Result<T,E>::unwrap
   4: deno::disk_cache::DiskCache::get_cache_filename_with_extension
   5: deno::program_state::ProgramState::get_emit
   6: deno::source_maps::get_orig_position
   7: deno_core::ops::json_op_sync::{{closure}}
   8: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
   9: deno::metrics::metrics_op::{{closure}}
  10: <alloc::boxed::Box<F> as core::ops::function::Fn<A>>::call
  11: <extern "C" fn(A0) .> R as rusty_v8::support::CFnFrom<F>>::mapping::c_fn
  12: _ZN2v88internal25FunctionCallbackArguments4CallENS0_15CallHandlerInfoE
  13: _ZN2v88internal12_GLOBAL__N_119HandleApiCallHelperILb0EEENS0_11MaybeHandleINS0_6ObjectEEEPNS0_7IsolateENS0_6HandleINS0_10HeapObjectEEESA_NS8_INS0_20FunctionTemplateInfoEEENS8_IS4_EENS0_16BuiltinArgumentsE
  14: _ZN2v88internalL26Builtin_Impl_HandleApiCallENS0_16BuiltinArgumentsEPNS0_7IsolateE
  15: Builtins_CEntry_Return1_DontSaveFPRegs_ArgvOnStack_BuiltinExit
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.
fatal runtime error: failed to initiate panic, error 5
fish: “env RUST_BACKTRACE=1 ./test” terminated by signal SIGABRT (Abort)

cc @bartlomieju

@lucacasonato lucacasonato added bug Something isn't working correctly cli related to cli/ dir labels Dec 1, 2020
@lucacasonato lucacasonato added this to the 1.6.0 milestone Dec 1, 2020
@kitsonk
Copy link
Contributor

kitsonk commented Dec 1, 2020

We probably need to use some other scheme for the module specifiers of the embedded bundle, as we will run into this in various places I suspect... It is the disk cache trying to check to see if there is a source map it is supposed to use. What module specifier are you using for the bundle internally?

@lucacasonato
Copy link
Member Author

It is a const defined in cli/standalone.rs. Currently it is file://$deno$/bundle.js

@kitsonk
Copy link
Contributor

kitsonk commented Dec 1, 2020

Yeah, I think we need another scheme (and check places where we analyse the scheme to make sure we don't panic)... something like bundle:///main.js?

Eventually we need to support source maps for bundles, something that I wanted to get done for 1.6 but got distracted with other things, and so we ultimately want that to work.

@bartlomieju
Copy link
Member

bartlomieju commented Dec 1, 2020

Yeah, I think we need another scheme (and check places where we analyse the scheme to make sure we don't panic)... something like bundle:///main.js?

I don't think that's true because currently "self-contained" binaries have stubbed out loader that can load only a single file, we actually need to disable source mapping errors altogether for "self-contained" binaries (at least for now, until we support some kind of VFS for those binaries).

Just to remind - currently errors are source mapped in two distinct places: in prepareStackTrace method in JS as well as in MainWorker.

I've got that covered in #8381 just need to backport it.

@kitsonk
Copy link
Contributor

kitsonk commented Dec 1, 2020

disable source mapping errors altogether for "self-contained" binaries (at least for now, until we support some kind of VFS for those binaries)

We could disable them for now, but we need to get source maps working for bundles and consuming/applying source maps that exist. We could simply inline the source maps in bundles. But I think it is a bad idea, generally, to use what is a file:/// URL for the inlined bundles. It is a lie, one we shouldn't make... we should use some other scheme. It makes it 100% clear that it isn't a physical file. If we expand and support some sort of vfs in the future, then we already have the minimal infrastructure. It is also the easiest way to "disable" source remapping too... simply a match branch on the scheme... I would be fine with something like vfs:/// as well.

@bartlomieju
Copy link
Member

@kitsonk ok I agree, you've got a good point and I think we should address it as well. Nonetheless I made a PR (#8581) that disables source maps in errors for self-contained binaries (actually disables formatting of CLI in errors altogether).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working correctly cli related to cli/ dir
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants