Closed Bug 411646 Opened 15 years ago Closed 15 years ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: standard8, Assigned: standard8)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files, 2 obsolete files)

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.
Attached patch The fix (diff -w) (obsolete) — Splinter Review
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)
Attached patch The fix (normal patch) (obsolete) — Splinter Review
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.
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)
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
Closed: 15 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.