Closed Bug 1495793 Opened 2 years ago Closed 2 years ago

Crash in nsStreamConverter::OnDataAvailable


(Thunderbird :: General, defect)

58 Branch
Windows 10
Not set


(thunderbird_esr6063+ fixed, thunderbird63 fixed, thunderbird64 fixed)

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


(Reporter: wsmwk, Assigned: jorgk-bmo)



(Keywords: crash)

Crash Data


(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/
7 xul.dll MessageLoop::Run ipc/chromium/src/base/
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
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]

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

LGTM, r=mkmelin
Attachment #9014316 - Flags: review?(mkmelin+mozilla) → review+
Pushed by
Fix crash in nsStreamConverter::OnDataAvailable(). r=mkmelin
Closed: 2 years 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]

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

Either way works for me. r=mkmelin
Attachment #9014461 - Flags: review?(mkmelin+mozilla) → review+
Pushed by
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+
See Also: → 1627440
You need to log in before you can comment on or make changes to this bug.