865972 - Incorrect use of error code generation macro.

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error code.

In once place, an error code generation macro, NS_ERROR_GENERATE_FAILURE(), is 
applied to a define error code which has been generated using the said macro.
Only one application is necessary and enough.  

See the comment now closed (duped) bug 859197 :

I am quoting an excerpt from it below.

PS: As I checked the error values, I found this nugget that needs fixing, too.
But that will be another bugzilla entry.

https://mxr.mozilla.org/comm-central/source/mailnews/compose/src/nsMsgSend.cpp#2621


2620           mSendReport->SetError(nsIMsgSendReport::process_Current,
2621               // XXX The following applies NS_ERROR_GENERATE_FAILURE twice,
2622               // which doesn't make sense.  Just NS_MSG_ERROR_ATTACHING_FILE is
2623               // surely what was intended.
2624               NS_ERROR_GENERATE_FAILURE(
2625                 NS_ERROR_MODULE_MAILNEWS,
2626                 static_cast<uint32_t>(NS_MSG_ERROR_ATTACHING_FILE)),
2627               false);


Fix is attached.

Comment 1

[ISHIKAWA, Chiaki]

Bug 783526 - Move all c-c error codes to nsError.h
is where the error code definitions (in which file) is discussed now BTW.

Comment 2

[Aryeh Gregor (:ayg) (no longer with Mozilla)]

Comment on attachment 742175 [details] [diff] [review]
Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error code.

Review of attachment 742175 [details] [diff] [review]:
-----------------------------------------------------------------

This needs review by a module owner/peer, because it changes the meaning of the existing code.  I.e., after this patch, the error that's being returned will be different than it was.  This was probably the original intent, but since this is in fact a behavior change, you should get actual review to confirm that this is safe.  (This is why I didn't change the behavior to start with.)

I confirm that this code is what was probably intended, though.

Comment 3

[ISHIKAWA, Chiaki]

(In reply to :Aryeh Gregor from comment #2)
> Comment on attachment 742175 [details] [diff] [review]
> Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error
> code.
> 
> Review of attachment 742175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
[snip snip snip]
> 
> I confirm that this code is what was probably intended, though.

Thank you for your comment. OK, let me find out who the module owner is and ask for the review. 

Hmm, from looking at

https://wiki.mozilla.org/Modules/SeaMonkey (mailnews subdirectory)
https://wiki.mozilla.org/Modules/Thunderbird (mail subdirectory)
https://wiki.mozilla.org/Modules/MailNews_Core (mail news core)

I put [e-mail] in the review field.


Considering that TB is now a community-driven support mode, I think we should make it easier for third party developers to figure out who the module owner is for a particular piece code is. But I digress.

Comment 4

[David :Bienvenu]

Comment on attachment 742175 [details] [diff] [review]
Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error code.

this looks OK to me, as long as the xpcshell and mozmill tests still pass. Thx for the patch.

Comment 5

[ISHIKAWA, Chiaki]

Hi,

I have submitted a tryserver run to check the patch.

https://tbpl.mozilla.org/?tree=Thunderbird-Try&rev=f7bba1427ec4

Although, it has a few patches of my own, and a few more (from others) which I think fixed the TB tryserver bustage,
I can say that the  patch above

https://bugzilla.mozilla.org/attachment.cgi?id=742175

does not break anything [It did not introduce any more
bugs than known "permanent orange" errors.]

BTW, the patch above is included for the job submission as
"ISHIKAWA, Chiaki – Originally noticed in bug 851917". The bug number
is incorrect. It should refer to 859197.

(M-C patch 893362-fix is for aligning the module names during
build.)

Unfortunately, linux (32 bits and 64 bits) have some configuration issue
on the tryserver and xpcshell test can't run under linux on TryServer.
XP and Win7 version showed that xpcshell ran successfully.
(Also, on my local PC, xpcshell test produced less errors (19) with the patches than untouched source files (28 error): there seem to be a few more permanent orange type errors in the linux version of xpcshell test suite output.)

If the output of the tryserver looks OK, I will put chekin-needed keyword.

Too bad, xpshell test can't be run for linux right now on the TB tryserver.)

TIA

Comment 6

[:aceman]

Comment on attachment 742175 [details] [diff] [review]
Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error code.

Chiaki, can you go on with this patch? Does it need another run at the try server?
I CC Irving as he is the mentor at bug 783526 and David Bienvenu will probably not respond anymore.

Comment 7

[:aceman]

Comment on attachment 742175 [details] [diff] [review]
Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error code.

Review of attachment 742175 [details] [diff] [review]:
-----------------------------------------------------------------

>Originally noticed in bug 851917.
Please update this into a proper commit message.

Comment 8

[ISHIKAWA, Chiaki]

Attachment: (Modified commit message, and fixed bit rot) Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error code. Carrying over mozilla=review+

(I may have mixed up something and a similar post may happen twice, sorry if this is the case.)

(In reply to :aceman from comment #6)
> Comment on attachment 742175 [details] [diff] [review]
> Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error
> code.
> 
> Chiaki, can you go on with this patch? Does it need another run at the try
> server?
> I CC Irving as he is the mentor at bug 783526 and David Bienvenu will
> probably not respond anymore.

I don't think we need another try run. I tested the patch again locally
and it did not produce any new errors other than the ones that I have been seeing in the code. (tested using |make mozill| and |make xpcshell-tests|).
[this means that the test may not cover this particular code path, or
that the error processing code doesn't care much about this particular code :-( ]

(In reply to :aceman from comment #7)
> Comment on attachment 742175 [details] [diff] [review]
> Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error
> code.
> 
> Review of attachment 742175 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> >Originally noticed in bug 851917.
> Please update this into a proper commit message.

Yes, I did and also fixed a bit rot issue: it did not apply to the latest code from comm-central (refreshed a couple of days ago), and I fixed that also.

Comment 9

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 8397605 [details] [diff] [review]
(Modified commit message, and fixed bit rot)   Fixing the incorrect double usage of NS_ERROR_GENERATE_FAILURE to an error code.  Carrying over mozilla=review+

Review of attachment 8397605 [details] [diff] [review]:
-----------------------------------------------------------------

This was introduced in http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&root=/cvsroot&subdir=mozilla/mailnews/compose/src&command=DIFF_FRAMESET&file=nsMsgSend.cpp&rev2=1.402&rev1=1.401; as far as I can tell, the extra NS_ERROR_GENERATE_FAILURE is a no-op; it sets all the high order bits in the error code that were already set, so there's no change to the code that actually executes.

Comment 10

[ISHIKAWA, Chiaki]

Dear Irving, 

Thank you for the analysis.

Ok, so the analysis suggests that the TB/FF software handled the result of the
strange-looking code without problem.
(I mean the test harness and the error handler probably encountered the 
error path, and in that case, the error code passed was an ordinary error code, despite the
obviously misleading code, and so the software as a whole is OK. That seems to be the case.)

But, I think we really should fix the outlook of the code as in
the proposed patch.

So anyone opposing the checking in this patch, please speak up.
I will probably put the checkin-needed
keyword on coming Monday or so.

TIA

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f54b4a260506