Live-share: 1.23-insiders does not work with Live Share - Live Share VS Code extension uses packages that need to be updated for Electron 2.0

[mjbvz]

Product and Version [VS/VSCode]:1.23.0 insiders
OS Version macos 10.13.3
Live Share Extension Version: 0.2.307
Target Platform or Language [e.g. Node.js]:

Steps to Reproduce / Scenario:

1. Open vs code

Bug

Extension host crashes with:

`` /Applications/Visual Studio Code - Insiders.app/Contents/Resources/app/out/vs/workbench/workbench.main.js:2776 /Applications/Visual Studio Code - Insiders.app/Contents/Frameworks/Code - Insiders Helper.app/Contents/MacOS/Code - Insiders Helper[47246]: ../../vendor/node/src/async-wrap.cc:357:void node::SetupHooks(const FunctionCallbackInfo<v8::Value> &): Assertionenv->async_hooks_init_function().IsEmpty()' failed.
1: node::Abort() [/Applications/Visual Studio Code - Insiders.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Libraries/libnode.dylib]
2: node::Assert(char const* const () [4]) [/Applications/Visual Studio Code - Insiders.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Libraries/libnode.dylib]
3: node::AsyncWrap::Initialize(v8::Local, v8::Local, v8::Local) [/Applications/Visual Studio Code - Insiders.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Libraries/libnode.dylib]
4: v8::internal::StrDup(char const) [/Applications/Visual Studio Code - Insiders.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Libraries/libnode.dylib]
5: v8::internal::compiler::BranchElimination::ControlPathConditions::operator==(v8::internal::compiler::BranchElimination::ControlPathConditions const&) const [/Applications/Visual Studio Code - Insiders.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Libraries/libnode.dylib]
6: v8::internal::compiler::BranchElimination::ControlPathConditions::operator==(v8::internal::compiler::BranchElimination::ControlPathConditions const&) const [/Applications/Visual Studio Code - Insiders.app/Contents/Frameworks/Electron Framework.framework/Versions/A/Libraries/libnode.dylib]
7: 0x2b1a64b8463d
````

This error goes away when we disable the live share extension

[mjbvz]

Possibly https://github.com/electron/electron/issues/10905?

[Chuxel]

@mjbvz Thanks for reporting this and the reference to the electron bug. You could be right. We'll have to take a look.

[bpasero]

I can reproduce, I doubt the liveshare extension can do anything. I believe this got reported in VSCode via https://github.com/Microsoft/vscode/issues/47264 so we can continue the discussion also there.

[Chuxel]

@bpasero Yeah, that looks like the same issue. We'll leave this issue open as reference for those using Insiders for the moment, but I'll tag as external..

[bpasero]

@Chuxel we have since released a new insiders version with the old Electron version. All of this seems to get triggered from a very old dependency to fsstream. More details in https://github.com/electron/electron/issues/10905#issuecomment-379248466

[Chuxel]

@bpasero Replied on mail as well - we don't directly use fsstream but we use the latest version of unzip which appears to have this problem. We also use the latest fs-extra which is also referenced in the above issue (but doesn't appear to use fsstream). Thanks very much for the support and help here!

Net-net though is it sounds like everyone should update their Insiders this morning if they hit this.

[bpasero]

@Chuxel is there an issue for the unzip module using the outdated module?

[Chuxel]

@bpasero I just checked and there is not currently - it does have an outdated dependency. "fstream": ">= 0.1.30 < 1". graceful-fs does appear to have an issue on node 8.3.0 though it does not use fsstream as far as I was able to see: https://github.com/isaacs/node-graceful-fs/issues/116

[bpasero]

@Chuxel wow is that https://github.com/EvanOxfeld/node-unzip which has been changed 4 years ago for the last time? Wouldn't there be an alternative module with the same functionality that is not so outdated?

[Chuxel]

@bpasero Yeah this was done due to other modules hitting compatibility issues with .NET Core. We're keeping this bug open so we can address the dependencies. @davidkpiano will be starting to take a look.

[bpasero]

@Chuxel sounds good, thanks

[lostintangent]

While the extension host crash has been resolved, we have a new issue with Insiders, where guest's receive "corrupted" contents when opening up files: https://github.com/Microsoft/vscode/issues/47363.

@Chuxel Should we update the title of this bug (or open a new one) to ensure that as users update to 1.23, they're aware that we're looking into the latest symptom? (which wouldn't be a crash)

[Chuxel]

@lostintangent Let's update the title of this one and create a new issue for the other behavior. We still have work to do to remove the old dependencies.

[Chuxel]

Opened #184 for garbled text behavior.

[bpasero]

@Chuxel @lostintangent we would like to move forward with the Electron update as soon as possible, would it be possible for you to make the move away from unzip library soon? Our current thinking is that we simply throw an error as soon as an extension (or any dependency like unzip) calls require("natives") to prevent the crash from happening.

[davidkpiano]

~It would (theoretically) - we can depend on yauzl instead of unzip.~

We actually cannot make the switch at this time - @srivatsn can explain.

[Chuxel]

@bpasero Adding another response since I'm not sure if David's edit would have popped up for you. :) I didn't see it.

It sounds like we can't switch because some sort of .NET Core issue - I don't know the details. @srivatsn looked at this for a while (trying a bunch of different libs) so he can fill in the details. We're trying to lock down a big release at the moment heading towards build so we can start stabilizing is part of the challenge for looking into alternatives. I'll let Sri speak to the details - I'll be playing "telephone" otherwise.

[bpasero]

@davidkpiano @Chuxel does that last comment mean you cannot (or will not) work on this until build conference is over? I do not think that is a model that would work for us, we have to move forward with Electron before that time frame.

PS: did you try to use the unzip-stream node module which seems to be a drop-in replacement for the unzip library?

PPS: we need to run an experiment with Code Insiders on the new Electron framework starting next week. This would mean that insiders users have to switch to Stable for this time to use LiveShare, I hope that is OK.

[bpasero]

@davidkpiano @Chuxel I went ahead and forked the unzip node module to change its dependency to fstream to latest so that the malicious natives module is not pulled in. This should be a drop-in replacement for you.

Can you try to change your dependency from "unzip" to "better-unzip" and report back how it goes?

[davidkpiano]

@bpasero I'm on it - I'm checking out node-unzipper which seems to be an up-to-date fork of unzip.

[srivatsn]

Ya we tried yauzl but that fails on some of the zip files we need because those zips don't follow the zip spec apparently.

[davidkpiano]

Verified that node-unzipper works (no natives dependency, and up-to-date).

[Chuxel]

It looks like an update went out to VS Code Insiders with the same version number (1.23-insiders) that blocks the use of natives and as a result Live Share now completely does not work on insiders. This fix is caught in a set of other changes we're validating, but right now only VS Code Stable works.

[bpasero]

@Chuxel yes, as announced and discussed. We will revert this early next week before releasing stable.

If possible, please do not wait with this issue until we put this into a future stable release. Thanks for understanding this 👍

[Chuxel]

We just released v0.2.399 which should resolve this issue!

[bpasero]

@Chuxel awesome, thanks!!