markdown.api.render does not work the first time when passed a TextDocument

[joelspadin]

Issue Type: Bug

When I try to send a TextDocument to the markdown.api.render command, it always fails the first time it is called. If I send a string, it works as expected.

In my extension, I have code which looks like this:

async function render(markdownFile: Uri) {
    const document = await 
    vscode.workspace.openTextDocument(markdownFile);
    return await vscode.commands.executeCommand('markdown.api.render', document) as string;
}

I insert the result into an HTML template and set it as a WebView's html. The first time I open my webview, I see a loading spinner when hovering my cursor over the view for a couple seconds (I assume the Markdown extension is loading in the background), but when it stops loading the webview is still blank. Sometimes VS Code even crashes instead. If it doesn't crash and I close and re-open my webview, I see the rendered markdown content as expected.

If I set a breakpoint on the executeCommand() line and then step over it, it jumps to c:\Program Files\Microsoft VS Code\resources\app\out\vs\workbench\services\extensions\node\vs\workbench\services\extensions\node\file:__w\1\s\src\vs\workbench\services\extensions\common\rpcProtocol.ts line 319. I see the following locals:

• this: d
• e: Error: e.getText is not a function
• i: undefined

If I change my code to pass the full text of the document...

return await vscode.commands.executeCommand('markdown.api.render', document.getText()) as string;

...then everything works as expected.

I noticed that the documentation for executeCommand() indicates that only certain types are safe to pass, and that TextDocument is not one of them, but https://code.visualstudio.com/updates/v1_38#_markdownapirender indicates that the command accepts either a string or a TextDocument. Since the command works properly once the extension is already loaded, maybe there is an issue with passing the document or keeping it alive while the extension is being loaded?

VS Code version: Code 1.38.0 (3db7e09, 2019-09-03T21:49:13.459Z)
OS version: Windows_NT x64 6.1.7601

System Info

Item	Value
CPUs	Intel(R) Core(TM) i7-3770 CPU @ 3.40GHz (8 x 3392)
GPU Status	2d_canvas: enabled flash_3d: enabled flash_stage3d: enabled flash_stage3d_baseline: enabled gpu_compositing: enabled multiple_raster_threads: enabled_on native_gpu_memory_buffers: disabled_software oop_rasterization: unavailable_off protected_video_decode: unavailable_off rasterization: unavailable_off skia_deferred_display_list: disabled_off skia_renderer: disabled_off surface_synchronization: enabled_on video_decode: unavailable_off viz_display_compositor: disabled_off webgl: enabled webgl2: enabled
Load (avg)	undefined
Memory (System)	31.96GB (8.08GB free)
Process Argv
Screen Reader	no
VM	0%

Extensions (39)

Extension	Author (truncated)	Version
gitignore	cod	0.6.0
arm	dan	0.5.0
xml	Dot	2.5.0
gitlens	eam	9.9.3
tsl-problem-matcher	eam	0.0.4
EditorConfig	Edi	0.13.0
vscode-npm-script	eg2	0.3.9
json-tools	eri	1.0.2
bitbake	Eug	1.1.2
tabsanity	jed	0.0.11
svg	joc	0.1.6
vscode-peacock	joh	3.1.5
chat	kar	0.21.1
nearley	kar	1.0.3
rainbow-csv	mec	1.3.1
python	ms-	2019.9.34474
cpptools	ms-	0.25.1
vscode-typescript-tslint-plugin	ms-	1.2.2
vsliveshare	ms-	1.0.766
vsliveshare-audio	ms-	0.1.64
vsliveshare-pack	ms-	0.3.3
autodocstring	njp	0.3.0
vscode-gitextensions	pmi	1.0.0
vscode-hexdump	sle	1.7.2
config-defaults	spa	1.0.0
rewrap	stk	1.9.1
win-ca	uko	3.1.0
vscode-icons	vsc	9.3.0
better-align	wwm	1.1.6

[joelspadin]

It seems like there are two possible solutions here:

1. Fix message passing so you can safely send a TextDocument.
2. Change the markdown.api.render command so it takes a Uri (which is documented as safe to pass) instead of a TextDocument and loads the document itself.

Disclaimer: I have no idea how passing messages to extensions works. I may be completely off base here.

[skprabhanjan]

@ChaosinaCan , Are you sending a PR for this or can I take this up ?

[joelspadin]

I've done a bit of investigation into this. It seems that the documentation on executeCommand() is incorrect. If the command is not yet registered, then the arguments are serialized and sent by RPC, which strips all methods and type info from objects.

Possible solutions:

1. Change markdown.api.render to take Uri | string. Update the documentation for executeCommand() to something like:

/**
 * Executes the command denoted by the given command identifier.
 *
 * * *Note 1:* When executing an editor command not all types are allowed to
 * be passed as arguments. Allowed are the primitive types `string`, `boolean`,
 * `number`, `undefined`, and `null`, as well as [`Position`](#Position), [`Range`](#Range), [`Uri`](#Uri) and [`Location`](#Location).
 * * *Note 2:* When executing commands that have been contributed by other 
 * extensions, any JSON-serializable objects are additionally allowed.
 * * *Note 3:* There are no restrictions when executing commands that have been
 * contributed by your own extension.

2. Fix executeCommand() so that instead of passing the arguments by RPC, it loads the extension that implements the command and then calls _executeContributedCommand() just like if the command has already been registered.

If solution 1 is acceptable, I can submit a PR. If we want solution 2 or something similar to it, that is a job for someone else.

[skprabhanjan]

IMHO solution 1 should be good , but it is @mjbvz 's call :)

[mjbvz]

The first proposal sounds good to me, but please do a separate PR for the documentation change. Let me know if you have any questions about implementing this

[joelspadin]

@jrieken's commit implements solution 2, which makes everything work like you'd expect given executeCommand()'s current documentation. Thanks!

[mjbvz]

So no work required for this, correct?

[joelspadin]

cf88c7f should resolve this with no further work needed.

Previously when executing a command which is contributed by an unloaded extension, then arguments would be serialized and sent to a proxy object. The extension would then be loaded, and the command would be run given the deserialized arguments. The serialization discarded information from the arguments, so the command would fail.

If I understand the fix correctly, instead of running the command with the deserialized arguments after loading the extension, it will now unwind back to where the arguments were serialized and directly execute the command with the original arguments (which is possible now that the extension is loaded), so the command succeeds.

[mjbvz]

Thanks. Closing this then since it sounds like the issue has already been fixed by cf88c7f