Closed Bug 1385375 Opened 3 years ago Closed 3 years ago
Crash in ns
1.15 KB, patch
|Details | Diff | Splinter Review|
921 bytes, patch
|Details | Diff | Splinter Review|
#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 email@example.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
3 years ago
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
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, ...".
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
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
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 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.
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 firstname.lastname@example.org: 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.
You need to log in before you can comment on or make changes to this bug.