1734609 - Crash in [@ mozilla::net::Http2Session::IsReused]

Status: RESOLVED FIXED

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

Description

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

Not sure if from bug 1718520 or not - the crash signature existed before and 3 crashes had been observed 2 days before that bug landed. The frequency definitely increased.

Maybe Fission related. (DOMFissionEnabled=1)

Crash report: https://crash-stats.mozilla.org/report/index/3a3d620b-78f0-4bb5-82cd-b3c3d0211006

Reason: EXCEPTION_ACCESS_VIOLATION_READ

Top 10 frames of crashing thread:

0 xul.dll mozilla::net::Http2Session::IsReused netwerk/protocol/http/Http2Session.cpp:4433
1 xul.dll mozilla::net::nsHttpTransaction::Close netwerk/protocol/http/nsHttpTransaction.cpp:1418
2 xul.dll mozilla::net::Http2Stream::Close netwerk/protocol/http/Http2Stream.cpp:1176
3 xul.dll mozilla::net::Http2Session::Shutdown netwerk/protocol/http/Http2Session.cpp:200
4 xul.dll mozilla::net::Http2Session::Close netwerk/protocol/http/Http2Session.cpp:3719
5 xul.dll mozilla::net::nsHttpConnection::CloseTransaction netwerk/protocol/http/nsHttpConnection.cpp:1669
6 xul.dll mozilla::net::nsHttpConnection::OnOutputStreamReady netwerk/protocol/http/nsHttpConnection.cpp:2375
7 xul.dll mozilla::net::nsSocketOutputStream::OnSocketReady netwerk/base/nsSocketTransport2.cpp:514
8 xul.dll mozilla::net::nsSocketTransport::OnSocketReady netwerk/base/nsSocketTransport2.cpp:2057
9 xul.dll mozilla::net::nsSocketTransportService::Run netwerk/base/nsSocketTransportService2.cpp:1150

Comment 1

[Dragana Damjanovic [:dragana]]

Bug bug 1718520 landed a day after this has started. It is probably not that bug.

Comment 2

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

There are crash reports for this with Firefox 94.0. Could you take a look at the issue?

Comment 3

[Kershaw Chang [:kershaw]]

It seems that Http2Session::mConneciton is null, but it's unclear how this can happen.
Http2Session::mConneciton is only set to null at here, but the stacktrace shows that this is the first time that Http2Session::Close is called.
I guess that somehow nsHttpTransaction::mConnection is pointed to a already closed Http2Session. I'll try to create a diagnostic patch.

Comment 4

[Kershaw Chang [:kershaw]]

Attachment: Bug 1734609 - Make sure we don't dispatch a transaction more than once, r=#necko

Comment 5

[Kershaw Chang [:kershaw]]

Attachment: Bug 1734609 - Fix the test:testAllRecordsInHttp3ExcludedList, r=#necko

Depends on D130250

Comment 6

[Kershaw Chang [:kershaw]]

Attachment: Bug 1734609 - Avoid crashing when mConnection is null, r=#necko

Comment 7

[Kershaw Chang [:kershaw]]

Keeps this bug open to investigate the real root cause.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09b7f5f688ac
Avoid crashing when mConnection is null, r=necko-reviewers,valentin

Comment 9

[Kershaw Chang [:kershaw]]

Comment on attachment 9250005 [details]
Bug 1734609 - Avoid crashing when mConnection is null, r=#necko

Beta/Release Uplift Approval Request

• User impact if declined: Firefox could crash
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• 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): This patch only adds a null check to avoid crashing, so the risk is low.
• String changes made/needed: no

Comment 10

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/09b7f5f688ac

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9250005 [details]
Bug 1734609 - Avoid crashing when mConnection is null, r=#necko

Approved for 96.0b6. Could be a good ride-along if we spin a 94 dot release too.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/163c739c27f6

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9250005 [details]
Bug 1734609 - Avoid crashing when mConnection is null, r=#necko

Approved for 94.0.2.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/d83fb2ad60fa

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c376158bf365
Make sure we don't dispatch a transaction more than once, r=necko-reviewers,dragana
https://hg.mozilla.org/integration/autoland/rev/9cabacd358e2
Fix the test:testAllRecordsInHttp3ExcludedList, r=necko-reviewers,dragana

Comment 16

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/c376158bf365
https://hg.mozilla.org/mozilla-central/rev/9cabacd358e2

Comment 17

[Calixte Denizet (:calixte)]

This crash report shows that an added assert has been hit (MOZ_DIAGNOSTIC_ASSERT(mConnection)):
https://crash-stats.mozilla.org/report/index/31c33f02-4f00-4623-bc95-e4b760211119

Comment 18

[Kershaw Chang [:kershaw]]

0 	xul.dll 	mozilla::net::Http2Session::SanityCheck() 	netwerk/protocol/http/Http2Session.cpp:4672 	context
1 	xul.dll 	mozilla::net::nsHttpTransaction::SetConnection(mozilla::net::nsAHttpConnection*) 	netwerk/protocol/http/nsHttpTransaction.cpp:531 	cfi
2 	xul.dll 	mozilla::net::Http2Session::AddStream(mozilla::net::nsAHttpTransaction*, int, bool, bool, nsIInterfaceRequestor*) 	netwerk/protocol/http/Http2Session.cpp:496 	cfi
3 	xul.dll 	mozilla::net::nsHttpConnection::AddTransaction(mozilla::net::nsAHttpTransaction*, int) 	netwerk/protocol/http/nsHttpConnection.cpp:821 	cfi
4 	xul.dll 	mozilla::net::nsHttpConnectionMgr::DispatchTransaction(mozilla::net::ConnectionEntry*, mozilla::net::nsHttpTransaction*, mozilla::net::HttpConnectionBase*) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:1529 	cfi
5 	xul.dll 	mozilla::net::nsHttpConnectionMgr::TryDispatchTransactionOnIdleConn(mozilla::net::ConnectionEntry*, mozilla::net::PendingTransactionInfo*, bool, bool*) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:1483 	cfi
6 	xul.dll 	mozilla::net::nsHttpConnectionMgr::TryDispatchTransaction(mozilla::net::ConnectionEntry*, bool, mozilla::net::PendingTransactionInfo*) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:1388 	cfi
7 	xul.dll 	mozilla::net::nsHttpConnectionMgr::ProcessNewTransaction(mozilla::net::nsHttpTransaction*) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:1702

The stack trace above shows that a h2 connection is reused. This is wrong, since a h2 connection should have mEverUsedSpdy set to true to avoid it being reused. Also. the stack trace shows that mSpdySession is not null, so Http2Session::AddStream is called.
So, the only call path that causes mSpdySession is not null and mEverUsedSpdy is fasle is nsHttpConnection::Start0RTTSpdy.
I think the problem is like:

1. A transaction is dispatched to a h2 connection.
2. We try to do 0RTT.
3. Before 0RTT is finished, the transaction is cancelled, so the h2 connection is reclaimed and being put to the idle connection list.
4. A new transaction is created and it's dispatched to the old h2 connection.

Comment 19

[Kershaw Chang [:kershaw]]

Attachment: Bug 1734609 - Make sure to not reuse a connection that has a mSpdySession, r=#necko

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a6772aa874f3
Make sure to not reuse a connection that has a mSpdySession, r=necko-reviewers,valentin

Comment 21

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/a6772aa874f3

Comment 22

[Kershaw Chang [:kershaw]]

Attachment: Bug 1734609 - Remove the diagnostic check to avoid crashing, r=#necko

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/361325187e7e
Remove the diagnostic check to avoid crashing, r=necko-reviewers,dragana

Comment 24

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/361325187e7e