Last Comment Bug 783289 - Fix return code issues revealed by nsresult changes in comm-central
: Fix return code issues revealed by nsresult changes in comm-central
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- normal (vote)
: Thunderbird 18.0
Assigned To: :Irving Reid (No longer working on Firefox)
:
:
Mentors:
Depends on: 779130
Blocks: 287969
  Show dependency treegraph
 
Reported: 2012-08-16 08:34 PDT by :Irving Reid (No longer working on Firefox)
Modified: 2012-11-29 02:13 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Return meaningful values in places touched by bug 779130 p1, p3, p4, p6, p8, p9 (12.54 KB, patch)
2012-08-16 08:45 PDT, :Irving Reid (No longer working on Firefox)
Pidgeot18: review+
standard8: review+
Details | Diff | Splinter Review
Return meaningful values in places touched by bug 779130 p1, p3, p4, p6, p8, p9 - cleaned up bitrot (12.60 KB, patch)
2012-09-28 10:36 PDT, :Irving Reid (No longer working on Firefox)
irving: review+
Details | Diff | Splinter Review

Description :Irving Reid (No longer working on Firefox) 2012-08-16 08:34:20 PDT
+++ This bug was initially created as a clone of Bug #779130 +++

Bug 779130 cleaned up a whole bunch of places in C-C where nsresult values were mixed with integer types (PRInt32 and boolean, mostly)

The patches in that bug were careful not to make any semantic changes to the code, but there were many places where the existing code was almost certainly incorrect - for example, functions that returned 'false' to indicate failure, which casts to integer 0, which equals nsresult NS_OK.
Comment 1 :Irving Reid (No longer working on Firefox) 2012-08-16 08:45:59 PDT
Created attachment 652464 [details] [diff] [review]
Return meaningful values in places touched by bug 779130 p1, p3, p4, p6, p8, p9

This doesn't cover most of the cases where existing integer values are static_cast<nsresult>; a separate patch should be built for that.
Comment 2 Joshua Cranmer [:jcranmer] 2012-08-28 10:49:44 PDT
Comment on attachment 652464 [details] [diff] [review]
Return meaningful values in places touched by bug 779130 p1, p3, p4, p6, p8, p9

r+ on the mime and news portion of this patch; I haven't looked at the other parts.

One thing that I'm concerned about is the replacement of NS_ENSURE_SUCCESS(rv, NS_OK) with (rv, rv), as it means that some of our code could become less tolerant of faults. Given that the hostinfo.dat file in particular is not necessary for the NNTP code, it should be possible to handle not being able to read it. The fact that we check for existence immediately beforehand is a 98% solution case, but we still have a few lines in between of potential raciness, and it doesn't account for odd things like "we don't have permissions to read this" (which is probably indicative of more issues, though).
Comment 3 :Irving Reid (No longer working on Firefox) 2012-09-28 10:36:07 PDT
Created attachment 665970 [details] [diff] [review]
Return meaningful values in places touched by bug 779130 p1, p3, p4, p6, p8, p9 - cleaned up bitrot

Cleaned up bitrot due to NSPR Types and nsCAutoString changes; carrying forward r=Standard8,jcranmer
Comment 4 :Irving Reid (No longer working on Firefox) 2012-09-28 10:56:53 PDT
https://hg.mozilla.org/comm-central/rev/4b6d99850c78

Note You need to log in before you can comment on or make changes to this bug.