util: add private fields preview support

[legendecas]

util.previewValue may reveal private fields of an object.

Fixes: #27404

Checklist

Feature list:

• reveal primitive values
• reveal nested objects with prototypes
• reveal properties of nested objects, not supported
• reveal function names

Common list:

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

[addaleax]

Generally, this PR looks good 👍

I feel like it’s worth having this come with a benchmark – I’d guess this makes inspecting private properties rather expensive, and that’s probably okay but it might be nice to get a better idea of what the performance impact is.

[addaleax]

Tiny nit, obviously not super important: Can you make is_sync the last parameter instead of the first? The signatures for all BaseObject subclasses take env and wrap as the first arguments, to that would make it a bit more consistent

[devsnek]

Could we just have like a globalThis.__inspect_target that we create and use and then delete, so we don't have to deal with the huge perf hit from forcing evaluation through the debug interpreter?

[joyeecheung]

As mentioned in #27110 (comment) these events only happen to be synchronous so that currently the listeners can be invoked before util.inspect returns. I guess we either need to mention the risk about that this can be broken in a future v8 update in the doc, or communicate with the V8 team to make this synchronicity part of the protocol (which seems difficult though).

cc @nodejs/v8

[joyeecheung]

Note that implicit synchronicity is still there - it's not guaranteed by V8 that the result would show up as this[resultSymbol] right after the call to this[connectionSymbol].dispatch(JSONStringify(message)), before post() returns. It only happens to work right now.

[BridgeAR]

The performance impact is frightening (-99.x%). We could consider adding an experimental option that we remove at some point to activate this for now.

Test wise we also have to check for already running inspector sessions. The user could already have a running session and the REPL itself also uses inspector sessions. I expect the inspection to fail in case another session is open? This should be clearly documented.

[BridgeAR]

Checking individually for privatePropertyPreviews requires a number of extra checks (everywhere, where protoProps is also checked). Instead, I suggest to rename protoProps and to add the private entries there. It is also logical to first show all regular properties, then to show all private members and then at the end all inherited ones.

[BridgeAR]

It would be useful to know the exact function type and the function name as well:

Suggested change

	str = '#[Function] {}';
	ctx.indentationLvl += 2;
	str = `#${formatValue(valueDescriptor.value)}`;
	ctx.indentationLvl -= 2;

[joyeecheung]

One possible workaround for core may be to add a different option to util.inspect and the console to return the results asynchronously with the inspector API (e.g. return a promise in util.inspect). That way people opt into a more expensive implementation for more information and are aware of the asynchronous contract. And I think it's generally a good idea to move to this kind of API based on the inspector protocol instead of relying on our home-grown implementation, even not for inspecting private members.

[BridgeAR]

@joyeecheung adding an option to return a promise in util.inspect() is IMO not a good idea. Not only is it confusing to have an API behaving sync in most cases but not in all, it would also increase the implementation complexity which is also not ideal.

Switching existing APIs to use the inspector would a) likely be slower, b) not work under all circumstances and c) be a breaking change for multiple data types (since we would have to return a promise).

[legendecas]

I'm very glad there might be other possibilities for this. But I found it hard to work out for the following issues:

If the private field inspection is going to be asynchronous, it can be not possible of multiple inspection requests at a single run. Since in util.inspect requests are initiated just on the thread on which the isolate is running.

The two available approaches are

1. Pause on the current execution and get all object ids to use later. This is what DevTools using to inspect values in frames.
2. Fetch global objects and inspect properties on it. This is what we doing in the PR right now for no pause required. And this is also what we can do in the console of DevTools while not paused.

In the asynchronous util.inspect, both two approaches are not going to be reliable to fetch correct desired object ids since the running context is not available to it.

@joyeecheung please fixes me if I'm not got right.

[joyeecheung]

Not only is it confusing to have an API behaving sync in most cases but not in all, it would also increase the implementation complexity which is also not ideal.

Can we just provide an entirely different API, then? It gives you some way to inspect these things in a reliable, well-encapsulated manner, instead of forcing the maintainability burden on either Node.js or V8.

Switching existing APIs to use the inspector would a) likely be slower, b) not work under all circumstances and c) be a breaking change for multiple data types (since we would have to return a promise).

That's why it should be opt-in and not always the default. I think it's reasonable to opt into the cost when you indeed need the underlying data. It is not sustainable to keep making util.inspect more powerful in the current fashion IMO (as my conclusion from the discussion in https://chromium-review.googlesource.com/c/v8/v8/+/1546559 where it was obvious that V8 prefers to support an implementation based on the inspector protocol), and private members would not be the only information that is affected by this implementation model.

[joyeecheung]

In the asynchronous util.inspect, both two approaches are not going to be reliable to fetch correct desired object ids since the running context is not available to it.

I wonder why...can you elaborate on the protocol methods you use in these two approaches? This PR is currently using Runtime.evaluate, but I am assuming that Debug.evaluateOnCallFrame should give the id you want here.

[devsnek]

It originally used call frame inspection but I asked that it be changed because enabling the debugger causes v8 to deoptimize things and calling console.log shouldn't make your wasm tier down

[joyeecheung]

@devsnek I think it’s a separate matter if the user opts into the performance hit of console.log() by setting certain value to true in order to obtain more detailed information. The fundamental issue here is that IMO it’s not realistic to expect that Node.js is able to dig out the guts of objects without the help of deoptimization or any custom inspector - and from what I can tell users should not rely on this feature when performance is critical anyway, and it should be reasonable to keep the inspected result simple when they don’t explicitly opt into detailed results at the cost of perf.

[devsnek]

i don't expect console.log to be fast, but i also know we don't need to tier down to read the private properties of an object. maybe we can convince v8 to provide the debugger apis in a more direct form in the v8::debug namespace or something.

[legendecas]

@joyeecheung Debug.pause is the precedent call of Debug.evaluateOnCallFrame. Since there is no definition on which frame Debug.pause should pause on if the request is initiated from the thread isolate running on, there is implicit synchronicity too if we are going to evaluate (inspect) on the right just call frame.

[joyeecheung]

but i also know we don't need to tier down to read the private properties of an object.

I don't think that's necessarily a guarantee that V8 can provide - it could very well refactor the implementation so that it needs to reparse the class body to reflect on this at run time (as that was brought up several times during the implementation of inspector support of private members). Requesting V8 to provide a performant API for this seems to impose unnecessary constraints to them especially when the method belongs to v8::debug. Why do we have to guarantee the performance of inspecting the guts of an object in a way that's intentionally unavailable to user-land JS? In addition the user has to opt into this kind of functionality with showHidden: true even if we don't add an additional option or expose it through another API anyway, so it does not affect the performance of the majority of the use cases.

there is implicit synchronicity too if we are going to evaluate (inspect) on the right just call frame.

Ah, I thought #31817 (comment) was about whether it's feasible to do this for an asynchronous API. Then it's not going to be an issue if it's done in an asynchronous manner.

[devsnek]

@joyeecheung i'm find with util.inspect being slow (maybe even involve reparsing, etc), it just shouldn't involve anything that causes the program to remain slow after util.inspect is done.

[joyeecheung]

@devsnek But the majority of users don't use showHidden: true anyway? If they opt into this a less performant mode I think it's fine just as it's fine to accept the performance hit when you turn on the debugger. What I am wondering is:

1. Is it necessary to provide the inspected result synchronously? Why? If it's only because util.inspect() is currently synchronous, why can't we add a separate method that's asynchronous but more powerful?
2. Why do we have to care about performance when the user needs to explicitly opt into this kind of detailed inspection in the first place (e.g. showHidden: true for the moment), which is AFAICT, rarely done anyway? (and when it's done, it's probably not used in a scenario where performance is critical)

[devsnek]

@joyeecheung all I'm saying is we shouldn't use the Debugger.* stuff. Runtime.getProperties provides what we need, and I agree that the 99% perf regression with showHidden: true doesn't really matter.

[joyeecheung]

@devsnek I don't think it's guaranteed by V8 that this shall always be available with just Runtime methods either, and it should be fine as long as the user is aware that using a more powerful variant of inspector requires Debugger.*, or otherwise we would be imposing another (unrealistic, IMO) constraint on ourselves again for a functionality that fundamentally needs some kind of escalation of privileges.

[legendecas]

@joyeecheung I thought #31817 (comment) was about whether it's feasible to do this for an asynchronous API. Then it's not going to be an issue if it's done in an asynchronous manner.

I demonstrated a demo of asynchronous API:

function previewValueAsync(target) {
  return new Promise((resolve, reject) => {
    session.once('Debugger.paused', (pausedContext) => {
      const callFrames = pausedContext.params.callFrames;
      session.post(
        'Debugger.evaluateOnCallFrame', {
          /** 0. inspector.dispatch, 1. inspector.post, 2. new Promise, 3. previewValueAsync */
          callFrameId: callFrames[3].callFrameId,
          expression: 'target',
        }, (err, result) => {
          if (err) {
            return reject(err);
          }
          const { result: evalResult } = result;

          if (evalResult.type !== 'object') {
            return resolve();
          }
          if (evalResult.subtype === 'error') {
            const error = new Error('Inspection failed: ' + evalResult.description);
            return reject(error);
          }
          session.post('Runtime.getProperties', {
            objectId: evalResult.objectId,
          }, (err, result) => {
            if (err) {
              return reject(err);
            }
            resolve(result);
          });
        });
    });
    session.post('Debugger.pause' () => { /* pause doesn't return anything. */ });
    return previews;
  });
}

But I'm wondering that we are still relying on synchronous semantics of session.post('Debugger.pause'), which can implementation details too.

[joyeecheung]

we are still relying on synchronous semantics of session.post('Debugger.pause')

I think this would not break even if the 'Debugger.paused' event is triggered asynchronously, in the worse case we can even resolve from C++.

[legendecas]

According to suggestions made above, I've updated the PR to add a method util.previewValue, which is somewhat identical to util.inspect, except it reveals the target object's own private fields and (public) properties.

[BridgeAR]

@legendecas the TODO about inspecting the whole object would likely require to duplicate most of the existing inspect code. Using the separate API is likely not as powerful as util.inspect() otherwise, but it would also be difficult to keep everything aligned.

I would still love to have a sync API built into util.inspect. That way it would be possible to keep the API mostly working as it is and provide the best user experience.

[jasnell]

Ping @legendecas

[legendecas]

According to the current suggestions and ideas, I don't believe the new asynchronous inspection method will be helpful in cases that I was expecting to work out. I'm closing this, anyone who get a use case can pick this up freely, and I'm willing to help too.