1972546 - startup crash [@ nsPop3Sink::IncorporateComplete ]

Status: RESOLVED FIXED

(MailNews Core :: Networking: POP, defect)

Description

https://searchfox.org/comm-central/rev/03ced4f21b3b7b768f25e43238f3018452899239/mailnews/local/src/nsPop3Sink.cpp#492

bp-3b37768b-623c-4fe4-b1ad-12c060250617

Reason:

EXC_BAD_ACCESS / KERN_INVALID_ADDRESS

Top 10 frames:

0  XUL  nsPop3Sink::IncorporateComplete(nsIMsgWindow*, int)  mailnews/local/src/nsPop3Sink.cpp:492
1  XUL  _NS_InvokeByIndex
2  XUL  CallMethodHelper::Invoke()  js/xpconnect/src/XPCWrappedNative.cpp:1620
2  XUL  CallMethodHelper::Call()  js/xpconnect/src/XPCWrappedNative.cpp:1174
2  XUL  XPCWrappedNative::CallMethod(XPCCallContext&, XPCWrappedNative::CallMode)  js/xpconnect/src/XPCWrappedNative.cpp:1120
3  XUL  XPC_WN_CallMethod(JSContext*, unsigned int, JS::Value*)  js/xpconnect/src/XPCWrappedNativeJSOps.cpp:966
4  XUL  CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::...  js/src/vm/Interpreter.cpp:494
4  XUL  js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstru...  js/src/vm/Interpreter.cpp:590
5  XUL  InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason)  js/src/vm/Interpreter.cpp:657
5  XUL  js::CallFromStack(JSContext*, JS::CallArgs const&, js::CallReason)  js/src/vm/Interpreter.cpp:662

Comment 1

[Magnus Melin [:mkmelin]]

Attachment: Bug 1972546 - Fix crash [@ nsPop3Sink::IncorporateComplete ]. r=#thunderbird-back-end-reviewers

Comment 2

[Ben Campbell]

That patch to IncorporateComplete() is Fine as a sticking-plaster, and as an internal-state-assertion.
But the bigger question is: how could it get into this state?

From a (very!) quick peek, m_outFileStream is set in IncorporateBegin(), and the only code path for leaving the stream pointer null will result in IncorporateBegin() returning an error.
So I'd say the real issue is that the pop3 code is continuing to attempt delivery to the nsIPop3Sink - by calling incorporateWrite()/incorporateComplete() - even though the nsIPop3Sink.incorporateBegin() has already told it to get stuffed :-)

Magnus: you probably know the JS side here better than anyone: can you spot any obvious issue there?

Comment 3

[Magnus Melin [:mkmelin]]

Seems like for the receivingMultiLineResponse cases we would not necessarily (easily be able to) bail out on a failed IncorporateBegin().
Not sure those are worth trying to catch it there, as with the added things should work out ok in the end anyway.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/58e864b1e70b
Fix crash [@ nsPop3Sink::IncorporateComplete ]. r=BenC

Comment 5

[Magnus Melin [:mkmelin]]

Comment on attachment 9495165 [details]
Bug 1972546 - Fix crash [@ nsPop3Sink::IncorporateComplete ]. r=#thunderbird-back-end-reviewers

Uplift Approval Request

• Please state case for uplift consideration and ensure bug severity is set: Crash fix
• User impact if declined: May crash
• Is this code covered by automated tests?: No
• Has the fix been verified in Daily?: Yes
• Has the fix been verified in Beta?: Yes
• Needs manual test from QA?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Basically just an added null check to avoid crash if we get into that situation
• String changes made/needed: none

Comment 6

[Corey Bryant]

Comment on attachment 9495165 [details]
Bug 1972546 - Fix crash [@ nsPop3Sink::IncorporateComplete ]. r=#thunderbird-back-end-reviewers

[Triage Comment]
Approved for esr140

Comment 7

[Daniel Darnell [:dandarnell]]

Thunderbird 140.0.1esr:
https://hg.mozilla.org/releases/comm-esr140/rev/e5d54fdfd9dc22966956af985d5d29ee12b17a16