Closed Bug 1495793 Opened 11 months ago Closed 11 months ago

Crash in nsStreamConverter::OnDataAvailable

Categories

(Thunderbird :: General, defect, critical)

58 Branch
x86
Windows 10
defect
Not set
critical

Tracking

(thunderbird_esr6063+ fixed, thunderbird63 fixed, thunderbird64 fixed)

RESOLVED FIXED
Thunderbird 64.0
Tracking Status
thunderbird_esr60 63+ fixed
thunderbird63 --- fixed
thunderbird64 --- fixed

People

(Reporter: wsmwk, Assigned: jorgk)

Details

(Keywords: crash)

Crash Data

Attachments

(2 files)

#89 crash for TB60 

This bug was filed from the Socorro interface and is
report bp-88ff876c-af8d-46e0-b9b1-7cd010181001.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll nsStreamConverter::OnDataAvailable comm/mailnews/mime/src/nsStreamConverter.cpp:899
1 xul.dll `anonymous namespace'::SyncRunnable5<nsIStreamListener, nsIRequest*, nsISupports*, nsIInputStream*, unsigned __int64, unsigned int>::Run comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:251
2 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1040
3 ntdll.dll ntdll.dll@0x41fd6 
4 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:517
5 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:97
6 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:319
7 xul.dll MessageLoop::Run ipc/chromium/src/base/message_loop.cc:299
8 xul.dll nsBaseAppShell::Run widget/nsBaseAppShell.cpp:157
9 xul.dll nsAppShell::Run widget/windows/nsAppShell.cpp:415

=============================================================

Mac crashes perhaps a different issue?  example bp-18fb812d-9706-49d2-bbdf-7868c0181002
nsStreamConverter.cpp:899
aIStream->Read(buf, aLength, &readLen);

We should check aIStream.
Assignee: nobody → jorgk
Attachment #9014316 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9014316 [details] [diff] [review]
1495793-crash-fix.patch

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

LGTM, r=mkmelin
Attachment #9014316 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/f0ac78200614
Fix crash in nsStreamConverter::OnDataAvailable(). r=mkmelin
Status: NEW → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 64.0
I'm having second thoughts on this fix now :-(

Clearly nsIStreamListener.onDataAvailable() shouldn't be called with a null stream, so this check shouldn't hurt, although I'm surprised that it would be necessary. On the other hand there is a code path that doesn't used the parameter, so we could have checked the parameter a little later before dereferencing it. Magnus, any opinion?
Ah well, maybe the caller, comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:251, does strange things. So better to check.
Better like this? Sorry about the churn.
Attachment #9014461 - Flags: review?(mkmelin+mozilla)
What about ENSURE_ARG_POINTER() ? Although placing it in the middle of a function is unusual.
Comment on attachment 9014461 [details] [diff] [review]
1495793-follow-up.patch

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

Either way works for me. r=mkmelin
Attachment #9014461 - Flags: review?(mkmelin+mozilla) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9766dccdd8b8
Follow-up: Move null check right before dereferencing to avoid behaviour change. r=mkmelin
Attachment #9014316 - Flags: approval-comm-esr60?
Attachment #9014316 - Flags: approval-comm-beta+
Forgot to say: Merged patch.
Attachment #9014316 - Flags: approval-comm-esr60? → approval-comm-esr60+
You need to log in before you can comment on or make changes to this bug.