4016-debug-extension: support debugging of extensions using Debug panel by danarad05 · Pull Request #8706 · eclipse-theia/theia

danarad05 · 2020-11-03T14:07:39Z

Signed-off-by: Dan Arad dan.arad@sap.com

What it does

fixed #4016 with this functionality:

Supports start, restart, stop of pwa-extensionHost launch type on debug toolbar.
Utilizes Hosted Plugin functionality.

How to test

Note: can only be tested on browser as electron doesn't support Hosted Plugin functionality currently (see #2868)

Add pwa-extensionHost configuration to launch configurations like so:
start debugging from debug toolbar
two threads will eventually appear after the hosted plugin windows will be opened - like so:
you can choose restart or stop in debug toolbar.

Approved CQ for copied code: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22801

Review checklist

as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

as a reviewer, I agree to behave in accordance with the review guidelines

danarad05 · 2020-11-03T14:11:59Z

FYI: @amiramw @DoroNahari

vince-fugnitto

@danarad05 I do not believe the @theia/debug extension should know about plugins.

cc @colin-grant-work @kenneth-marut-work you may be interested in the following changes which impact debugging.

kittaakos · 2020-11-03T14:53:48Z

I won't have the bandwidth to review the changes this week, but as I see, it requires a CQ. Please start the CQ process as documented here, by the time we have the legal approval for the merge, hopefully, we will have the review too ;)

danarad05 · 2020-11-03T14:56:08Z

@danarad05 I do not believe the @theia/debug extension should know about plugins.

cc @colin-grant-work @kenneth-marut-work you may be interested in the following changes which impact debugging.

@vince-fugnitto I have created a debug ContributionProvider for hosted plugin to registering to. Issue #4016 talks about invoking hosted plugin from debug toolbar (maybe thinking eventually to removing invoking hosted plugin from command box).
@amiramw

danarad05 · 2020-11-03T15:15:06Z

e time we have the legal approval for the merge, hopefully, we will have the review too ;)

Hi @kittaakos

Thanks for your input.
I'm not able to find the intellectual property section - tried also searching for it and the CQ but to no avail. Please advise.
Furthermore, I copied a couple of very small structures - nothing major IMO - if that is an issue I can change it a bit to simplify things... it would be a shame if this process takes time for this small of a change.

Thanks
Fyi: @amiramw

kittaakos · 2020-11-03T15:26:01Z

2. I'm not able to find the intellectual property section - tried also searching for it and the CQ but to no avail. Please advise.

I do not understand this 😊 You do not know how to submit a CQ, is this the question?

3. copied a couple of very small structures - nothing major IMO

I am not a lawyer nor a decision-maker, so I do one thing during the review; if there is copied code, we need a CQ:

theia/packages/debug/src/browser/debug-plugin-contribution.ts

Line 32 in 78664d3

    
           // copied from https://github.com/microsoft/vscode-node-debug2/blob/bcd333ef87642b817ac96d28fde7ab96fee3f6a9/src/nodeDebugInterfaces.d.ts

amiramw · 2020-11-03T15:42:10Z

@danarad05 i'll try to help with the CQ

danarad05 · 2020-11-04T08:07:08Z

I'm not able to find the intellectual property section - tried also searching for it and the CQ but to no avail. Please advise.

I do not understand this 😊 You do not know how to submit a CQ, is this the question?

I meant that explanation in the link you provided didn't get me to the "Intellectual Property" section etc... I don't know if it works for you - I logged in and didn't see and "Intellectual Property" section and tried searching for it. Maybe you should check if explanation there is still valid..

I will try to resolve it with the help of @amiramw, Thanks.

packages/debug/src/browser/debug-plugin-contribution.ts

packages/debug/src/browser/debug-session.tsx

amiramw · 2020-11-15T08:27:59Z

CQ for copied code: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22801

amiramw · 2020-11-16T21:32:26Z

CQ for copied code: https://dev.eclipse.org/ipzilla/show_bug.cgi?id=22801

If I understand correctly the CQ is approved.

danarad05 · 2020-12-08T15:14:28Z

@vince-fugnitto @akosyakov
cc: @amiramw
Hi,
A question I hope you can help me with (or if you can direct me to someone who might help that would be also great):
I'm starting debugging a pwa-extensionHost config type whicheventually creates 4 threads created under Threads pane like so:

When I try to stop debugging - one thread is disposed of but not the others. If I'm clicking stop again - then it disposes all the others.

