CallSite enhancements

[Alex Kodat]

In managing a menagerie of scripts from various sources, I've found it useful to enhance CallSite locally to have two additional functions getScriptId and getScriptSource. The former provides a unique identifier for the script that contains the code in the stack frame and the latter access to the script source so one can say extract a sourceMappingUrl or display more than one line around the point of error or show source lines in a stack trace or whatever. While it's possible to maintain a table of source ourselves (the scriptId would be helpful in managing such a table) there's no great way of managing the lifespan of entries in the table so it seems useful to be able to get access to the source via the Script object which manages the lifespan of the source nicely.

It was pretty trivial to add these so it doesn't seem like there's a major downside, so does it seem reasonable to submit a CL for this? I'd offer to fix up the doc at https://v8.dev/docs/stack-trace-api if it does, but, of course, I don't have access.

A bit more controversial, perhaps, would be that it seems it would be useful to add a lazily instantiated metadata object on (internal) Script objects. A CallSite could have a getScriptMetadata function to provide access to this object. Similarly, maybe a GetScriptMetadata method for the UnboundScript and maybe Module and Function classes would also provide access to the object. The first request for the ScriptMetadata could create an empty JS Object which could then be used by apps to store script metadata such as, for example, a dereferenced sourceMappingURL. Again, we could manage such metadata externally ourselves but it's a major pain managing the lifespan of such metadata, especially as there's no way of having a direct weak reference to the internal Script object -- the closest we can get is a weak reference to the UnboundScript which is a proxy for the SharedFunctionInfo  which isn't quite right but might be workable.

Does ScriptMetadata sound too crazy/special-purpose or should I try coding it up and submitting a CL and let the reviewers judge if the pain justifies the gain?

Thanks

[Ben Noordhuis]

On Wed, Feb 17, 2021 at 5:42 AM Alex Kodat <ako...@rocketsoftware.com> wrote:
>
> It was pretty trivial to add these so it doesn't seem like there's a major downside, so does it seem reasonable to submit a CL for this? I'd offer to fix up the doc at https://v8.dev/docs/stack-trace-api if it does, but, of course, I don't have access.

The docs are on GitHub:
https://github.com/v8/v8.dev/blob/main/src/docs/stack-trace-api.md

[Yang Guo]

Generally I would discourage adding more non-spec features to the V8's JS implementation. In the long run, these non-spec features cause compatibility problems, and often are a security nightmare. They also add unnecessary complexity to maintain. The reason we have not removed the prepareStackTrace API is because it was introduced more than 10 years ago, and so much existing code already relies on it. We would not have introduced it today. Providing this non-spec feature is already causing unnecessary memory leaks in some cases, and exposing script source as you suggest is not going to improve that.

I'm also skeptical about exposing the script ID. We only expose it via Chrome DevTools protocol, which is orthogonal to the JS-accessible stack trace API.

+Shu-yu Guo to maybe provide another perspective.

I'd encourage you to use the Chrome DevTools protocol to achieve what you are trying to do.

Cheers,

Yang

On Wed, Feb 17, 2021 at 5:42 AM Alex Kodat <ako...@rocketsoftware.com> wrote:

--
--
v8-dev mailing list
v8-...@googlegroups.com
http://groups.google.com/group/v8-dev
---
You received this message because you are subscribed to the Google Groups "v8-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to v8-dev+un...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/46c81d5a-e18e-43fe-b1ae-80002cbd56d8n%40googlegroups.com.

[Alex Kodat]

Thanks Yang,

Got it on prepareStackTrace -- it's a funny and sketchy beast. getThis and getFunction are particularly beyond the pale in terms of encapsulation. In fact, it wouldn't hurt to have a flag to disable the API.

  

In hindsight, the right solution is for me to use Exception::GetStackTrace and the StackFrame class. This does have the ScriptId. I guess I had it in my head that prepareStackTrace was the preferred API. Clearly not. ;-) 

