1801292 - Change how necko finishes TLS handshake

Status: NEW

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

Comment 1

[Dragana Damjanovic [:dragana]]

Attachment: Bug 1801292 - Change how necko finishes TLS handshake r=#necko

Currently, NSS calls the handshake completed callback function, HandshakeCallback from nsNSSCallbacks.cpp. HandshakeCallback calls nsNSSSocketInfo::SetHandshakeCompleted that calls TlsHandshaker::HandshakeDone. All of this is happening while NSS holds a lock. When a handshake is done, necko may need to finish setting up Http/2Session(in the nsHttpConnection::HandshakeDoneInternal) and for that, it needs info from NSS, but NSS holds the lock and that is not possible. To resolve this problem necko sets TlsHandshakerTlsHandshaker::mTlsHandshakeComplitionPending to true and dispatches a runnable that will call nsHttpConnection::HandshakeDoneInternal). This dispatch causes bug 1762911, probably due to a shutdown happening just before the runnable is executed.

To resolve the issue and to better integrate HandshakeDoneInternal into necko we will do the following:
TlsHandshaker::HandshakeDone will set mTlsHandshakeComplitionPending to true and call nsHttpConnection::ForceSend. The nsHttpConnection::ForceSend function is a better way to force the handshake to be finished because it is taking already existing standard necko code paths.
nsHttpConnection::ForceSend calls nsHttpConnection::OnOutputStreamReady that will call TlsHandshaker::EnsureNPNComplete that will call nsHttpConnection::HandshakeDoneInternal.

Comment 2

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1801292 - SendConnectRequest should only be called when mState==SETTING_UP_TUNNEL r=#necko

mTlsHandshaker->EnsureNPNComplete() may change nsHttpConnection::mState.
Calling SendConnectRequest() requires that we are in the SETTING_UP_TUNNEL.
As such, we should move the call to EnsureNPNComplete, to ensure we never
call SendConnectRequest when mProxyConnectStream is null.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5178acaddef4
Change how necko finishes TLS handshake r=necko-reviewers,kershaw
https://hg.mozilla.org/integration/autoland/rev/b456d468fd42
SendConnectRequest should only be called when mState==SETTING_UP_TUNNEL r=necko-reviewers,kershaw

Comment 4

[Marian-Vasile Laza]

Backed out 2 changesets (Bug 1801292) for causing bc failures on browser_test_marker_network_serviceworker_no_fetch_handler.js.
Backout link
Push with failures <--> bc4
Failure Log

Comment 5

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

There are some r+ patches which didn't land and no activity in this bug for 2 weeks.
:valentin, could you have a look please?
If you still have some work to do, you can add an action "Plan Changes" in Phabricator.
For more information, please visit auto_nag documentation.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Valentin. You are still assigned to this bug. Do you plan to finish the patches? Thanks!

Comment 7

[Valentin Gosu [:valentin] (he/him)]

I'll do another try push and try to land this in the next few days if it's all looking good.

Comment 8

[Valentin Gosu [:valentin] (he/him)]

Attachment: Bug 1801292 - Adjust test expectation r=#necko

Depends on D160892

Comment 9

[Valentin Gosu [:valentin] (he/him)]

The change is regressing the newly added HTTP3 tests, causing leaks.
https://treeherder.mozilla.org/jobs?repo=try&revision=c505fdfae0970c1e0eebbfda267f402ccec77ee1&selectedTaskRun=Og3jzwb_TDaDDjAxzcER7w.0

I don't think I'll be able to land this very soon.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Hi Valentin, I assume that there are still no free cycles for the network team to have a look at this bug again?

Comment 11

[Kershaw Chang [:kershaw]]

(In reply to Henrik Skupin [:whimboo][⌚️UTC+1] from comment #10)

Hi Valentin, I assume that there are still no free cycles for the network team to have a look at this bug again?

I've added the necko-priority-next flag, so we will discuss this in our triage meeting.

Comment 12

[Valentin Gosu [:valentin] (he/him)]

So, from my previous attempts to land this I think it wouldn't actually fix the leak in bug 1762911.
Those were caused by one of these methods

https://searchfox.org/mozilla-central/rev/9bc264fbc5d6e618d8f3b9677a8f5e8550b94dbc/netwerk/protocol/http/Http2Compression.h#109-113

void GetStatus(nsACString& hdr) { hdr = mHeaderStatus; }
void GetHost(nsACString& hdr) { hdr = mHeaderHost; }
void GetScheme(nsACString& hdr) { hdr = mHeaderScheme; }
void GetPath(nsACString& hdr) { hdr = mHeaderPath; }
void GetMethod(nsACString& hdr) { hdr = mHeaderMethod; }

referencing the nsCString that was in the static table.

For example, GetPath is only called from here so for bug 1762911 comment 1 the leak is probably caused by HTTP2 push .

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Valentin. Does it mean we need a new bug for the potential leak in HTTP2 push?