uninitialized error string in mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp

RESOLVED FIXED in Thunderbird 40.0

Status

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

({regression})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird38+ fixed, thunderbird39 fixed, thunderbird40 fixed, seamonkey2.35 fixed, seamonkey2.36 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

4 years ago
+++ This bug was initially created as a clone of Bug #1149320 comment 2 +++

Found by a subtle compile warning:
mailnews/extensions/mdn/src/nsMsgMdnGenerator.cpp:1133:83: warning: 'exitString' may be used uninitialized in this function [-Wmaybe-uninitialized]
     bundle->FormatStringFromName(exitString, params, 1, getter_Copies(failed_msg));
Assignee

Comment 1

4 years ago
 neil@parkwaycc.co.uk 2015-04-01 18:45:09 CEST

Comment on attachment 8585742 [details] [diff] [review] [diff] [review]
patch

>     switch (aExitCode)
>     {
>       case NS_ERROR_UNKNOWN_HOST:
>       case NS_ERROR_UNKNOWN_PROXY_HOST:
>         exitString = MOZ_UTF16("smtpSendFailedUnknownServer");
>         break;
>       case NS_ERROR_CONNECTION_REFUSED:
>@@ -1099,16 +1099,19 @@ NS_IMETHODIMP nsMsgMdnGenerator::OnStopR
>         exitString = MOZ_UTF16("smtpPasswordUndefined");
>         break;
>       default:
>         if (aExitCode != NS_ERROR_ABORT && !NS_IS_MSG_ERROR(aExitCode))
>           exitString = MOZ_UTF16("smtpSendFailedUnknownReason");
>       break;
>     }
As the compiler points out, the string is not set for NS_ERROR_ABORT or any NS_IS_MSG_ERROR error. Do we already have strings for those errors? Is there a function somewhere we can call to get the string name of those errors?
Assignee

Comment 2

4 years ago
:aceman 2015-04-04 10:55:21 CEST

(In reply to neil@parkwaycc.co.uk from comment #6)
> (In reply to aceman from comment #5)
> Aha, I found it.
> 
> It's called errorStringNameForErrorCode.
> 
> Sounds familiar to anyone?

Yeah, you seem to be right. Before bug 952493 the aExitCode was preserved in the "default" case and passed on to FormatStringFromID. After the bug, the exitString is left empty if the aExitCode is ABORT or one of the MSG errors. That is a regression.
Component: Backend → Composition
Keywords: regression
Assignee

Comment 3

4 years ago
Posted patch patch (obsolete) — Splinter Review
xpcshell tests passed for me locally. mozmill does not run as a whole so I can't say.
Attachment #8588288 - Flags: review?(rkent)
Attachment #8588288 - Flags: review?(neil)
Comment on attachment 8588288 [details] [diff] [review]
patch

>       case NS_ERROR_SMTP_PASSWORD_UNDEFINED:
>         exitString = MOZ_UTF16("smtpPasswordUndefined");
>         break;
>       default:
>-        if (aExitCode != NS_ERROR_ABORT && !NS_IS_MSG_ERROR(aExitCode))
>+        if (NS_IS_MSG_ERROR(aExitCode))
>+          exitString = errorStringNameForErrorCode(aExitCode);
>+        else
>           exitString = MOZ_UTF16("smtpSendFailedUnknownReason");
>       break;
Interestingly errorStringNameForErrorCode already defaults to smtpSendFailedUnknownReason, so you don't actually need to special-case it. It also knows about NS_ERROR_SMTP_PASSWORD_UNDEFINED so you can fold that case in too. (I don't have any better ideas for NS_ERROR_ABORT though.)
Assignee

Comment 5

4 years ago
Posted patch patch v2 (obsolete) — Splinter Review
Yes, good ideas.
Attachment #8588288 - Attachment is obsolete: true
Attachment #8588288 - Flags: review?(rkent)
Attachment #8588288 - Flags: review?(neil)
Attachment #8588370 - Flags: review?(neil)
Assignee

Updated

4 years ago
No longer depends on: 1149320
Comment on attachment 8588370 [details] [diff] [review]
patch v2

>       default:
>+        exitString = errorStringNameForErrorCode(aExitCode);
>-        if (aExitCode != NS_ERROR_ABORT && !NS_IS_MSG_ERROR(aExitCode))
>-          exitString = MOZ_UTF16("smtpSendFailedUnknownReason");
>-      break;
>     }
Please restore the break;
Attachment #8588370 - Flags: review?(neil) → review+
Assignee

Comment 7

4 years ago
What is the "break" good for in the default case?
Assignee

Comment 8

4 years ago
Posted patch patch v2.1 (obsolete) — Splinter Review
Attachment #8588370 - Attachment is obsolete: true
Attachment #8589889 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
This patch has bitrotted, please update it (and set again the 'checkin-needed' keyword).
Flags: needinfo?(acelists)
Keywords: checkin-needed
Assignee

Comment 10

4 years ago
Posted patch patch v2.2Splinter Review
Thanks, updated. Clash with Suyash's string fixes :) This is also an error string fix needed for TB38.
Attachment #8589889 - Attachment is obsolete: true
Flags: needinfo?(acelists)
Attachment #8592402 - Flags: review+
Assignee

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/d41f334331bf
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
Attachment #8592402 - Flags: approval-comm-beta?
Attachment #8592402 - Flags: approval-comm-aurora?
Please set the target milestone to match the first revision where the patch landed, in this case Thunderbird 40. The affected flags are for other revisions. Yes it is badly misnamed.
Target Milestone: Thunderbird 38.0 → Thunderbird 40.0
You need to log in before you can comment on or make changes to this bug.