2051776 - MOZ_ASSERT(mState == State::Open) on late loser Finish0RTT after Cleanup

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Initially found with m-c 20260624-3fbd071a3248. I have not been able to reproduce the issue.

Crash signature: MOZ_ASSERT(mState == State::Open) "Finish0RTT declaring winner on a non-Open handle" at netwerk/protocol/http/ZeroRttHandle.cpp:198 (reported as SIGSEGV / SEGV_MAPERR @ 0x0 on Android, which is how MOZ_CRASH manifests there).

Crash stack (socket thread):

  mozilla::net::ZeroRttHandle::Finish0RTT   ZeroRttHandle.cpp:198
  mozilla::net::nsHttpConnection::HandshakeDoneInternal   nsHttpConnection.cpp:2648
  mozilla::net::TlsHandshaker::HandshakeDone (runnable)
  mozilla::net::nsSocketTransportService::Run

Root cause:

ZeroRttHandle is the shared 0-RTT coordinator for one Happy Eyeballs race. Every racer's nsHttpConnection calls Finish0RTT() on the same handle when its TLS handshake completes, so the handle must tolerate a late Finish0RTT from a losing racer after a winner has already been declared and the handle cleaned up.

The loser short-circuit guard checks the wrong field:

    // ZeroRttHandle.cpp:188
    if (mWinner) {
      // Late Finish0RTT on a loser ... ignore
      return NS_OK;
    }
    MOZ_ASSERT(mState == State::Open, ...);   // :198 fires

The normal winner path calls Cleanup() (line 285), and Cleanup() -> Transition(State::CleanedUp) nulls mWinner (line 352) to break the RefPtr cycle, while leaving mState == State::CleanedUp. So once a winner has been declared and cleaned up:

• mWinner is nullptr, so the guard at line 188 is not taken
• mState is CleanedUp, so the assert at line 198 fires

The header documents the field that should have been used: mHadWinner is described as outliving mWinner precisely so callers can distinguish "no winner yet" from "winner declared and then cleaned up". The guard consults mWinner instead.

Two ways to reach it:

1. Winner then loser (most likely): racer A wins -> WinnerDeclared -> Cleanup() nulls mWinner, mState = CleanedUp. Racer B's handshake completes -> Finish0RTT -> mWinner null, mState == CleanedUp -> assert.

2. "Real txn gone" path: the first Finish0RTT takes the !realTxn branch (line 243) and calls Cleanup() at line 250 without ever setting mHadWinner. A subsequent Finish0RTT then also passes the mWinner guard and asserts. This variant would NOT be caught by an mHadWinner-based guard; only an mState != State::Open check covers both.

Suggested fix:

Key the loser short-circuit off lifecycle state rather than the transient mWinner pointer:

if (mState != State::Open) {
  // Winner already declared (and possibly cleaned up), or the handle
  // was torn down on the real-txn-gone path. Late loser Finish0RTT: no-op.
  return NS_OK;
}

This subsumes the existing mWinner guard and closes both entry paths, making the MOZ_ASSERT(mState == State::Open) at line 198 genuinely unreachable.

Comment 1

[Max Inden]

I believe this was introduced in Bug 2025276. Adding severity S3, given that Happy Eyeballs is in Nightly only.

@Kershaw would you mind taking a look? Please let me know if you would prefer me to prepare a patch.

Comment 2

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

Set release status flags based on info from the regressing bug 2025276

Comment 3

[Kershaw Chang [:kershaw]]

Attachment: Bug 2051776 - Tolerate a late loser Finish0RTT after ZeroRttHandle cleanup, r=#necko

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e7edb37d7252
https://hg.mozilla.org/integration/autoland/rev/b3aac5bf2b1a
Tolerate a late loser Finish0RTT after ZeroRttHandle cleanup, r=necko-reviewers,valentin

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/106d45bb88ac
https://hg.mozilla.org/integration/autoland/rev/8003883fc418
Revert "Bug 2051776 - Tolerate a late loser Finish0RTT after ZeroRttHandle cleanup, r=necko-reviewers,valentin" for causing build bustages

Comment 6

[Silaghi Andreea]

Backout for causing build bustages

push with failure
failure log

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/e16f27031559
https://hg.mozilla.org/integration/autoland/rev/23ac20ee9771
Tolerate a late loser Finish0RTT after ZeroRttHandle cleanup, r=necko-reviewers,valentin

Comment 8

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/23ac20ee9771