17597 - [DOGFOOD] no body when replying to news messages

Status: VERIFIED FIXED

(MailNews Core :: MIME, defect, P3)

Description

[Scott MacGregor]

In today's 10/29 commercial windows build I crash when I try to reply to
newsgroup messages in n.p.m.mailnews.


I crash here in nsStreamConverter::OnStopRequest
mimeHeaders->Initialize(msd->headers->all_headers);

because msd->headers is null so we dereference a null ptr.
I tried to add some bullet proofing earlier in the method where we do
if (msd) such that it looks like if (msd && msd->headers)

and that did fix the crash but then the message body wasn't gettting quoted.

So it looks like the fix needs more than that.

Sending over to rhp. Nominating for dogfood.

Comment 1

[lchiang]

We have a bug on this already, filed by marina, I think. I'll find it.

Comment 2

[lchiang]

The other bug is http://bugzilla.mozilla.org/show_bug.cgi?id=17463, assigned to
ducarroz

Comment 3

[leger]

Putting on PDT+ radar.

Comment 4

[rhp (gone)]

Over to jeff :-)

Comment 5

[jefft]

Accepting M11....

Comment 6

[rhp (gone)]

*** Bug 17463 has been marked as a duplicate of this bug. ***

Comment 7

[Jean-Francois Ducarroz]

Jeff, I wrote this code, I can fix it if you want. Just reassign it to me...

Comment 8

[jefft]

Reassing to ducarroz per request....

Comment 9

[Jean-Francois Ducarroz]

So far, I am not able to reproduce the crash on Mac but I can see that the original body is missing.

RHP, I think I would need you help on this one. the mime stream converter is created, as well the HTML emitter. I get
the nsStreamConverter::OnStartRequest and nsStreamConverter::OnStopRequest but I never get the
nsStreamConverter::OnDataAvailable! Therefore no body. Any Idea?

Comment 10

[rhp (gone)]

Sounds like a problem getting the data from the NNTP server?
mscott, seth...any ideas?

- rhp

Comment 11

[Jean-Francois Ducarroz]

It works when we display the message (uses XUL emitter) but not when we try to quote it (uses HTML emitter). I am
not quiet sure it has someting to do with the NNTP server communication.

Comment 12

[rhp (gone)]

Then it could be the way libmime is being used as a stream converter for this
case. Weird...I'm really engrossed in a complicated mail compose issue right
now, but I will try to look at this later if I can.

- rhp

Comment 13

[Jean-Francois Ducarroz]

Reassign to rhp for the body missing issue.

Comment 14

[rhp (gone)]

Ok, I can fix the crashes we were seeing by initializing some variables that
are garbage, never initialized and then PR_Free()'ed. Still looking into why we
aren't getting the quoting data.

- rhp

Comment 15

[rhp (gone)]

sspitzer is going to look why we are not getting data from the NNTP query. This
seems to be why libmime is not quoting any real data...it's not getting any
data.

I have a 2 line fix for the null pointer crashing part of this problem:

The first will be to update mozilla\mailnews\mime\src\nsStreamConverter.cpp and
put a check before accessing through a null pointer so:

        mimeHeaders->Initialize(msd->headers->all_headers);

changes to:

        if (msd->headers)
          mimeHeaders->Initialize(msd->headers->all_headers);

Also, make the following changes in
mozilla\mailnews\compose\src\nsMsgCompose.cpp:

This will fix a bunch of possible garbage pointer frees.

Index: mozilla/mailnews/compose/src/nsMsgCompose.cpp
===================================================================
RCS file: /cvsroot/mozilla/mailnews/compose/src/nsMsgCompose.cpp,v
retrieving revision 1.99
diff -p -r1.99 nsMsgCompose.cpp
*** nsMsgCompose.cpp	1999/11/03 02:46:15	1.99
--- nsMsgCompose.cpp	1999/11/03 22:20:12
*************** NS_IMETHODIMP QuotingOutputStreamListene
*** 827,833 ****
  	      	nsString replyTo;
            nsString newgroups;
            nsString followUpTo;
!           char *outCString;
            PRUnichar emptyUnichar = 0;

            mHeaders->ExtractHeader(HEADER_REPLY_TO, PR_FALSE, &outCString);
--- 827,833 ----
  	      	nsString replyTo;
            nsString newgroups;
            nsString followUpTo;
