Closed Bug 1385375 Opened 3 years ago Closed 3 years ago

Crash in nsPop3Protocol::InitializeInternal

Categories

(MailNews Core :: Networking: POP, defect, critical)

Unspecified
Windows 7
defect
Not set
critical

Tracking

(thunderbird_esr52 unaffected, thunderbird54 wontfix, thunderbird56 fixed, thunderbird57 verified)

VERIFIED FIXED
Thunderbird 57.0
Tracking Status
thunderbird_esr52 --- unaffected
thunderbird54 --- wontfix
thunderbird56 --- fixed
thunderbird57 --- verified

People

(Reporter: wsmwk, Assigned: jorgk-bmo)

References

Details

(Keywords: crash, regression, Whiteboard: [regression:TB54])

Crash Data

Attachments

(2 files, 1 obsolete file)

#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
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 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
Attachment #8891609 - Flags: review?(rkent) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2dd67431020a
Fix crash in nsPop3Protocol::InitializeInternal(). r=rkent
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
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.
Attachment #8891609 - Flags: approval-comm-beta+
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?
Assignee: nobody → jorgk
Flags: needinfo?(jorgk)
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, ...".
Flags: needinfo?(jorgk)
Whiteboard: [update tracking]
it's all good progress. no worries.
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
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.
Flags: needinfo?(jorgk)
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.
Attachment #8910841 - Flags: review?(rkent) → review+
Addressed review issues. Yes, I agree, Kent. Thanks.
Attachment #8910841 - Attachment is obsolete: true
Attachment #8914012 - Flags: review+
Comment on attachment 8914012 [details] [diff] [review]
1385375-crash.patch - take 2 (v2)

That needs uplift to join its brother on TB 57.
Attachment #8914012 - Flags: approval-comm-beta+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/94a4ec7dff4b
Fix crash in nsPop3Protocol::InitializeInternal() - take 2. r=rkent
No crashes so far in 57 beta, so I think it is dead.  Thanks for the patches.
Status: RESOLVED → VERIFIED
Whiteboard: [regression:TB54]
Version: 52 → 54
You need to log in before you can comment on or make changes to this bug.