Question regarding leaking ExternalReferences when making JsRuntime

[nyannyacha]

Hi!
I'm sorry if this was a stupid question.

While I investigating the runtime that was built on Deno code base (actually edge-runtime of supabase 😁), I observed a memory leak on JsRuntime (and precisely the part of leaking external references).

But I don't understand why ExternalReferences should be leaked on all paths.

deno_core/core/runtime/jsruntime.rs

Lines 698 to 700 in a283806

	let refs = bindings::external_references(&op_ctxs, &additional_references);
	// V8 takes ownership of external_references.
	let refs: &'static v8::ExternalReferences = Box::leak(Box::new(refs));

I knew that ownership of external references must be maintained while creating a snapshot 1 2.
But I never heard that restriction also applied when creating a normal isolate instance 🧐

Is there some background that I didn't know about on this?

Before I submitted this issue, I made a fork that adjusts the leak behavior, and there were no leaks regarding external references as expected.

There's the same discussion about this on the Deno repository, but that was open for a long time and it seems no one has a clear answer. 🙄 (denoland/deno#18414)

Any thoughts about this?

[mmastrac]

I'm about to rework a lot of the runtime/realm setup code over the next few weeks, so I can take a look at this when I'm in there.

[bartlomieju]

I believe this is now fixed in #509

[nyannyacha]

@bartlomieju 🙄 But... If I'm looking at it right…it looks like you leaked the lifetime again...? Isn't it?
Am I looking at it wrong? I'm confused 😵‍💫

#499
https://github.com/denoland/deno_core/blob/main/core/runtime/bindings.rs#L92-L94
https://github.com/denoland/deno_core/blob/main/core/runtime/setup.rs#L115-L129