search

microsoft / terminal

The new Windows Terminal and the original Windows console host, all in the same place!
MIT License
95.67k stars 8.33k forks source link

Comments in profile.json cause crashes when a tab is closed #2856

Closed iankronquist closed 5 years ago

iankronquist commented 5 years ago

When I add comments to my profile.json I am able to open new tabs, but when I close a tab the whole program hangs. If I remove the comments, closing tabs works fine.

My guess is that you're reparsing the profile.json every time a tab is closed with a different parser, or with the same parser in a mode with comments disabled, and then not handling an exception. Unfortunately, I don't have time to attach a debugger and find out for sure. Thanks for the new terminal, it's an overdue improvement.

DHowett-MSFT commented 5 years ago

This is probably correlation and not causation.

DHowett-MSFT commented 5 years ago

/feedback

iankronquist commented 5 years ago

It's definitely reproducible for me. I encourage you to give it a try.

DHowett-MSFT commented 5 years ago

Oh, whoops: /feedback looks really bad and dismissive when the bot isn't working.

Would you mind filing feedback through the feedback hub, then using the [share feedback] button to get a link you can paste here?

We're only reloading settings when the file changes, so I'm concerned it might be some other bug wearing this one's clothing. :smile:

PhMajerus commented 5 years ago

App shouldn't crash and instead try to recover gracefully or show an error description when configuration files are corrupted by the user, but let's be clear, JavaScript comments in a JSON file make it invalid.

Now we could argue all day about whether comments should be valid in JSON (I think it would greatly improve JSON configuration files), but the specs are unfortunately clear, and whenever Microsoft decides to apply common sense to extend a standard, they get in hot water.

Maybe the Windows Terminal should switch to using JSON5 instead of JSON for its configuration files?

zadjii-msft commented 5 years ago

Woah lets not get into a holy wardiscussion about whether or not json was the right choice for our settings file. The "comments in json" thing is almost certainly a red herring here. Jsoncpp, which we're using for our json parsing, has plenty of support for comments in json ("jsonc"). This is not the case of "Microsoft extending a standard", this is a case of Microsoft leveraging an existing open-source project and standard.

We have a bunch of other issues open that are all akin to "when I close a tab, the Terminal crashes". The root of them is generally #2375. It's much more likely that's at fault here, but without a feedback hub link, we won't be able to positively identify it as a dupe.

ghost commented 5 years ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 4 days. It will be closed if no further activity occurs within 3 days of this comment.