search

github / VisualStudio

GitHub Extension for Visual Studio
https://visualstudio.github.com
MIT License
2.38k stars 1.2k forks source link

PR failed to load #1539

Closed drguthals closed 6 years ago

drguthals commented 6 years ago

Issue from Twitter: https://twitter.com/bchavez/status/974374196724736000

Following up with more details. This happens when a PR title is clicked in our GitHub pane.

This appears to be the same as #1532.

How to fix

The important steps for me were:

bchavez commented 6 years ago

Hi, thank you for opening the PR @sguthals.

My Visual Studio version is:

devenv_1282

The log file for the GitHub extension per @jcansdale 's request:

extension.log

I started a new log with the current error because the original log was very large and at first glance looked like it contained sensitive information, I still have the original in case your interested.

I did a quick search on the error yesterday and ran across a few hints it might have been related to LibGit2Sharp and the newly imposed SSL/TLS changes on GitHub. Some GoogleFu findings pointed me to an "EasyFix" Microsoft Support site that essentially changed the default ordering of "SSL/TLS" when underlying requests were made with WinHTTP.

Something along these lines: https://github.com/JuliaLang/julia/issues/26167 https://support.microsoft.com/en-us/help/3140245/update-to-enable-tls-1-1-and-tls-1-2-as-a-default-secure-protocols-in

After trying and installing the EasyFix, I pretty much gave up yesterday.

Some possible issues that may be contributing to this problem:

In summary, I won't be able to update Internet Explorer if IE turns out to be the underlying problem with the VS extension.

It would be nice if your GitHub VS extension worked on Windows 7, but it's not a huge deal for me because I prefer TorutiseGit for everything.

Thanks, Brian

:walking_man: :walking_woman: Missing Persons - Walking in L.A.

jcansdale commented 6 years ago

@bchavez,

Thanks for the info. Looking at the versions of VS and the extension you're using, they should be ready for the SSL/TLS changes on GitHub.

Here is someone who was seeing a similar exception: https://mengyiyuanblog.wordpress.com/2016/11/29/configure-bower-with-company-proxy/

Are you working behind a company proxy? If you are, could you try again using a different network?

bchavez commented 6 years ago

Hi @jcansdale,

Nah, I'm not behind any company proxy since I work from home. I do have a firewall on my main dev box that is pretty locked down though. I'll try on a clean Win7 VM install without any firewall rules and try again. Also, it will give me a chance to test the IE8 vs IE9 theory too.

Thanks, Brian

:zap: :runner: "I was struck by lighting. Walkin' down the street..."

bchavez commented 6 years ago

Also, FWIW, don't know if this helps, but Fiddler shows communication with github.com looks okay...

explorer_1306

bchavez commented 6 years ago

More testing on Win7 VM:

GitHub extension v2.2.0.10 works (the one installed with most recent Visual Studio Installer)

vmware_1312

screen_1316

Then update GitHub extension to v2.4.3.1737:

vmware_1317

vmware_1319

screen_1323

And failure. No dice with the latest version of the GitHub extension. Most definitely, the most recent GitHub extension update broke something.

:crown: :gem: "I know everything that shine ain't always gold..."

bchavez commented 6 years ago

More testing on the Win7 VM:

Hope that helps, Brian

:chocolate_bar: :cookie: :lollipop: Ronald Jenkees - Stay Crunchy

grokys commented 6 years ago

@bchavez does this happen with only a single PR, a single repository, or all PRs on all repositories? For example, if you clone this repository (github/VisualStudio) does it fail opening our PRs?

bchavez commented 6 years ago

Hi @grokys , how would you like me to clone the github/VisualStudio repo?

Apparently, the Open in Visual Studio button is broken too.

clonefail

Wow. I feel really bad for new windows developers who try to figure out this open source GitHub stuff out. FWIW, this testing is happening on a new fresh Windows 7 VM with latest updates.

:watch: :city_sunset: "I just can't wait... I just can't wait... for saturday night..."

grokys commented 6 years ago

Hmm, just tried opening the PRs that you were having trouble opening and not seeing the problem here:

2018-03-17_10-20-37

But I'm on Win10 so I wonder if this is a Win7 issue? It's Saturday so I can't look into it properly today, but we'll look into it on Monday. @meaghanlewis do we test on win7?

