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

Expose (some) CDP commands #893

Closed
hediet opened this issue Dec 13, 2020 · 21 comments · Fixed by #964
Closed

Expose (some) CDP commands #893

hediet opened this issue Dec 13, 2020 · 21 comments · Fixed by #964
Assignees
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Milestone

Comments

@hediet
Copy link
Member

hediet commented Dec 13, 2020

I'd love to play around with the Page.captureScreenshot request for an experimental VS Code extension.
It would be awesome if the VS Code Debug Adapter would expose some request that would allow to call these CDB requests through debugSession.customRequest.

Advantages

  1. This would allow for some cool (experimental) VS Code extensions. The captureScreenshot request could be used to show a screenshot of variables that point to an HTML element. I guess there are also some other cool methods exposed by chrome that could further push the debugging experience.

Risks

  1. User experience could be degraded if such extensions bring the debug adapter in an inconsistent state by calling some CDP requests they are not supposed to call.

I guess to mitigate 1), the exposed CDP requests could be allow-listed (e.g. only when using the non-insiders build to enable exploring ideas). Page.captureScreenshot should be pretty risk free.

What do you think?

@hediet hediet added the feature-request Request for new features or functionality label Dec 13, 2020
@connor4312
Copy link
Member

I'd be open to that. Feel free to toss in a PR, or I can get to it eventually. Code pointers:

  1. Add a new request to dapCustom.json and npm run generateapis
  2. Hook in the extension site to debugSession.customRequest
  3. Listen to that request around here, the thread provides the cdp connection e.g.
    this.dap.on('getPerformance', () =>
    this._withThread(thread => performanceProvider.retrieve(thread.cdp())),
    );

@hediet
Copy link
Member Author

hediet commented Dec 14, 2020

Thanks - I'll try to submit a PR this week!

@hediet
Copy link
Member Author

hediet commented Jan 8, 2021

Sorry for the delay. I had a look today, but I was not able to debug the debug-adapter. I opened the debug-adapter in the latest vscode insider build, ran npm run watch and launched the "Extension" launch config. The entry point of the extension called. However, my breakpoints in the constructor of the DebugAdapter are not hit.

I also get this error in the Extension Development Host:
image

I guess its a trivial mistake, but I'm stuck.

I didn't figure out that I had to uninstall the bundled JavaScript Debugger Extension. I didn't even know this is possible...
My breakpoints are being hit now 😉

It works nicely:

image

I'll try to prepare a PR soon that exposes this cdp method.
I think a hover provider can be implemented that shows a screenshot of HTML elements when hovering on an HtmlElement while debugging the code.

@swissmanu
Copy link
Contributor

swissmanu commented Mar 2, 2021

I would like to piggyback on vscode-js-debugs CDP connection in my own extension/debug adapter. As I understand your proposal @hediet, you try to expose very specific commands through the customRequest, whereas I would like to proxy "everything" to my own adapter.

What do you think about this and how is your PR coming? Maybe we can collaborate on this?

@connor4312
Copy link
Member

whereas I would like to proxy "everything" to my own adapter

I would prefer a design where you don't actually proxy 'everything'. For example, you may not care about scriptParsed events for your use case and only request/response pairs and output events. CDP is chatty and latency is important, I don't want to add extra work unnecessarily.

@connor4312
Copy link
Member

connor4312 commented Mar 10, 2021

I've started working with the Edge Devtools team who would like to integrate with js-debug using this mechanism too. I'm not sure how far you've gone, my initial thought for a design would be something like this:

  • js-debug exposes a command js-debug.requestCdpProxy with the debug session id. The first time this is executed, js-debug stands up a websocket server for debugging.
    • In the remote scenario, js-debug will automatically open a tunnel so that UI extensions can connect to it.
    • The command returns the port the server is listening on in the form { remotePort: number, localPort: number }. These may be different if the user had something running on the local port.
  • Messages are sent in the websocket frames. This'll be some rpc-like protocol, which we can create an npm package for. Methods/events:
    • cdpSubscribe(event: string) -> void asks that js-debug proxy the given event. Prefix-matching via stars allowed, e.g. Debugger.*
    • cdpSend(method: string, params: object, sessionId?: string) -> object sends a request to CDP, returns the resulting response or error
    • do we need any other introspection ability?

Sequence diagram

