411646 - Crash [@ nsMsgCompose::Initialize] when initializing with null compose fields pointer in the params

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Mark Banner (:standard8)]

Regression from bug 216497.

I was looking at creating some unit tests for areas of nsMsgCompose.cpp and started by initializing an nsMsgCompose instance with basic values, with null where I didn't know what to put in. When I ran the test I found it crashed:

http://bonsai.mozilla.org/cvsblame.cgi?file=/mozilla/mailnews/compose/src/nsMsgCompose.cpp&rev=1.541&mark=807,815-823#806

The problem is composeFields is null and it hasn't been checked. I'm not sure how realistic this case is, but I think we should fix it anyway.

I have a patch and unit test that I'll attach later.

Comment 1

[Mark Banner (:standard8)]

Attachment: The fix (diff -w)

The fix, adds a simple check and a unit test. Also note that I'm planning on adding more tests to the test file, and that I'm expecting for complete nsMsgCompose coverage we'll want more than one test file.

Comment 2

[Mark Banner (:standard8)]

Attachment: The fix (normal patch)

Comment 3

[[e-mail]]

Comment on attachment 296390 [details] [diff] [review]
The fix (diff -w)

>+    var params = Components.classes[MsgComposeParamsContractID]
>+                           .createInstance(nsIMsgComposeParams);
>+
>+    do_check_true(params != null);
createInstance never returns null

>+    params.type = Components.interfaces.nsIMsgCompType.New;
>+    params.format = Components.interfaces.nsIMsgCompFormat.Default;
>+    params.originalMsgURI = "";
>+    params.identity = null;
>+    params.composeFields = null;
>+    params.bodyIsLink = false;
>+    params.sendListener = null;
>+    params.smtpPassword = "";
>+    params.origMsgHdr = null;
What does this achieve?

Comment 4

[Mark Banner (:standard8)]

(In reply to comment #3)
> (From update of attachment 296390 [details] [diff] [review])
> >+    params.type = Components.interfaces.nsIMsgCompType.New;
> >+    params.format = Components.interfaces.nsIMsgCompFormat.Default;
> >+    params.originalMsgURI = "";
> >+    params.identity = null;
> >+    params.composeFields = null;
> >+    params.bodyIsLink = false;
> >+    params.sendListener = null;
> >+    params.smtpPassword = "";
> >+    params.origMsgHdr = null;
> What does this achieve?

Opps, nothing really, its what I was playing with at the time, I forgot that it should initialise itself.

Comment 5

[Mark Banner (:standard8)]

Attachment: The fix v2 (diff -w)

Cuts out the redundant checks and setups in the unit test.

Comment 6

[Mark Banner (:standard8)]

Attachment: The fix v2 (normal patch)

Comment 7

[[e-mail]]

Comment on attachment 296527 [details] [diff] [review]
The fix v2 (diff -w)

>+
>+
Nit: do we normally have two blank lines in a row?

Comment 8

[Mark Banner (:standard8)]

Checked in with DIRS += test rather than TOOLS_DIRS += test as obviously we aren't doing tools dir processing in /mailnews. I'll check with Benjamin next week and file a different bug if necessary (which would cover both this compose addition and the address book one we already have).