We should open separate issues for the Open in Visual Studio and performance regression (which again, I'm not seeing here).

bchavez commented 6 years ago

Correct. Just tested in a Win10 VM and the PRs load fine.

However, I still get the performance regression in the Win10 VM:

vmware_1336

Maybe it's because these machines are running inside a VM.

Also, found it strange with some Windows 10 themes the GitHub extension for highlighting selection items just doesn't seem right. The highlight color and foreground text font blend in together so much you can't even read the highlighted item.

screen_1340

To summarize, PRs loading correctly:

Version Windows 7 Windows 10
v2.2.0.10 :heavy_check_mark: :heavy_check_mark:
v2.4.3.1737 :x: :heavy_check_mark:
meaghanlewis commented 6 years ago

Hey @grokys, no I only test in Windows 10, but sounds like it would be valuable to also test in Windows 7. I'll set up a VM with Windows 7 this week.

I also haven't been able to reproduce these issues (viewing pull requests, opening a repo in Visual Studio, or the unreadable highlighted items) on Windows 10.

@bchavez could you open up separate issues for the open in Visual Studio and highlight problem. Also, could you specify the Windows 10 themes that show the highlight problem?

grokys commented 6 years ago

I suspect this may be caused by TLS1 being turned off on github.com:

https://github.com/libgit2/libgit2sharp/issues/1524#issuecomment-354556193

The difference between [Windows] 7 and 10 is what the default value for that key is if it isn't set. The patch for 7 added the key, but didn't actually make TLS 1.1 and 1.2 enabled by default, which is why you still have to add the key to enable them.

However, Windows 10 does enable them by default, so you don't need the key to override the defaults.

jcansdale commented 6 years ago

@grokys, @bchavez mentioned that he tried the EasyFix as also mentioned in the issue you linked: https://github.com/libgit2/libgit2sharp/issues/1524#issuecomment-354553965

I wonder why that didn't fix it? 😕

@bchavez we'll do some testing on Windows 7 and get back to you. Thanks for your quality issue report and research!

bchavez commented 6 years ago

The peculiar thing I find interesting about this bug on Windows 7 is that:

Thanks for the info. Looking at the versions of VS and the extension you're using, they should be ready for the SSL/TLS changes on GitHub.

Maybe, maybe, are you doing any extra HTTPS or LibGit requests AFTER the initial PR data is downloaded that isn't setup for SSL/TLS?

Almost seems like the v2.4.3.1737 GH extension is doing something a little extra under the hood after the initial PR data is downloaded. Then an exception is thrown, and the exception bubbles up the stack that makes "Failed to load Pull Request" display. The DIFF between v2.2.0.10 and v2.4.3.1737 just after the PR data is downloaded would be an interesting read.

Just some food for thought.

:collision: :dizzy: "Crashing, hit a wall. Right now I need a miracle..."

bchavez commented 6 years ago

MmmHmm... that suspicious SSLv3 Handshake Failure:

vmware_1358

Next question is, who is 192.30.255.113 on your network?

chrome_1359

:hourglass_flowing_sand: :mag: "But I still haven't found what I'm for..."

grokys commented 6 years ago

To give a bit of background to this, between v2.2.0.10 and v2.4.3.1737 two things changed:

  1. We upgraded our version of https://github.com/libgit2/libgit2sharp
  2. We do a git fetch after loading the PR details from the API (this is needed because to get line numbers of PR review comments, we need to do diffs, and to do diffs we need to make sure that the PR HEAD and BASE are fetched)

So what is happening, is that the initial load of the PR is working, but then the git fetch fails and the error message is shown.

It would appear to be failing due to TLS1 being turned off and something in either Windows 7 or libgit2sharp (or more precisely a combination of the two) isn't working with later TLS version. Exactly why that is, I'm still not sure.

jcansdale commented 6 years ago

@bchavez I initially thought the following was similar to what you'd tried, but looking at the registry entries I think it might be different.

If you are using Windows 7 or Windows Server 2008 R2, the TLS 1.2 protocol will need to be enabled at the operating system level for .NET Framework (and therefore Visual Studio and Git Credential Manager) to be able to make use of it. These settings are described here and below and in the TLS 1.2 Settings documentation (https://docs.microsoft.com/en-us/windows-server/security/tls/tls-registry-settings#tls-12

reg add "HKLM\SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Protocols\TLS 1.2\Client" /v DisabledByDefault /t REG_DWORD /d 0 /f
reg add "HKLM\SYSTEM\CurrentControlSet\Control\SecurityProviders\SCHANNEL\Protocols\TLS 1.2\Client" /v Enabled /t REG_DWORD /d 1 /f

Source: https://developercommunity.visualstudio.com/content/problem/201457/unable-to-connect-to-github-due-to-tls-12-only-cha.html

Could you give that a try and let me know if it improves things? 🤞

bchavez commented 6 years ago

@jcansdale Ran the commands as administrator:

vmware_1367

vmware_1368

No dice, the problem persists. My bet is on that libgit2sharp dependency update. :confused:

:beach_umbrella: :trumpet: Beach Boys - Good Vibrations (Nick Warren bootleg)

jcansdale commented 6 years ago

@bchavez It looks like @nk111 and @ethomson might be ahead of us. 😉

You'd need to add a certificate validation callback to your code. But if you're using the GitHub for Visual Studio 2017, then you need to open an issue in that project.

https://github.com/libgit2/libgit2sharp/issues/1546

@grokys do you know anything about certificate validation callbacks?

nk111 commented 6 years ago

Also discussed here:

https://github.com/github/VisualStudio/issues/1532

nk111 commented 6 years ago

I think, what's needed is kind of a dialog which lets the user decide if a remote certificate is trusted if validation fails...

https://msdn.microsoft.com/de-de/library/system.net.security.remotecertificatevalidationcallback%28v=vs.110%29.aspx?f=255&MSPPError=-2147217396

https://github.com/libgit2/libgit2sharp/blob/6854a1786fd77e5246a0f688a8a66c46c4ea5d3a/LibGit2Sharp.Tests/desktop/SmartSubtransportFixture.cs

Maybe win7 workstations like mine might have a problem using TLS 1.2 required by github?

nk111 commented 6 years ago

and check this out. There is a possible fix mentioned:

https://github.com/rust-lang/cargo/issues/5066

bchavez commented 6 years ago

Hi @nk111,

Thanks for that Rust/Cargo link! Read through the entire thread and finally got it to work!

vmware_1377

Finally.

The important steps for me were:

Thanks a bunch! That works!

:violin: :palm_tree: "'Cause it's a bittersweet symphony, this life..."

jcansdale commented 6 years ago

@bchavez @nk111 thanks for your research one this! It's great to have a confirmed fix. I'll add that to the description at the top.

@ethomson any chance you could give us some pointers on how to fix using using Libgit2Sharp?

ethomson commented 6 years ago

@jcansdale Should be fixed in the 0.25 prerelease builds... we should have an 0.25 release out directly

jcansdale commented 6 years ago

@ethomson Excellent, thanks for the heads up! How soon do you think we should push a 0.25 version? How stable are the current 0.25 prerelease builds?

meaghanlewis commented 6 years ago

Closing this issue out since a solution has been found for this scenario.