1350687 - Crash in nsPop3Protocol::Abort

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[Wayne Mery (:wsmwk)]

First seen in nightly 55.0a1 build 20170313030207 bp-c364efe9-9f32-4d1e-b24b-e314d2170314. Second in 54.0a2 build 20170314004004 bp-7085aa66-767f-4049-8db2-17a5c2170315. Which makes me wonder if this was the result of an uplift

0 	xul.dll	nsPop3Protocol::Abort()	C:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/local/src/nsPop3Protocol.cpp:1039
1 	xul.dll	nsPop3Protocol::Cancel(nsresult)	C:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/local/src/nsPop3Protocol.cpp:1057
2 	xul.dll	nsPop3Protocol::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, nsresult)	C:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/local/src/nsPop3Protocol.cpp:539
3 	xul.dll	mozilla::net::nsAsyncResolveRequest::DoCallback()	netwerk/base/nsProtocolProxyService.cpp:249
4 	xul.dll	mozilla::net::nsAsyncResolveRequest::OnQueryComplete(nsresult, nsCString const&, nsCString const&)	netwerk/base/nsProtocolProxyService.cpp:211
5 	xul.dll	mozilla::net::ExecuteCallback::Run()	netwerk/base/nsPACMan.cpp:81
6 	xul.dll	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp:1264
7 	xul.dll	NS_ProcessNextEvent(nsIThread*, bool)	xpcom/threads/nsThreadUtils.cpp:389
8 	xul.dll	mozilla::gmp::GeckoMediaPluginServiceParent::Observe(nsISupports*, char const*, char16_t const*)	dom/media/gmp/GMPServiceParent.cpp:303

Comment 1

[Jorg K (CEST = GMT+2)]

This was caused by bug 1344538, the "take 2" on the proxy issue. That was meant to fix the error handling, not produce a crash :-(

Look at the stack:
nsPop3Protocol::OnProxyAvailable() calls
  rv = LoadUrlInternal(m_url);
  if (NS_FAILED(rv)) {
    Cancel(rv); <=== 539
  }

nsPop3Protocol::Cancel() calls

    m_proxyRequest = nullptr;
  }

  Abort(); <==== 1057
  return nsMsgProtocol::Cancel(NS_BINDING_ABORTED);

void nsPop3Protocol::Abort() and that crashes:

  m_nsIPop3Sink->AbortMailDelivery(this);

One line fix: test m_nsIPop3Sink before dereferencing it. Leave it with me.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1350687-pop-abort-crash.patch (v1).

Looks link Cancel()/Abort() is now called in unexpected ways before the protocol is fully initialised. It would be nice to know how we get into this situation.

Comment 3

[Kent James (:rkent)]

Comment on attachment 8851370 [details] [diff] [review]
1350687-pop-abort-crash.patch (v1).

Review of attachment 8851370 [details] [diff] [review]:
-----------------------------------------------------------------

This seems pretty safe, though I wish I understood the design plan for shutting down a protocol better.

Comment 4

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/044bc24451d2870e57a6f2524914f98288c18627

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8851370 [details] [diff] [review]
1350687-pop-abort-crash.patch (v1).

Needs to get fixed in TB 54 (Aurora), too.

Comment 6

[Jorg K (CEST = GMT+2)]

Aurora (TB 54):
https://hg.mozilla.org/releases/comm-aurora/rev/aaa1a42a4334910500af6c29f5f106cfd80792c2

Comment 7

[Jorg K (CEST = GMT+2)]

Wayne, I'm sure you'll let me know how we fare with that fix. With Windows Dailies currently broken (bug 1352456), we might see something on Aurora.

Comment 8

[Wayne Mery (:wsmwk)]

Noted. Looks like we will need a few more days to be able to conclude it's effectiveness

Comment 9

[Wayne Mery (:wsmwk)]

No crashes for nightly builds after 20170329030228 and 54.0a2 builds after 20170330004005 (I wrote this a few days ago)

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks. No question here, so clearing NI.