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

Prevent serialization of phosphor widgets when command calls from plugin #7959

Closed
wants to merge 1 commit into from

Conversation

vzhukovs
Copy link
Contributor

@vzhukovs vzhukovs commented Jun 4, 2020

What it does

This changes proposal adds a check for return value after command been executed from third-party plugin.

The initial problem steps to reproduce:

  1. Call from third-party plugin internal command, which returns a widget, e.g. workbench.files.action.focusFilesExplorer
    packages/plugin-ext/src/main/browser/command-registry-main.ts#L80
  2. Executes command which returns a widget:
    packages/navigator/src/browser/navigator-contribution.ts#L241
  3. Phosphor widget tries to be serialized into JSON, to be provided into plugin, but JSON serializer catch exception with message: TypeError: Converting circular structure to JSON

We need to check return type of the value that is executed and if there is a phosphor widget, return undefined. Anyway, there is no need to pass the whole widget into plugin as callback value.

Fixes issue: #7853

Signed-off-by: Vladyslav Zhukovskyi vzhukovs@redhat.com

How to test

This can be reached by using test plugin, located here.

Review checklist

Reminder for reviewers

Signed-off-by: Vladyslav Zhukovskyi <vzhukovs@redhat.com>
@vzhukovs vzhukovs added bug bugs found in the application Team: Che-Editors issues regarding the che-editors team vscode issues related to VSCode compatibility commands issues related to application commands labels Jun 4, 2020
@vzhukovs vzhukovs self-assigned this Jun 4, 2020
@akosyakov
Copy link
Member

Could you check this PR instead: #7957?

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Jun 4, 2020

Could you check this PR instead: #7957?

Sure, one moment

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Jun 4, 2020

@akosyakov I've checked #7957, seems, that your PR also fixes this problem too.

@vzhukovs
Copy link
Contributor Author

vzhukovs commented Jun 4, 2020

Going to close this PR as redundant. @akosyakov thanks for the fix!

@vzhukovs vzhukovs closed this Jun 4, 2020
@vzhukovs vzhukovs deleted the preventWidgetsSerialization branch June 4, 2020 12:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug bugs found in the application commands issues related to application commands Team: Che-Editors issues regarding the che-editors team vscode issues related to VSCode compatibility
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants