1385375 - Crash in nsPop3Protocol::InitializeInternal

Status: VERIFIED FIXED

(MailNews Core :: Networking: POP, defect)

Description

[Wayne Mery (:wsmwk)]

#7 crash for 55.0b2

oldest build ID I find is 55.0a1 in bp-2dee44e4-2bcd-444f-a43a-f76c80170710.
Crash also appears in 54.0a2 bp-59b5114a-3adb-49e9-8116-f56140170519 and 54.0b3 bp-05eb859b-b0b6-400a-aa89-335700170717

 0 	xul.dll	nsPop3Protocol::InitializeInternal(nsIProxyInfo*)	C:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/local/src/nsPop3Protocol.cpp:562
1 	xul.dll	nsPop3Protocol::OnProxyAvailable(nsICancelable*, nsIChannel*, nsIProxyInfo*, nsresult)	C:/builds/moz2_slave/tb-c-cen-w64-ntly-000000000000/build/mailnews/local/src/nsPop3Protocol.cpp:524
2 	xul.dll	mozilla::net::nsAsyncResolveRequest::DoCallback()	netwerk/base/nsProtocolProxyService.cpp:249
3 	xul.dll	mozilla::net::nsAsyncResolveRequest::OnQueryComplete(nsresult, nsCString const&, nsCString const&)	netwerk/base/nsProtocolProxyService.cpp:211
4 	xul.dll	mozilla::net::ExecuteCallback::Run()	netwerk/base/nsPACMan.cpp:81
5 	xul.dll	nsThread::ProcessNextEvent(bool, bool*)	xpcom/threads/nsThread.cpp:1264
6 	xul.dll	NS_ProcessNextEvent(nsIThread*, bool)	xpcom/threads/nsThreadUtils.cpp:389
7 	xul.dll	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)	ipc/glue/MessagePump.cpp:96 

One of the few users with multiple crashes, Rog, on beta:
bp-655b01eb-442b-4059-a135-f03d40170723
bp-8480061d-b103-4ddd-8dde-30b210170723
bp-a1f99b46-770e-4e66-8bc7-4170c0170723

Comment 1

[Jorg K (CEST = GMT+2)]

The last three from comment #0 all crash on nsPop3Protocol.cpp:551: mailnewsUrl->GetMsgWindow(getter_AddRefs(msgwin));

mailnewsUrl may be null, since we test it explicitly at line 525.

Patch coming.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1385375-crash-nsPop3Protocol-InitializeInternal.patch (v1).

Comment 3

[Kent James (:rkent)]

Comment on attachment 8891609 [details] [diff] [review]
1385375-crash-nsPop3Protocol-InitializeInternal.patch (v1).

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

I'm not sure how this can work without a url, but there is previous precedent for checking that in earlier code, and getting a particular msgWindow is not worth a crash. So r+=me

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2dd67431020a
Fix crash in nsPop3Protocol::InitializeInternal(). r=rkent

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8891609 [details] [diff] [review]
1385375-crash-nsPop3Protocol-InitializeInternal.patch (v1).

#7 crash in TB 55 beta, should fix for TB 56 beta.

Comment 6

[Jorg K (CEST = GMT+2)]

Beta (TB 56):
https://hg.mozilla.org/releases/comm-beta/rev/33f2a35560a7caed5f0f8df3244a0007b82ddd78

Comment 7

[Wayne Mery (:wsmwk)]

Thanks for the patch.

I found a different "oldest" crash, 54.0a1 buildid 20170226030207 bp-f67fadfb-e9f0-47d4-b0fa-ce4e62170226 2017-02-26 13:50:19.
The crash then becomes quite common in 54.0a2 - was probably a topcrash - so clearly a regression

Seems likely a regression from bug 791645, which landed 2017-02-10. Do you agree?

Comment 8

[Jorg K (CEST = GMT+2)]

Absolutely. In bug 791645 we turned our four protocols IMAP, POP, NNTP and SMTP upside down causing plenty of regressions, bug 1367707, bug 1350687, bug 1344538 and this bug here. But we had no choice since M-C removed sync proxy resolution, so we had to switch to async. Bug 791645 had been sitting there in the "too hard" basket until the issue became burning and we had to get it fixed by hook or by crook.

The code that now received the extra check was there in nsPop3Protocol::Initialize() in TB 45 in the sync processing but was refactored into the async processing in nsPop3Protocol::InitializeInternal() in the proxy reshuffle.

Let's see whether the problem now goes away or whether we need more checks, like Kent said: "I'm not sure how this can work without a url, ...".

Comment 9

[Wayne Mery (:wsmwk)]

it's all good progress. no worries.

Comment 10

[Wayne Mery (:wsmwk)]

no nightly crashes since 
56.0a1 	20170730030209
56.0a1 	20170725030207
56.0a1 	20170725030207
56.0a1 	20170724030206 

but nightly crash rate is very low, and nightly updates have been spotty. So we'll need to use beta 56.0b3 stats to determine effectiveness.  one more week

Comment 11

[Wayne Mery (:wsmwk)]

Not 100% clear, but I think the crash rate on beta is not better - https://crash-stats.mozilla.com/signature/?release_channel=beta&signature=nsPop3Protocol%3A%3AInitializeInternal&date=%3E%3D2017-06-21T13%3A06%3A21.000Z&date=%3C2017-09-21T13%3A06%3A21.000Z#graphs

for example bp-a8f226db-2553-4582-ad53-4c5570170912 56.0b3

Comment 12

[Jorg K (CEST = GMT+2)]

So you're saying that despite landing a patch for TB 56 beta, this hasn't improved?

So according to the crash you quote this is crashing in nsPop3Protocol.cpp:572:
That makes total sense:   m_url->GetPort(&port);

So we protected against m_url being null above, but not here, so the crash move down. As Kent said: "I'm not sure how this can work without a url".

So let's skip the entire function in this case.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: 1385375-crash.patch - take 2 (v1)

Comment 14

[Kent James (:rkent)]

Comment on attachment 8910841 [details] [diff] [review]
1385375-crash.patch - take 2 (v1)

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

::: mailnews/local/src/nsPop3Protocol.cpp
@@ +520,5 @@
>  
>    m_proxyRequest = nullptr;
>  
> +  if (!m_url)
> +    return NS_OK;

When I see InitializeInternal used, the next thing it is going to do is to try to load the URL (which will not exist).

Ideally we would figure out the conditions that cause m_url to be null, but I spent a few minutes and it was not obvious. But the null value WILL cause failures later, so better to fail now rather than later.

That is, I would prefer:

NS_ENSURE_TRUE(m_url, NS_ERROR_NOT_INITIALIZED);

r=me with that change, if you agree with it.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1385375-crash.patch - take 2 (v2)

Addressed review issues. Yes, I agree, Kent. Thanks.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8914012 [details] [diff] [review]
1385375-crash.patch - take 2 (v2)

That needs uplift to join its brother on TB 57.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/94a4ec7dff4b
Fix crash in nsPop3Protocol::InitializeInternal() - take 2. r=rkent

Comment 18

[Jorg K (CEST = GMT+2)]

Beta (TB 57)
https://hg.mozilla.org/releases/comm-beta/rev/cd796e4f34e95f3dda6b2851d2b31c2a5c915956

Comment 19

[Wayne Mery (:wsmwk)]

No crashes so far in 57 beta, so I think it is dead.  Thanks for the patches.