1495793 - Crash in nsStreamConverter::OnDataAvailable

Status: RESOLVED FIXED

(MailNews Core :: MIME, defect)

Description

[Wayne Mery (:wsmwk)]

#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

Comment 1

[Jorg K (CEST = GMT+2)]

nsStreamConverter.cpp:899
aIStream->Read(buf, aLength, &readLen);

We should check aIStream.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1495793-crash-fix.patch

Comment 3

[Magnus Melin [:mkmelin]]

Comment on attachment 9014316 [details] [diff] [review]
1495793-crash-fix.patch

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

LGTM, r=mkmelin

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f0ac78200614
Fix crash in nsStreamConverter::OnDataAvailable(). r=mkmelin

Comment 5

[Jorg K (CEST = GMT+2)]

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?

Comment 6

[Jorg K (CEST = GMT+2)]

Ah well, maybe the caller, comm/mailnews/imap/src/nsSyncRunnableHelpers.cpp:251, does strange things. So better to check.

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1495793-follow-up.patch

Better like this? Sorry about the churn.

Comment 8

[:aceman]

What about ENSURE_ARG_POINTER() ? Although placing it in the middle of a function is unusual.

Comment 9

[Magnus Melin [:mkmelin]]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9766dccdd8b8
Follow-up: Move null check right before dereferencing to avoid behaviour change. r=mkmelin

Comment 11

[Jorg K (CEST = GMT+2)]

Beta (TB 63):
https://hg.mozilla.org/releases/comm-beta/rev/853d965cb50b3c64343e91fbc58a0e8054206ff9

Comment 12

[Jorg K (CEST = GMT+2)]

Forgot to say: Merged patch.

Comment 13

[Jorg K (CEST = GMT+2)]

TB 60.3 ESR:
https://hg.mozilla.org/releases/comm-esr60/rev/c25d2ff48b8166853fd0234af7b2dcec2dcb5ff0