1940508 - ESR 128.6.0 Crash in [@ mozilla::net::nsHttpConnection::HandshakeDoneInternal]

Status: RESOLVED FIXED

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

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This is topcrash #2 for ESR 128.6.0 (373 crashes from 131 installs), affects all OS and 3/4 of the crashes are in the first 5 minutes after launch. 75% of the Firefoxes are in Russian, 21% in en-US. There are no such crashes for Firefox 128.5.2esr.

Kershaw, could you investigate this? This is the pushlog. The error is an NPN transaction failure.

Crash report: https://crash-stats.mozilla.org/report/index/ad155441-96fe-40e6-a506-842380250108

Reason:

EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames:

0  xul.dll  mozilla::net::nsHttpConnection::HandshakeDoneInternal()  netwerk/protocol/http/nsHttpConnection.cpp:2439
0  xul.dll  mozilla::net::TlsHandshaker::HandshakeDone::<lambda_18>::operator()() const  netwerk/protocol/http/TlsHandshaker.cpp:63
0  xul.dll  mozilla::detail::RunnableFunction<`lambda at /builds/worker/checkouts/gecko/n...  xpcom/threads/nsThreadUtils.h:548
1  xul.dll  nsThread::ProcessNextEvent(bool, bool*)  xpcom/threads/nsThread.cpp:1198
1  xul.dll  NS_ProcessNextEvent(nsIThread*, bool)  xpcom/threads/nsThreadUtils.cpp:480
2  xul.dll  mozilla::net::nsSocketTransportService::Run()  netwerk/base/nsSocketTransportService2.cpp:1198
3  xul.dll  nsThread::ProcessNextEvent(bool, bool*)  xpcom/threads/nsThread.cpp:1198
3  xul.dll  NS_ProcessNextEvent(nsIThread*, bool)  xpcom/threads/nsThreadUtils.cpp:480
4  xul.dll  mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*)  ipc/glue/MessagePump.cpp:300
5  xul.dll  MessageLoop::RunInternal()  ipc/chromium/src/base/message_loop.cc:370

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Bug 1929156 seems most likely.

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

I suspect we need an "if (mTransaction) { ... }" around the Close() call

Comment 4

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

Attachment: Bug 1940508 - Add null check to HandshakeDoneInternal before using mTransaction r=#necko

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 9446387 [details]
Bug 1940508 - Add null check to HandshakeDoneInternal before using mTransaction r=#necko

ESR Uplift Approval Request

• If this is not a sec:{high,crit} bug, please state case for ESR consideration: Fix a crash in an uplifted patch
• User impact if declined: Users (in russia?) will see crashes
• Fix Landed on Version: 134 (uplifted)
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): null-check

Beta/Release Uplift Approval Request

• User impact if declined/Reason for urgency: Crashes for some users, mostly in russia
• Is this code covered by automated tests?: Unknown
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): adds nullcheck
• String changes made/needed: none
• Is Android affected?: Unknown

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e238e8b0efa
Add null check to HandshakeDoneInternal before using mTransaction r=necko-reviewers,jesup

Comment 7

[Kershaw Chang [:kershaw]]

I think we need to investigate further why we are encountering so many NPN mismatches.
Perhaps the negotiatedNPN is empty?

Valentin, what do you think about adding a MOZ_DIAGNOSTIC_CRASH here and use crash reporter to collect the NPN token?

Comment 8

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

If mozilla::psm::TransportSecurityInfo::GetNegotiatedNPN returns an error code that means mNPNCompleted is false.
Not sure if a MOZ_DIAGNOSTIC_CRASH gives us enough information.
What we could do is to add NS_SUCCEEDED(rvDebug) to the if (!transactionNPN.IsEmpty() && negotiatedNPN != transactionNPN) { condition.

Comment 9

[Cristina Horotan [:chorotan]]

https://hg.mozilla.org/mozilla-central/rev/1e238e8b0efa

Comment 10

[Kershaw Chang [:kershaw]]

(In reply to Valentin Gosu [:valentin] (he/him) {{ PTO 21 Dec - 06 Jan }} from comment #8)

If mozilla::psm::TransportSecurityInfo::GetNegotiatedNPN returns an error code that means mNPNCompleted is false.
Not sure if a MOZ_DIAGNOSTIC_CRASH gives us enough information.
What we could do is to add NS_SUCCEEDED(rvDebug) to the if (!transactionNPN.IsEmpty() && negotiatedNPN != transactionNPN) { condition.

I assume we could do something like this to annotate the NPN strings.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9446387 [details]
Bug 1940508 - Add null check to HandshakeDoneInternal before using mTransaction r=#necko

Approved for 135.0b3.

Comment 12

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/5d68d40f767e

Comment 13

[Pascal Chevrel:pascalc]

Comment on attachment 9446387 [details]
Bug 1940508 - Add null check to HandshakeDoneInternal before using mTransaction r=#necko

Taking as a ride-along in 134.0.2, thanks.

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-release/rev/3343d99a81e7

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9446387 [details]
Bug 1940508 - Add null check to HandshakeDoneInternal before using mTransaction r=#necko

Approved for 128.7esr.

Comment 16

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-esr128/rev/2647a6742e75