Failure on sending S/Mime messages should display the correct reason

VERIFIED FIXED

Status

MailNews Core
Security: S/MIME
VERIFIED FIXED
16 years ago
9 years ago

People

(Reporter: kaie, Assigned: kaie)

Tracking

Other Branch

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

16.25 KB, patch
David P. Drinan
: review+
Details | Diff | Splinter Review
16.21 KB, patch
Jean-Francois Ducarroz
: review+
jag (Peter Annema)
: superreview+
Details | Diff | Splinter Review
(Assignee)

Description

16 years ago
For examples, see bug 127364 or bug 121287.

The current code gives misleading information to the user when sending is not
possible. Currently, it just displays a general statement about "wrong email
settings".

It should be stating that there is a problem caused by certificates, if that's
the real reason.
(Assignee)

Comment 1

16 years ago
Assigning to me, nsbeta1+.
Assignee: ssaux → kaie
Keywords: nsbeta1+
(Assignee)

Comment 2

16 years ago
*** Bug 119540 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

16 years ago
I'd like to land this before the UI freeze.

Although I'm just defining two additional strings, I guess it's better to land
today or tomorrow.

Will attach a patch in a minute.
(Assignee)

Comment 4

16 years ago
Created attachment 76586 [details] [diff] [review]
Suggested fix

David, can you please review the code?

Sean, can you please review the two added strings? (search for "properties" in
the patch).

Thanks!
(Assignee)

Comment 5

16 years ago
David, in the code I'm changing, I'm removing comments which say
  XXX fix this later

The comment did not say what exactly should get fixed. Are those comments from
you and is the only thing that's missing, giving feedback to the user? Or is
there more to do?


A hint for reviewing: I'm removing a line that checks for "status < 0". I'm
removing it, because a couple of lines above, there is already the same line,
and the variable is not changed after and and before the removed check.

Comment 6

16 years ago
Comment on attachment 76586 [details] [diff] [review]
Suggested fix

r=ddrinan.
Attachment #76586 - Flags: review+
(Assignee)

Comment 7

16 years ago
Jag, can you please sr= ?

I would like to land this today or on Friday, because it is a UI relevant change.

Thanks!

Comment 8

16 years ago
Comment on attachment 76586 [details] [diff] [review]
Suggested fix

>Index: mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp,v
>retrieving revision 1.11
>diff -u -d -r1.11 nsMsgComposeSecure.cpp
>--- mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp	27 Mar 2002 03:27:43 -0000	1.11
>+++ mozilla/mailnews/extensions/smime/src/nsMsgComposeSecure.cpp	28 Mar 2002 17:47:25 -0000
...
>+  if (NS_SUCCEEDED(res) && errorString.Length())

!errorString.IsEmpty()

(Both places)

>@@ -572,17 +630,19 @@
>   PR_ASSERT(mSelfEncryptionCert);
>   PR_SetError(0,0);
>   mEncryptionCinfo = do_CreateInstance(NS_CMSMESSAGE_CONTRACTID, &rv);
>-  if (NS_FAILED(rv)) return 0;
>-  rv = mEncryptionCinfo->CreateEncrypted(mCerts); // XXX Fix this later XXX //
>+  if (NS_FAILED(rv)) return NS_ERROR_OUT_OF_MEMORY;

I would just return rv here instead of assuming we're out of memory. If you
suspect mEncryptionCinfo could be null while NS_SUCCEEDED(rv), add a ! check on
that and then return NS_ERROR_OUTOFMEMORY.

Also, you're effectively changing this case from NS_SUCCEEDED to NS_FAILED, are
all callers ready to deal with that?

(Both places)
(Assignee)

Comment 9

16 years ago
>>+  if (NS_SUCCEEDED(res) && errorString.Length())
> 
> !errorString.IsEmpty()
> 
> (Both places)

ok, changed.


>>  mEncryptionCinfo = do_CreateInstance(NS_CMSMESSAGE_CONTRACTID, &rv);
>>-  if (NS_FAILED(rv)) return 0;
>>+  if (NS_FAILED(rv)) return NS_ERROR_OUT_OF_MEMORY;
> 
> I would just return rv here instead of assuming we're out of memory. 

You're right, I changed that in both places.


> Also, you're effectively changing this case from NS_SUCCEEDED to NS_FAILED, are
> all callers ready to deal with that?

Yes, I think it should be that way.
Our only caller is in nsMsgSendPart, routed through nsMsgSend.
We arrive in this method only if there is actually encryption work to do.
If we are asked to encrypt, but we can't do our job, then we should return
failure, and cause the mail sending code to give up.
(Assignee)

Comment 10

16 years ago
Created attachment 76764 [details] [diff] [review]
Updated Patch

Comment 11

16 years ago
Comment on attachment 76764 [details] [diff] [review]
Updated Patch

a=asa (on behalf of drivers) for checkin to the 1.0 trunk
Attachment #76764 - Flags: approval+
(Assignee)

Updated

16 years ago
Keywords: adt1.0.0

Comment 12

16 years ago
adt1.00+ per ADT triage, pending L10N approval. Adding mcarlson for L10N
approval for strings. 
Keywords: adt1.0.0 → adt1.0.0+

Comment 13

16 years ago
l10n approved. thanks

Comment 14

16 years ago
Comments on the new strings:

+ErrorCanNotEncrypt=Unable to encrypt the message. Please check that you have a
valid email certificate for each recipient. Please check that your own
configured mail security certificates for this mail account are valid and trusted.

Change to: "Unable to encrypt message. Please check that you have a valid email
certificate for each recipient. Please check that the certificates specified in
Mail & Newsgroups Account Settings for this mail account are valid and trusted.

+ErrorCanNotSign=Unable to sign the message. Please check that your own
configured mail security certificates for this mail account are valid and trusted.

Change to: "Unable to sign message. Please check that the certificates specified
in Mail & Newsgroups Account Settings for this mail account are valid and trusted.

Reason in both cases is to give user a bit more of a clue re where to look, plus
consistency with usage elsewhere.

Updated

16 years ago
QA Contact: alam → carosendahl
Comment on attachment 76764 [details] [diff] [review]
Updated Patch

looks good to me, R=ducarroz
Attachment #76764 - Flags: review+

Comment 16

16 years ago
Comment on attachment 76764 [details] [diff] [review]
Updated Patch

sr=jag
Attachment #76764 - Flags: superreview+
(Assignee)

Comment 17

16 years ago
Checked in, using Sean's suggested text.
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED

Comment 18

16 years ago
Verified.
Status: RESOLVED → VERIFIED

Comment 19

16 years ago
Is this also in the branch yet?
(Assignee)

Comment 20

16 years ago
This landed on the trunk before the 1.0.0 branch was created, so the answer is yes.

Updated

13 years ago
Component: Security: S/MIME → Security: S/MIME
Product: PSM → Core

Updated

9 years ago
Component: Security: S/MIME → Security: S/MIME
Product: Core → MailNews Core
QA Contact: carosendahl → s.mime
You need to log in before you can comment on or make changes to this bug.