So changing my proposal to adding a Local<Value> GetScriptSource to StackFrame. The idea is to prevent storage leaks by eliminating the need to manage script source external to the internal Script as it's very difficult to manage the lifetime of the source that way. By providing access to the source via the internal Script, the lifetime is nicely managed for us. This seems pretty safe from a security/encapsulation perspective as embedders can limit access to this information as they see fit.

 

It would also be useful to have a Local<Object> GetScriptMetadata (or GetScriptMemo?) in StackFrame to allow memoization of the script data. The metadata object could be lazily instantiated and again, would reduce the risk of storage leaks as the lifetime of the ScriptMetadata would generally match the lifetime of the internal Script object. UnboundScript and maybe Module could also have a GetScriptMetadata to provide access to the same object.

Might either of these be more palatable?

[Yang Guo]

Hi,

I think the hurdle for a C++ API is a lot lower, and I can see the usefulness of GetScriptSource as you suggest.

I'm somewhat indifferent about a metadata object. Isn't that a bit like import.meta for modules?

Cheers,

Yang

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/2f6aaef5-4d2f-4c29-9df6-0ed133e0143fn%40googlegroups.com.

[Alex Kodat]

Thanks Yang,

FWIW, it appears that one can effectively disable the prepareStackTrace API by setting Error.stackTraceLimit to 0. Fortunately, this does not affect Exception::StackTrace. But you knew that.

My impression of import.meta is that it's a way of the environment passing information into a module, presumably information about the module itself.

My ScriptMetadata proposal is more about maintaining information about the script for use outside of the script though it could, in theory, be exposed inside a script if an embedder chooses to do so (doubt if we would). 

I guess one similarity to import.meta is the read-only/lazy-instantiation of ScriptMetadata so, like import.meta, if it's not used, there's no cost but once it's set, it can't be changed. Also ScriptMetadata and import.meta would have lifetimes that would tend to follow the lifetimes of the corresponding script or module which is why ScriptMetadata would be useful.

[Yang Guo]

To understand this a bit better, the reason a map from script to metadata doesn't work for you, when implemented on the embedder side, is that you can't have a weak map with script IDs as keys?

I'm concerned that since this seems like a niche use case, adding another field to Script objects is not a good idea. So we'd have to implement that as a weak map inside of V8 either way.

Cheers,

Yang

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/37509e39-de36-445a-8990-0eec38a9363cn%40googlegroups.com.

[Alex Kodat]

Yang,

Thanks for your response. I've decided to bail out on the metadata part of my proposal for the very reason you indicated -- it's kind of a niche application and, in many ways, a time-based cache makes more sense for my application. I'll submit a CL soon to add GitScriptSource and GetScriptSourceMappingURL methods to StackFrame. These are very straightforward, require no data structure changes and expose nothing unless an embedder choses to do so. Hopefully this is OK.

Not that its important but I'm a little puzzled by your comment about the weak map. Scripts are already managed via a WeakArrayList -- functionality that would be nice in JS but, of course, only usable because the iterator is able to block GC while it's alive and, of course, exposing something like that in JS would be a bad idea.

Cheers,

Alex

[Yang Guo]

It is true that Scripts are already listed in the weak array list. However, that's hidden in V8. So if we were to keep a weak list of scripts to metadata mapping, that would work fine. If we did this on the embedder side, then we'd need something else than script IDs as keys.

Looking forward to your patch :)

Yang

To view this discussion on the web visit https://groups.google.com/d/msgid/v8-dev/8efe618c-26c0-4b08-ab3f-27dac6765209n%40googlegroups.com.

[Alex Kodat]

Yang,

I see. My intent had been to put a reference to the metadata object directly in the Script object so no weak map would be required -- the metadata would live as long as Script, Script would just have gotten a little bigger. But, since there's no compelling use case, it's all academic.

Thanks,

Alex