!           char *outCString = nsnull;
            PRUnichar emptyUnichar = 0;

            mHeaders->ExtractHeader(HEADER_REPLY_TO, PR_FALSE, &outCString);
*************** NS_IMETHODIMP QuotingOutputStreamListene
*** 835,841 ****
            {
              // Convert fields to UTF-8
              ConvertToUnicode(aCharset, outCString, replyTo);
!             PR_Free(outCString);
            }

            mHeaders->ExtractHeader(HEADER_NEWSGROUPS, PR_FALSE, &outCString);
--- 835,841 ----
            {
              // Convert fields to UTF-8
              ConvertToUnicode(aCharset, outCString, replyTo);
!             PR_FREEIF(outCString);
            }

            mHeaders->ExtractHeader(HEADER_NEWSGROUPS, PR_FALSE, &outCString);
*************** NS_IMETHODIMP QuotingOutputStreamListene
*** 843,849 ****
            {
              // Convert fields to UTF-8
              ConvertToUnicode(aCharset, outCString, newgroups);
!             PR_Free(outCString);
            }

            mHeaders->ExtractHeader(HEADER_FOLLOWUP_TO, PR_FALSE, &outCString);
--- 843,849 ----
            {
              // Convert fields to UTF-8
              ConvertToUnicode(aCharset, outCString, newgroups);
!             PR_FREEIF(outCString);
            }

            mHeaders->ExtractHeader(HEADER_FOLLOWUP_TO, PR_FALSE, &outCString);
*************** NS_IMETHODIMP QuotingOutputStreamListene
*** 851,857 ****
            {
              // Convert fields to UTF-8
              ConvertToUnicode(aCharset, outCString, followUpTo);
!             PR_Free(outCString);
            }

            if (! replyTo.IsEmpty())
--- 851,857 ----
            {
              // Convert fields to UTF-8
              ConvertToUnicode(aCharset, outCString, followUpTo);
!             PR_FREEIF(outCString);
            }

            if (! replyTo.IsEmpty())

Comment 16

[rhp (gone)]

Ok, my changes are in for the crashing, but now we need to figure out why I'm
never getting OnDataAvailable() from the NTTP request. Over to Seth :-)

- rhp

Comment 17

[(not reading, please use [e-mail] instead)]

changig summary.  accepting.

Comment 18

[(not reading, please use [e-mail] instead)]

I'm also seeing no body when replying to local mail messages.

rhp, are you seeing that?

Comment 19

[lchiang]

Seth - I'm seeing this in today's release build (11/3) Win32.

Comment 20

[Karen Huang]

I have seen the same problem for today's build on Linux, but Ninoschka didn't
get the same problem as me. Cc: Ninoschka.

Comment 21

[Karen Huang]

And it also happened when replying to "mail" messages.
Is anybody seeing that on Linux platform?!

Comment 22

[rhp (gone)]

This seemed to work fine for me with pop and imap, just news where I didn't get
any quoted body.

- rhp

Comment 23

[(not reading, please use [e-mail] instead)]

fix in hand, awaying approval.

Comment 24

[(not reading, please use [e-mail] instead)]

the problem was when we ran the nntp url to get the message we weren't calling
OnDataAvailabe() on the channel listener. the reason is the channel listener
only got poked when ActionDisplayArticle was set as the action on the nntp url.
but this wouldn't be the case when we got run from the quoting backend. I added
a new action, and made sure to set it when running the url from the quoting back
end, and then made sure the nntp backend did the right thing with a url with
that action.

the next step is to talk to mscott, rhp, and jefft to see what changed to break
this.

another possible fix to consider is to check if we have a channel listener in
nsNNTPProtocol, and if so, poke it, unless the action is ActionSaveMessageToDisk

my guess is recently, the quoting / channel listener architecture changed, and
the IMAP protocol state machine handles it correctly, without looking at the
action.

opening a new bug on why quoting from the "Local Mail" folder is also blank.
it may be a similar problem with the nsMailboxProtocol state machine or it may
be a problem caused by alecf's uri changes.

Comment 25

[(not reading, please use [e-mail] instead)]

I've checked in a better fix.

now we just check if there is a channel listener, and if so, do the right
things.

we have a channel listener when displaying the article, or when quoting.

this is better because now the quoting back end doesn't need to know about nntp
urls.

Comment 26

[Karen Huang]

Passed by retesting on 11-04-08-M11 build / Linux platform.
But, it's still take too long for reading the news message....
mark as verified!