From what I can see last thread received 'terminated' event (because clicking on stop debug button selects by default last thread to stop) and so it was disposed successfully - BUT previous thread (thread #3 in list) has not received 'terminated' event by DebugSession and therefore it isn't disposed of.
If on the other hand I select first thread and then click 'stop' - all threads are disposed of.

So my questions are:

You have any idea/input concerning why 'terminated' event is not received by DebugSession for 3rd thread in list?
Is it a viable solution in your eyes that when clicking 'stop' debug - first thread will be selected by default instead of last one?

Any suggestions are welcomed. I tried different approaches but to no avail.

Thanks,
-Dan

Issue also reproduced for "Launch VSCode Tests" config:

amiramw · 2020-12-09T20:43:14Z

@danarad05 i don't think you need to address this behavior in this PR as it is not unique for hosted plugin debug.

danarad05 · 2020-12-10T12:35:14Z

@vince-fugnitto @amiramw ready for your review.
p.s. I didn't solve issues with debug stop not closing all threads.

danarad05 · 2021-01-05T08:54:20Z

@vince-fugnitto @amiramw
#8706 (comment)
reminder

danarad05 · 2021-01-19T10:24:26Z

@vince-fugnitto @amiramw
#8706 (comment)
reminder

vince-fugnitto

I tested the feature using pwa-extensionHost and it worked correctly but I'm wondering why the standard extensionHost type is not supported yet (which is the default when generating an extension):

{
    "name": "Run Extension",
    "type": "extensionHost",
    "request": "launch",
    "args": [
        "--extensionDevelopmentPath=${workspaceFolder}"
    ],
    "outFiles": [
        "${workspaceFolder}/out/**/*.js"
    ],
    "preLaunchTask": "${defaultBuildTask}"
},

packages/debug/src/browser/debug-session-contribution.ts

packages/debug/src/browser/debug-session-manager.ts

packages/debug/src/browser/debug-session.tsx

packages/plugin-dev/src/browser/hosted-plugin-controller.ts

packages/debug/src/browser/debug-session.tsx

danarad05 · 2021-01-20T14:44:30Z

I tested the feature using `pwa-extensionHost` and it worked correctly but I'm wondering why the standard `extensionHost` **type** is not supported yet (which is the default when generating an extension):

```json
{
    "name": "Run Extension",
    "type": "extensionHost",
    "request": "launch",
    "args": [
        "--extensionDevelopmentPath=${workspaceFolder}"
    ],
    "outFiles": [
        "${workspaceFolder}/out/**/*.js"
    ],
    "preLaunchTask": "${defaultBuildTask}"
},

I don't know if and when extensionHost type worked. Currently pwa-extensionHost in vscode-js-debug builtin plugin replaced it. See:

vince-fugnitto · 2021-01-20T15:00:41Z

I don't know if and when extensionHost type worked. Currently pwa-extensionHost in vscode-js-debug builtin plugin replaced it. See:

https://github.com/microsoft/vscode-js-debug/blob/master/OPTIONS.md

https://github.com/microsoft/vscode-js-debug/blob/4747389c7d50639738aeb33c1e2f6e2980d2bd0f/COMMON_PROBLEMS.md#my-app-doesnt-run-using-a-node-10-release-before-around-10180

@danarad05 I believe that extensionHost should be supported, not only pwa-extensionHost which only limits debugging a small subset of extensions:

https://github.com/microsoft/vscode/blob/e2bce32da4b7345e776e1b54caf40e30e7da4ad7/src/vs/workbench/contrib/debug/common/debugUtils.ts#L61-L63

Moreover, the extensionHost is the default when generating extensions (using yo code for example), and is the extension type of all the vscode-builtins from which the framework consumes:

The following was just a subset of extension for example purposes. Since the framework supports the debug configurations defined in .vscode/launch.json, I would not expect end-users to prefix pwa- before their type when debugging with the framework.

Please let me know whether you believe this is outside the scope of the pull-request, but we are limiting what the framework can successfully debug, and it looks like extensionHost is the more common type. For my own understanding I wanted to know why pwa- was required but not the standard type.

CHANGELOG.md

packages/debug/src/browser/debug-session.tsx

packages/plugin-dev/src/browser/hosted-plugin-manager-client.ts

packages/plugin-dev/src/node/hosted-instance-manager.ts

danarad05 · 2021-03-31T10:24:27Z

@amiramw fixed and rebased. some convs - please check response. Thanks

packages/plugin-dev/src/browser/hosted-plugin-manager-client.ts

danarad05 · 2021-04-11T14:34:31Z

@kittaakos are you planning to review this PR? Thanks

danarad05 · 2021-05-05T12:23:18Z

@vince-fugnitto @kittaakos
cc: @offer8
anyone planning on reviewing this?

danarad05 · 2021-05-20T10:31:04Z

@vince-fugnitto @kittaakos @marechal-p

Rebased.
In continuation of my remark here - I would like to complete this PR review ASAP.

Thanks

danarad05 · 2021-05-26T13:13:11Z

@vince-fugnitto Are we needing another reviewer approval or is this PR going to be added to this release?

colin-grant-work

I've tested this somewhat superficially in base Theia and against our internal debugging setup. Debugging a VSCode plugin in the browser seemed to work fairly well, though I got a notice on session start that a task 'npm' couldn't befound. Testing against our internal setup, I found that the changes to the session restart logic seem to have broken scenarios that previously worked - the session is killed but doesn't restart - but I haven't been able to track down exactly where the problem occurs. On that basis, I'd prefer that we wait till after the release to merge this, but then, with a little more time to explore its consequences, I'd be happy to work to get it merged next week.

colin-grant-work · 2021-05-26T15:53:06Z

packages/debug/src/browser/debug-session.tsx

+            if (res instanceof Error) {
+                this.fireExited(res);
+            }
+        }).catch(this.fireExited);


I believe that this needs to be this.fireExited.bind(this) or () => this.fireExited(). Otherwise this won't have the right referent and this.connection will be undefined when this.connection['fire']('exited', { reason }); is run.

In fact, it came up in my testing:

colin-grant-work · 2021-05-26T15:53:32Z

CHANGELOG.md

 - [core] updated `nsfw` dependency to `^2.1.2` [#9267](https://github.com/eclipse-theia/theia/pull/9267)
 - [debug] fixed hover issues for the `currentFrame` editor [#9256](https://github.com/eclipse-theia/theia/pull/9256)
 - [debug] improved error messages [#9386](https://github.com/eclipse-theia/theia/pull/9386)
+- [debug] added support for managing debug sessions for extensions from debug panel (previously only possible using `Hosted Plugin` commands) [#8706](https://github.com/eclipse-theia/theia/pull/8706)


We'll need to update the position of this entry in the changelog.

EstherPerelman · 2021-07-14T10:12:42Z

@colin-grant-work updated PR accordingly, Can you merge please?

colin-grant-work · 2021-07-15T23:36:54Z

@EstherPerelman, it looks like more conflicts have cropped up in the CHANGELOG. Sorry it's such a pain. Once those are fixed, I have no objection to merging, if others see no problem.

1. Supports start, restart, stop of pwa-extensionHost launch type on debug toolbar. 2. Utilizes Hosted Plugin functionality. Signed-off-by: Dan Arad <dan.arad@sap.com>

Signed-off-by: Esther Perelman <esther.perelman@sap.com>

EstherPerelman · 2021-07-19T11:01:15Z

@EstherPerelman, it looks like more conflicts have cropped up in the CHANGELOG. Sorry it's such a pain. Once those are fixed, I have no objection to merging, if others see no problem.

@colin-grant-work resolved conflicts, I wish you'll merge soon!

vince-fugnitto

I confirmed that the debugging behavior works fine, and there are no regressions from what I can tell.

vince-fugnitto · 2021-07-19T14:24:15Z

packages/debug/src/browser/debug-session.tsx

        protected readonly messages: MessageClient,
-        protected readonly fileService: FileService) {
+        protected readonly fileService: FileService,
+        protected readonly debugContributionProvider: ContributionProvider<DebugContribution>


Breaking change? Should it be reflected in the changelog?
The same would be true for packages/plugin-ext/src/main/browser/debug/plugin-debug-session-factory.ts even if it is not likely that someone would extend it.

vince-fugnitto · 2021-07-19T14:32:19Z

@msujew is the pull-request something you're interested in taking a look at, I'm not sure if it impacts the work you've done as part of #9613.

msujew · 2021-07-19T14:42:46Z

@vince-fugnitto I took a quick look at it. I'm expecting some small merge conflicts, but nothing major. Feature-wise this is quite independent of my changes. Thanks for watching out though 👍

…el (eclipse-theia#8706) * 4016-debug-extension: support debugging of extensions using Debug panel 1. Supports start, restart, stop of pwa-extensionHost launch type on debug toolbar. 2. Utilizes Hosted Plugin functionality. Signed-off-by: Dan Arad <dan.arad@sap.com> Signed-off-by: Esther Perelman <esther.perelman@sap.com> Co-authored-by: Dan Arad <dan.arad@sap.com> Co-authored-by: Esther Perelman <esther.perelman@sap.com>

vince-fugnitto reviewed Nov 3, 2020

View reviewed changes

vince-fugnitto added debug issues that related to debug functionality vscode issues related to VSCode compatibility labels Nov 3, 2020

vince-fugnitto requested review from kittaakos and paul-marechal November 3, 2020 14:43

kittaakos added the CQ Required (deprecated) issues requiring a CQ (contributor questionnaire) label Nov 3, 2020

amiramw reviewed Nov 15, 2020

View reviewed changes

danarad05 force-pushed the 4016-debug-extension branch from 156fe9b to c70850e Compare November 15, 2020 10:53

danarad05 force-pushed the 4016-debug-extension branch from e30a678 to 01b6c74 Compare December 2, 2020 10:05

danarad05 force-pushed the 4016-debug-extension branch from 0abacdb to a6d6b1d Compare December 10, 2020 12:31

danarad05 requested review from amiramw and vince-fugnitto December 10, 2020 12:33

vince-fugnitto reviewed Jan 19, 2021

View reviewed changes

packages/debug/src/browser/debug-session-contribution.ts Outdated Show resolved Hide resolved

packages/debug/src/browser/debug-session-manager.ts Show resolved Hide resolved

packages/debug/src/browser/debug-session.tsx Outdated Show resolved Hide resolved

paul-marechal reviewed Jan 19, 2021

View reviewed changes

packages/plugin-dev/src/browser/hosted-plugin-controller.ts Outdated Show resolved Hide resolved

packages/debug/src/browser/debug-session.tsx Show resolved Hide resolved

amiramw reviewed Mar 31, 2021

View reviewed changes

danarad05 force-pushed the 4016-debug-extension branch 3 times, most recently from 884def3 to fcb4cd7 Compare March 31, 2021 10:22

amiramw approved these changes Mar 31, 2021

View reviewed changes

amiramw reviewed Mar 31, 2021

View reviewed changes

packages/plugin-dev/src/browser/hosted-plugin-manager-client.ts Show resolved Hide resolved

danarad05 force-pushed the 4016-debug-extension branch from fcb4cd7 to e2b0f97 Compare May 20, 2021 10:28

vince-fugnitto requested a review from colin-grant-work May 26, 2021 13:14

colin-grant-work reviewed May 26, 2021

View reviewed changes

EstherPerelman force-pushed the 4016-debug-extension branch from e2b0f97 to fa47a99 Compare July 14, 2021 10:09

kittaakos removed their request for review July 18, 2021 12:22

danarad05 and others added 3 commits July 19, 2021 13:55

4016-debug-extension: support debugging of extensions using Debug panel

5d52762

1. Supports start, restart, stop of pwa-extensionHost launch type on debug toolbar. 2. Utilizes Hosted Plugin functionality. Signed-off-by: Dan Arad <dan.arad@sap.com>

Fix catch fireExisted

e3e777d

Signed-off-by: Esther Perelman <esther.perelman@sap.com>

Update CHANGELOG position.

96169ac

Signed-off-by: Esther Perelman <esther.perelman@sap.com>

EstherPerelman force-pushed the 4016-debug-extension branch from fa47a99 to 96169ac Compare July 19, 2021 10:58

vince-fugnitto approved these changes Jul 19, 2021

View reviewed changes

colin-grant-work merged commit 61f229e into eclipse-theia:master Jul 20, 2021

vince-fugnitto mentioned this pull request Jul 20, 2021

Add vscode API DebugSessionOptions #9613

Merged

1 task

vince-fugnitto added this to the 1.16.0 milestone Jul 29, 2021

Conversation

danarad05 commented Nov 3, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What it does

How to test

Review checklist

Reminder for reviewers

Uh oh!

danarad05 commented Nov 3, 2020

Uh oh!

vince-fugnitto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kittaakos commented Nov 3, 2020

Uh oh!

danarad05 commented Nov 3, 2020

Uh oh!

danarad05 commented Nov 3, 2020

Uh oh!

kittaakos commented Nov 3, 2020

Uh oh!

amiramw commented Nov 3, 2020

Uh oh!

danarad05 commented Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

amiramw commented Nov 15, 2020

Uh oh!

amiramw commented Nov 16, 2020

Uh oh!

danarad05 commented Dec 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amiramw commented Dec 9, 2020

Uh oh!

danarad05 commented Dec 10, 2020

Uh oh!

danarad05 commented Jan 5, 2021

Uh oh!

danarad05 commented Jan 19, 2021

Uh oh!

vince-fugnitto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danarad05 commented Jan 20, 2021

Uh oh!

vince-fugnitto commented Jan 20, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danarad05 commented Mar 31, 2021

Uh oh!

Uh oh!

danarad05 commented Apr 11, 2021

Uh oh!

danarad05 commented May 5, 2021

Uh oh!

danarad05 commented May 20, 2021

Uh oh!

danarad05 commented May 26, 2021

Uh oh!

colin-grant-work left a comment

danarad05 commented Nov 3, 2020 •

edited

Loading

vince-fugnitto left a comment •

edited

Loading

danarad05 commented Nov 4, 2020 •

edited

Loading

danarad05 commented Dec 8, 2020 •

edited

Loading

vince-fugnitto commented Jan 20, 2021 •

edited

Loading