If you've got something working, I'd like to help bring that in, let me know 🙂

@hediet
Copy link
Member Author

hediet commented Mar 10, 2021

I like that!
But couldn't js-debug expose a typescript API (rathar than a websocket server with an rpc protocol), similiar to the typescript extension or liveshare extension?
I think this might simplify consumption of the API and might even make the API more stable. A light npm package might provide types/helper functions to consume this API.

Also, it would be very nice if this design allows multiple extensions to use this API, not just exclusive access. I guess this is unproblematic as cdbSubscribe can only subscribe to events but not requests issued by the debuggee?

I got something working, but it was just a technical proof of concept, nothing that could help yet. Due to university exams and my masters thesis, I had to put my idea on hold, but I will resume in a couple of months.

@connor4312
Copy link
Member

But couldn't js-debug expose a typescript API (rathar than a websocket server with an rpc protocol), similiar to the typescript extension or liveshare extension?

There's a small difficulty is that the typescript api is only accessible by extensions running in the same extension host. js-debug will always run in the workspace, and this would mean ui extensions don't have access. However I'm not sure if m/any extensions will run into this, there are not many ui extensions.

I guess this is unproblematic as cdbSubscribe can only subscribe to events but not requests issued by the debuggee?

Yep

I had to put my idea on hold, but I will resume in a couple of months.

Ok, thanks for letting me know. Hope your thesis goes well!

@hediet
Copy link
Member Author

hediet commented Mar 10, 2021

There's a small difficulty is that the typescript api is only accessible by extensions running in the same extension host. js-debug will always run in the workspace, and this would mean ui extensions don't have access. However I'm not sure if m/any extensions will run into this, there are not many ui extensions.

I see. Anyways, I think it might make sense to make this a hidden implementation detail of some vscode-js-debug-api npm package, what do you think? I feel like a js debug adapter protocol behind the debug adapter protocol might be quite confusing.

Ok, thanks for letting me know. Hope your thesis goes well!

Thanks! I was lucky and found a nice employer who will let me work on VS Code, I hope I can focus more on VS Code/extensions then ;)

@swissmanu
Copy link
Contributor

swissmanu commented Mar 21, 2021

I've started working with the Edge Devtools team who would like to integrate with js-debug using this mechanism too. I'm not sure how far you've gone, my initial thought for a design would be something like this ...

I am digging my way through a similar solution right now, with three differences:

  • The proxy is exposed as a local socket (for now)
  • The modified version of vscode-js-debug does not expose a requestProxy command (yet; I like this idea!)
  • The modified version of vscode-js-debug tries to skip as much internal processing as possible when the proxy forwards/receives data. I.e., it hooks into the communication stream as early as possible. As I understand your sequence diagram, js-debug takes a more active role?

Good to hear that my approach was not that much off so far 😅 @connor4312 I will adapt my solution to your considerations and keep you posted.

do we need any other introspection ability?

For my project personally, I feel like this is enough.

@swissmanu
Copy link
Contributor

swissmanu commented Mar 22, 2021

I am currently implementing the WebSocket server to proxy the calls from another extension.

