Don't leak RPC objects from API

[jrieken]

This is a continuation of #115530 and an effort to make our API implementation more strict/locked-down.

Most of our API is implemented with the help of RPC calls to the renderer - that can be retrieving data or doing something. This is implemented with a protocol instance and objects that represent a slice of the API, aka proxies, like MainThreadTextEditorsShape. These proxies usually mirror the capabilities of the API but are often more powerful and therefore we should make sure these objects cannot be reached from the API.

These leakages usually occur when having an API implemented as object which holds API-properties and internal/private-properties. Since non-native private properties aren't hidden to consumers an extension can reach them.

There are usually two ways of fixing this

1. Use a native private property - that is a property which name starts with #. Those properties aren't accessible to outsiders but you should know that TypeScript down-level compiles native privates to a construct that uses weak map and is therefore significantly slower. This is a sample commit that hides a proxy by using a native private property: a01d16e
2. A better approach is to not mix API and implementation into a single class/function-scope but to separate them. c399d03 is a sample for that, instead of mixing things the NotebookEditorDecorationType-type has now a value-property which is the API. All internals are hidden inside the containing class/scope. This has an additional benefit of TypeScript checking that your API isn't too large, e.g when using a literal for a certain API-type then the compiler will check that you have no extra properties. In the sample above having the NotebookEditorDecorationType.value.notDeclaredAPI :string-property yield in a compile error.

To test this I have added the assertNoRpcFromEntry and assertNoRpc utilities which recursively crawl an API object and assert that no RPC object can be reached.

These utilities run during teardown and on objects the API creates, like a status bar entry item. The checking for leaked RPC object does depend therefore on test coverage.

[jrieken]

There are a tests that are skipped because they would be failing. Assigning folks

• createStatusBarItem @bpasero
• createSourceControl @lszomoru
• createCommentController @rebornix
• createInputBox @chrmarti (for a better fix than a01d16e)

[jrieken]

ping @eamodio

[jrieken]

ping @rebornix

[jrieken]

ping @eamodio @rebornix

[rebornix]

finished the comment API part. At the beginning I was marking proxy as real private properties but it will still expose non-API methods like eventuallyUpdateCommentThread. To ensure we only expose methods declared in API, a value property suggested above is clean and perfect. @eamodio you may want to do the same for ExtHostSourceControl as it has public functions which are out of the scope of API.

This is a nice pattern to have, thanks for bringing this up @jrieken ❤️ .

[lszomoru]

SCM fixed with 85694fc

[lszomoru]

@jrieken, do you want to keep the issue opened, or can we go ahead and close it?

[jrieken]

Thanks @lszomoru This can be closed 👯 I have added a few more tests recently but didn't find further RPC leaks. There are likely some more left but we can handle them case by case.