1362959 - Crash in mozilla::net::nsHttpConnection::CloseConnectionFastOpenTakesTooLongOrError

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-720cae20-771a-4875-8494-ad9750170508.
=============================================================
Crashing Thread (11)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::net::nsHttpConnection::CloseConnectionFastOpenTakesTooLongOrError(bool) 	netwerk/protocol/http/nsHttpConnection.cpp:2364
1 	xul.dll 	mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetFastOpenConnected(nsresult) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:3554
2 	xul.dll 	mozilla::net::nsSocketTransport::RecoverFromError() 	netwerk/base/nsSocketTransport2.cpp:1722
3 	xul.dll 	mozilla::net::nsSocketTransport::OnSocketDetached(PRFileDesc*) 	netwerk/base/nsSocketTransport2.cpp:2268
4 	xul.dll 	mozilla::net::nsSocketTransportService::DetachSocket(mozilla::net::nsSocketTransportService::SocketContext*, mozilla::net::nsSocketTransportService::SocketContext*) 	netwerk/base/nsSocketTransportService2.cpp:252
5 	xul.dll 	mozilla::net::nsSocketTransportService::DoPollIteration(mozilla::BaseTimeDuration<mozilla::TimeDurationValueCalculator>*) 	netwerk/base/nsSocketTransportService2.cpp:1087
6 	xul.dll 	mozilla::net::nsSocketTransportService::Run() 	netwerk/base/nsSocketTransportService2.cpp:928
7 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1270
8 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:393
9 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:368
10 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
11 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
12 	xul.dll 	nsThread::ThreadFunc(void*) 	xpcom/threads/nsThread.cpp:501
13 	nss3.dll 	PR_NativeRunThread 	nsprpub/pr/src/threads/combined/pruthr.c:397
14 	nss3.dll 	pr_root 	nsprpub/pr/src/md/windows/w95thred.c:95
15 	ucrtbase.dll 	o__realloc_base 	
16 	kernel32.dll 	BaseThreadInitThunk 	
17 	ntdll.dll 	RtlUserThreadStart

this is a new crash signature appearing on nightly after build 20170506030204 which looks related to the patch in bug 1188435.

Comment 1

[Dragana Damjanovic [:dragana]]

I think I know what is the problem here and I will fix it in bug 1362821.

Comment 2

[Dragana Damjanovic [:dragana]]

This crashes are interesting:
https://crash-stats.mozilla.com/signature/?build_id=%3E20170509000000&version=55.0a1&signature=mozilla%3A%3Anet%3A%3AnsHttpConnection%3A%3ACloseConnectionFastOpenTakesTooLongOrError&date=%3E%3D2017-05-06T18%3A46%3A37.000Z&date=%3C2017-05-09T18%3A46%3A37.000Z&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_columns=install_time&_sort=-date&page=1#reports

I thought that they are going to go away :(

0 	xul.dll 	mozilla::net::nsHttpConnection::CloseConnectionFastOpenTakesTooLongOrError(bool) 	netwerk/protocol/http/nsHttpConnection.cpp:2350
1 	xul.dll 	mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetFastOpenConnected(nsresult) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:3572
2 	xul.dll 	mozilla::net::nsSocketTransport::Close(nsresult) 	netwerk/base/nsSocketTransport2.cpp:2474
3 	xul.dll 	mozilla::net::nsHttpConnectionMgr::TimeoutTick() 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:2868
4 	xul.dll 	mozilla::net::nsHttpConnectionMgr::Observe(nsISupports*, char const*, char16_t const*) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:342
5 	xul.dll 	nsTimerImpl::Fire(int) 	xpcom/threads/nsTimerImpl.cpp:500


So what is happening:
in mozilla::net::nsHttpConnectionMgr::TimeoutTick

2856  if (half->IsFastOpenBackupHalfOpen()) {
2857      // this transport belongs to a fast open connection.
2858      // It will be canceled through that connection.
2859      continue;
2860  }
2861  double delta = half->Duration(currentTime);
2862  // If the socket has timed out, close it so the waiting
2863  // transaction will get the proper signal.
2864  if (delta > maxConnectTime_ms) {
2865      LOG(("Force timeout of half open to %s after %.2fms.\n",
2866      ent->mConnInfo->HashKey().get(), delta));
2867       if (half->SocketTransport()) {
2868           half->SocketTransport()->Close(NS_ERROR_NET_TIMEOUT);
2869      }
...

I have added a check half->IsFastOpenBackupHalfOpen(). This only checks if mConnectionNegotiatingFastOpen if nullptr. 
If mConnectionNegotiatingFastOpen is not nullptr it should continue, skip tis halfOpen. So it does not skip it and calls
half->SocketTransport()->Close(NS_ERROR_NET_TIMEOUT);
which still has pointer to this halfOpen and it calls SetFastOpenConnected(reason);

In SetFastOpenConnected:
3571  MOZ_ASSERT(mConnectionNegotiatingFastOpen);
3572  RefPtr<nsAHttpTransaction> trans = mConnectionNegotiatingFastOpen->CloseConnectionFastOpenTakesTooLongOrError(false);

assertion does not fail, but that halfOpen does not have mConnectionNegotiatingFastOpen.

Writing this down I figured what is the problem :)

I will write a patch.

Comment 3

[Dragana Damjanovic [:dragana]]

The signature from comment 0 does not exist any more. That crash was fix by bug 1362821.

The new crash stack looks like this:

0 	xul.dll 	mozilla::net::nsHttpConnection::CloseConnectionFastOpenTakesTooLongOrError(bool) 	netwerk/protocol/http/nsHttpConnection.cpp:2350
1 	xul.dll 	mozilla::net::nsHttpConnectionMgr::nsHalfOpenSocket::SetFastOpenConnected(nsresult) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:3572
2 	xul.dll 	mozilla::net::nsSocketTransport::Close(nsresult) 	netwerk/base/nsSocketTransport2.cpp:2474
3 	xul.dll 	mozilla::net::nsHttpConnectionMgr::TimeoutTick() 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:2868
4 	xul.dll 	mozilla::net::nsHttpConnectionMgr::Observe(nsISupports*, char const*, char16_t const*) 	netwerk/protocol/http/nsHttpConnectionMgr.cpp:342
5 	xul.dll 	nsTimerImpl::Fire(int) 	xpcom/threads/nsTimerImpl.cpp:500

This crash was cause by a small mistake in patch for bug 1362821. (I forgot to add "if (mFDFastOpenInProgress)")

Comment 4

[Dragana Damjanovic [:dragana]]

Attachment: bug_1362959_v1.patch

This patch adds "If (mFDFastOpenInProgress)" to nsSocketTransport::Close.

The rest of a patch is just an assertion and 2 not that imporant changes.

Comment 5

[Dragana Damjanovic [:dragana]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c4f19db8c36c6415572f3a2681da141426ddcd94

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d45a5b8429e7
Call SetFastOpenConnected only if socketTransport has TFO in progress. r=mcmanus

Comment 7

[Carsten Book [:Tomcat]]

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