I create the CDPProxy instance tied to a DebugAdapter once the first requestCDPProxy DAP message is received. As far as I understand it, the DebugAdapter has access to an instance of the "high-level" CDP API, created by CDPSession (https://github.com/microsoft/vscode-js-debug/blob/main/src/cdp/connection.ts#L225), only. This high-level API is nice to use within js-debugs debug adapter, but makes a more general proxying a bit more involved.

If there would be a way to expose the underlying CDPSession (https://github.com/microsoft/vscode-js-debug/blob/main/src/cdp/connection.ts#L46) to the DebugAdapter, we could use the sessions connection more efficiently, because we would not have to work with the high-level abstractions of it.

@connor4312 Is exposing the CDPSession something you could agree with from a design perspective, or do you have an even better idea how we could do the proxying as simple and fast as possible?

EDIT: I opted for a different approach for now... Please ignore this message :)

@swissmanu
Copy link
Contributor

swissmanu commented Mar 28, 2021

A first implementation for a CDP proxy is available here: https://github.com/swissmanu/vscode-js-debug/tree/bde7c1bbcdfc312b876cbf24f5a4790921d7d896

The communication protocol is available in a separate repository: https://github.com/swissmanu/vscode-js-debug-cdp-proxy-api

Even though this version is still a bit rough around the edges, it already allows other extensions to interact with js-debugs CDP connection. @connor4312 @hediet what do you think?

Once you had a look at my changes, maybe you guys have an idea how I could solve the following, ugly any casts?

I tried a few things, but could not come up with something more satisfying so far.

ps. @connor4312 let me know if you prefer a draft pull request for my changes; I was not sure yet since it's still more kind of a proposal than a "ready to merge change" at its state :)

@connor4312
Copy link
Member

connor4312 commented Mar 31, 2021

Thanks for your work, that looks like a great start!

Once you had a look at my changes, maybe you guys have an idea how I could solve the following, ugly any casts?

There is, but you may not like it 😄

type DomainMethods<D extends keyof Cdp.Api> = {
  [M in keyof Cdp.Api[D]]: Cdp.Api[D][M] extends (params: infer P) => Promise<infer R>
    ? [P, R]
    : never;
};

private async sendToCDP<
  Domain extends keyof Cdp.Api,
  Method extends keyof DomainMethods<Domain>,
  Signature extends DomainMethods<Domain>[Method]
>(domain: Domain, method: Method, params: Signature[0]): Promise<Signature[1]> {
  const agent = this.cdp[domain];

  if (agent) {
    const fn = agent[method];
    if (typeof fn === 'function') {
      return await fn(params);
    } else {
      throw new Error(`Unknown method for domain "${method}"`);
    }
  } else {
    throw new Error(`Unknown domain "${domain}"`);
  }
}

As far as I know there isn't a way to do that for the second case, however, since the overloaded on does not play nicely with this object mapping trick. I'm fine with any's here.

Feel free to put in a PR, that will give me write access and I can help put in tweaks to get this ready to ship 🚢

@swissmanu
Copy link
Contributor

Thank you for your input! I tried to do something similar, but obviously missed the infer part. Nice to have another example how infer can be applied 💪

As far as I know there isn't a way to do that for the second case, however, since the overloaded on does not play nicely with this object mapping trick. I'm fine with any's here.

Let's keep it simple then. Even with the suggested DomainMethods type, we would have to cast the strings received from the socket somewhere. Without introducing a proper parsing, we would not really have improved anything then.

Feel free to put in a PR, that will give me write access and I can help put in tweaks to get this ready to ship 🚢

Nice! I just opened #964. Please let me know how you want to proceed and where I can jump in.

@connor4312
Copy link
Member

Thanks for your work and input, I've documented this here

@connor4312
Copy link
Member

fyi @swissmanu @hediet please be aware of this breaking change in the upcoming nightly #987

@connor4312 connor4312 added this to the May 2021 milestone May 3, 2021
@swissmanu
Copy link
Contributor

thx! will keep that in mind 👍

@connor4312 connor4312 added the verification-needed Verification of issue is requested label Jun 1, 2021
@connor4312
Copy link
Member

@hediet please verify this one!

@swissmanu
Copy link
Contributor

for what it's worth, i have this successfully running with the current release of rxjs-debugging-for-vscode and the latest nightly of js-debug:

https://github.com/swissmanu/rxjs-debugging-for-vscode/blob/698a904bdcc3ab9d3a81ac0dfedcdac14ced0ac4/src/ui/telemetryBridge/cdpClientProvider.ts#L11-L13

@hediet hediet added the verified Verification succeeded label Jun 2, 2021
@hediet
Copy link
Member Author

hediet commented Jun 2, 2021

My toy extension works nicely with this new API, but I cannot get the current stack trace if the extensions connects to the proxy while the debugger is already in a paused state.
This information is just not exposed.

@connor4312
Copy link
Member

connor4312 commented Jun 2, 2021

@hediet We can add replay behavior for the Debugger.paused event here:

this.replay.capture(cdp, 'CSS', 'styleSheetAdded');
cdp.CSS.on('fontsUpdated', evt => {
if (evt.font) {
this.replay.addReplay('CSS', 'fontsUpdated', evt);
}
});
cdp.CSS.on('styleSheetRemoved', evt =>
this.replay.filterReply('CSS', s => s.params.styleSheetId !== evt.styleSheetId),
);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request Request for new features or functionality verification-needed Verification of issue is requested verified Verification succeeded
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants