Closed Bug 1521729 Opened 7 months ago Closed 5 months ago

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

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla68
Tracking Status
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- fixed

People

(Reporter: Gijs, Assigned: kershaw)

References

(Blocks 1 open bug)

Details

(Whiteboard: [qf:p2:responsiveness][necko-triaged])

Attachments

(2 files, 2 obsolete files)

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.

Whiteboard: [qf]

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?
Flags: needinfo?(valentin.gosu)

(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.

  1. 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.

Flags: needinfo?(valentin.gosu)
Whiteboard: [qf] → [qf:p2:responsiveness]

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

Flags: needinfo?(sdeckelmann)

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

Flags: needinfo?(sdeckelmann) → needinfo?(kershaw)
Priority: -- → P2

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

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

Sure.

Assignee: nobody → kershaw
Flags: needinfo?(kershaw)
Whiteboard: [qf:p2:responsiveness] → [qf:p2:responsiveness][necko-triaged]

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.
Attachment #9043909 - Attachment is obsolete: true
  1. Add AsyncCheckIsSecureURI in nsISiteSecurityService.
  2. Make DataStorage queue the pending functions before ready. Flush and execute pending funcs when it's ready.
  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.
Attachment #9044686 - Attachment is obsolete: true
Attachment #9044687 - Attachment description: Bug 1521729 - P2: Support to get the result from NS_ShouldSecureUpgrade asynchronously → Bug 1521729 - P1: Support to get the result from NS_ShouldSecureUpgrade asynchronously

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.
Pushed by kjang@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla68

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.

Depends on: 1536236
Depends on: 1539148
Depends on: 1539081
You need to log in before you can comment on or make changes to this bug.