Regression: Toggling shields does not re-enable HTTPSE

[alexwykoff]

Did you search for similar issues before submitting this one?
Yes

Describe the issue you encountered:
When toggling the shield, HTTPSE was not re-enabled.

Expected behavior:
If the shields are up and HTTPSE is enabled, the site should load via https.

• Platform (Win7, 8, 10? macOS? Linux distro?):
  Tested on OS X
• Brave Version:
  0.12.2 rc2
• Steps to reproduce:
  1. Visit http://www.apple.com, notice it becomes https://www.apple.com
  2. Turn off HTTPSE via the shield
  3. Visit http://www.apple.com, notice it stays http://www.apple.com
  4. Turn on HTTPSE via the shield
  5. Visit http://www.apple.com, notice it becomes https://www.apple.com
  6. Toggle 'Shields Down'
  7. Visit http://www.apple.com, notice it stays http://www.apple.com
  8. Toggle 'Shields Up'
  9. Visit http://www.apple.com, notice it stays http://www.apple.com
• Screenshot if needed:
  
• Any related issues:

[bbondy]

This is a URLbar update problem, the state shows it is actually upgrading to HTTPS

[bridiver]

this actually doesn't appear to be a urlbar update problem. When I look at the network traffic it is still going to http://www.apple.com after enabling shields. The brave panel shows 13 upgrades, but http://www.apple.com is not one of them

[bridiver]

it does work correctly on http://https-everywhere.badssl.com/ and updates the address bar which is unfortunate because I don't want to use http://www.apple.com in the test, but I'll figure out what the issue is first

[bbondy]

This was reverted here, so re-opening
1d65e0b

[bbondy]

Also moving this to 0.12.3 since it's just an intermittent race condition that probably happened before as well.

[alexwykoff]

This has been a part of every release test and has never had an issue before. If it was intermittent, it should have been noticed more.

[bridiver]

It is defiantly an intermittent issue. It is intermittent on Apple.com and not reproducible at badssl

On Sep 24, 2016, at 12:06 PM, Alex Wykoff notifications@github.com wrote:

This has been a part of every release test and has never had an issue before. If it was intermittent, it should have been noticed more.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bridiver]

I had actually identified a general race condition before this bug and I verified it when testing this

On Sep 24, 2016, at 12:06 PM, Alex Wykoff notifications@github.com wrote:

This has been a part of every release test and has never had an issue before. If it was intermittent, it should have been noticed more.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bridiver]

also as notes below the page is still on http and only the subresouces that loaded after the race were upgraded

On Sep 24, 2016, at 12:06 PM, Alex Wykoff notifications@github.com wrote:

This has been a part of every release test and has never had an issue before. If it was intermittent, it should have been noticed more.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

[bridiver]

I didn't actually check this because I had already verified the race condition with other methods, but you should see a different number of upgrades when it shows http vs https because some of the upgrades were Apple.com subresouces that should already be https when the page loads as https

On Sep 24, 2016, at 12:06 PM, Alex Wykoff notifications@github.com wrote:

This has been a part of every release test and has never had an issue before. If it was intermittent, it should have been noticed more.

—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub, or mute the thread.

[diracdeltas]

i can no longer repro the original bug - turning shields off then on again and going to http://apple.com updates to https://apple.com as expected.

[bridiver]

@diracdeltas there is a race condition so the issue is intermittent and we haven't done anything that would fix the underlying issue

[bbondy]

@bridiver you had a PR for this that we had to backout, did you ever get back to that?

[bsclifton]

original commit was done w/ 35de6cf

and revert done w/ 1d65e0b #4267

IIRC, this caused some issues... one of them being the ledger not properly considering the status code (I believe because it doesn't get the response)

[luixxiul]

As Apple enables HTTPS by default, let's close this issue. If the issue is still experienced on other sites, let's open a new issue for that.