1521729 - Make nsHttpChannel::AsyncOpen wait (async) for HSTS data to be available before connecting to non-https URLs

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement, P2)

Description

[:Gijs (he/him)]

In bug 1372889, captive portal detection causes a non-https http connection to a server, which causes us to load hsts data, which we do sync and blocks the main thread, causing jank.

To avoid this, in principle we'd need to make Connect() async, but in the short term, it may be easier to make asyncopen check if the data is available, and only proceed with making the connection once it is.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey valentin,

The qf team is trying to triage this, but we need some more information that maybe you can help us with:

1. Are we right when we assume that this will happen on every start-up?
2. Are we right when we assume that this blocks loading any other pages until this operation completes?

Comment 2

[Valentin Gosu [:valentin] (he/him) {{ FYI: OOO 14 June - 4 August }}]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #1)

Hey valentin,

The qf team is trying to triage this, but we need some more information that maybe you can help us with:

1. Are we right when we assume that this will happen on every start-up?

Yes, the captive portal service usually does a check on every start-up. The exception would be Android, where the CPS isn't enabled.

2. Are we right when we assume that this blocks loading any other pages until this operation completes?

Given that bug 1372889 is about the connection blocking the main thread, I think it's fair to say that it blocks loading other pages until the HSTS data is loaded.

Comment 3

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:selenamarie, could you have a look please?

Comment 4

[Selena Deckelmann :selenamarie :selena]

Kershaw -- could you have a look at this bug?

Comment 5

[Kershaw Chang [:kershaw]]

(In reply to Selena Deckelmann :selenamarie :selena use ni? pronoun: she from comment #4)

Kershaw -- could you have a look at this bug?

Sure.

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Bug 1521729 - Add new API to check whether to upgrade https asynchronously

This patch contains changes of necko and security part.
Necko part:

1. Add new API NS_AsyncShouldSecureUpgrade, which is the async version of NS_ShouldSecureUpgrade.
2. Make nsHttpChannel::OnBeforeConnect async.

Security part:

1. Add async version of IsSecureURI, which is only used in NS_AsyncShouldSecureUpgrade.
2. Make DataStorage queue the pending functions before ready. Flush and execute pending funcs when it's ready.

Comment 7

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02373ffcdd0385d4ce749e63b6fe32f1044d6ead

Comment 8

[Kershaw Chang [:kershaw]]

Attachment: Bug 1521729 - P1: Add new API to check the secure URI asynchronously

1. Add AsyncCheckIsSecureURI in nsISiteSecurityService.
2. Make DataStorage queue the pending functions before ready. Flush and execute pending funcs when it's ready.

Comment 9

[Kershaw Chang [:kershaw]]

Attachment: Bug 1521729 - P1: Support to get the result from NS_ShouldSecureUpgrade asynchronously

1. Make nsHttpChannel::OnBeforeConnect async.
2. There are two ways to get the result from NS_ShouldSecureUpgrade. One is getting the result synchronously from the input reference and the other is via the provided callback.
   Note that the callback is only used when the storage is not ready to read at startup.

Comment 10

[Kershaw Chang [:kershaw]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=32f56cffaf3bf234298a1f2ae32b478405edf734&selectedJob=231225017

Comment 11

[Kershaw Chang [:kershaw]]

Attachment: Bug 1521729 - P2: Fix failure tests

Due to the p1 patch, the first time we get the result back from NS_ShouldSecureUpgrade is always asynchronously.
This means that the first http request could be completed slower than the http requests opened after the first one. There are some tests affected by this change, since these tests assume that http requests should be completed in the same order as these requests were created.
This patch fixes the tests by:

1. Move the order of asyncopen calls.
2. Stop the http server until all requests are done.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/917f02e20ab6
P1: Support to get the result from NS_ShouldSecureUpgrade asynchronously r=mayhemer
https://hg.mozilla.org/integration/autoland/rev/70b9b6dbe5b8
P2: Fix failure tests r=mayhemer

Comment 13

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/917f02e20ab6
https://hg.mozilla.org/mozilla-central/rev/70b9b6dbe5b8

Comment 14

[Julien Cristau [:jcristau]]

I'm assuming this is fine to ride the trains to 68, but if not feel free to reset the 67 status to affected and request uplift.