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

RESOLVED FIXED

Status

RESOLVED FIXED
11 years ago
8 years ago

People

(Reporter: standard8, Assigned: standard8)

Tracking

({crash, regression})

Trunk
crash, regression
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments, 2 obsolete attachments)

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.
Created attachment 296390 [details] [diff] [review]
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.
Attachment #296390 - Flags: superreview?(neil)
Attachment #296390 - Flags: review?(neil)
Created attachment 296391 [details] [diff] [review]
The fix (normal patch)

Comment 3

11 years ago
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?
(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.
Created attachment 296527 [details] [diff] [review]
The fix v2 (diff -w)

Cuts out the redundant checks and setups in the unit test.
Attachment #296390 - Attachment is obsolete: true
Attachment #296391 - Attachment is obsolete: true
Attachment #296527 - Flags: superreview?(neil)
Attachment #296527 - Flags: review?(neil)
Attachment #296390 - Flags: superreview?(neil)
Attachment #296390 - Flags: review?(neil)
Created attachment 296528 [details] [diff] [review]
The fix v2 (normal patch)

Comment 7

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

>+
>+
Nit: do we normally have two blank lines in a row?
Attachment #296527 - Flags: superreview?(neil)
Attachment #296527 - Flags: superreview+
Attachment #296527 - Flags: review?(neil)
Attachment #296527 - Flags: review+
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).
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Blocks: 216479
No longer blocks: 216497
Flags: in-testsuite+
Product: Core → MailNews Core
Crash Signature: [@ nsMsgCompose::Initialize]
You need to log in before you can comment on or make changes to this bug.