`delete globalThis.closed` panics REPL

[hyp3rflow]

Deno 1.42.4
exit using ctrl+d, ctrl+c, or close()
> delete globalThis.closed

============================================================
Deno has panicked. This is a bug in Deno. Please report this
at https://github.com/denoland/deno/issues/new.
If you can reliably reproduce this panic, include the
reproduction steps and re-run with the RUST_BACKTRACE=1 env
var set and include the backtrace in your report.

Platform: macos aarch64
Version: 1.42.4
Args: ["deno", "repl"]

thread 'main' panicked at cli/tools/repl/session.rs:310:8:
called `Option::unwrap()` on a `None` value
stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::panic
   3: core::option::unwrap_failed
   4: deno::tools::repl::run::{{closure}}
   5: deno::spawn_subcommand::{{closure}}
   6: <deno_unsync::task::MaskFutureAsSend<F> as core::future::future::Future>::poll
   7: tokio::runtime::task::raw::poll
   8: deno::main
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[MujahedSafaa]

@hyp3rflow I will try to investigate in this bug and find out the root cause.

[MujahedSafaa]

@bartlomieju I have a question regarding the fix for this bug. Should the fix prevent deleting properties such as close from globalThis, or should it display an error message indicating that the close property has been deleted? From my understanding, I believe the fix should prevent deleting properties of globalThis, such as the close.

[bartlomieju]

@MujahedSafaa the ideal fix would involve "capturing" any variables we reference from the global scope either using Chrome Devtools Protocol or by storing v8::Global handles and using these instead of trying to call globalThis.<variable> on every iteration. We should allow users to delete these properties and the REPL should still work correctly. Let me know if you need more pointers.

[MujahedSafaa]

@bartlomieju Could you please give me more details or any code hints that might be helpful?

[bartlomieju]

@MujahedSafaa take a look in cli/tools/repl/session.rs - there you will find REPL_INTERNALS_NAME and get_prelude(). You want to save the reference to globalThis.closed in the get_prelude() function, just like we save references to Deno.noColors or Deno[Deno.internal].inspectArgs. Then instead of calling self.evaluate_expression("(this.closed)") you want to call that from the REPL_INTERNALS_NAME object.

Let me know if that's enough info to get you started.

[MujahedSafaa]

Thanks, @bartlomieju, for the explanation; it was really helpful. I added globalThis.closed to the REPL_INTERNALS_NAME object and replaced the call from self.evaluate_expression("(this.closed)") with a call from the REPL_INTERNALS_NAME object, as you suggested.

However, I have a question regarding the REPL_INTERNALS_NAME.closed value. I think the value is only evaluated once when the REPL_INTERNALS_NAME object is initialized in the initialize function with repl_session.evaluate_expression(&get_prelude()).await?;. This means that if this.closed changes later, it will not be reflected in the REPL_INTERNALS_NAME.closed value since it is just evaluated once. So this could cause incorrect behavior right?

[bartlomieju]

@MujahedSafaa have you tried running delete globalThis.closed and then close()? If the REPL doesn't panic and exits it should be all okay. I believe the solution is correct because globalThis.closed is a getter and it should be evaluated on every access.

[MujahedSafaa]

@bartlomieju Yes, I tried it, and the issue is resolved. It didn't cause any panic. I have no concerns now, so I will open a pull request for the bug.