609210 - Compiler warning: Use of NS_ENSURE_TRUE and NS_ENSURE_SUCCESS WITHOUT the second argument to it.

Status: RESOLVED DUPLICATE of bug 788242

(Core :: XPCOM, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: Pathes for ./mozilla subdirectory of comm-central

From the compilation of TB3 using comm-central,
I found one disturbing set of warnings
generated by the use of two-argument macros NS_ENSURE_TRUE and NS_ENSURE_SUCCESS
WITHOUT the second argument, i.e., empty string to them.
GCC generated the warning that the empty second argument to the macro
will cause undefined behavior in ISO C preprocessor or to some such effect.
(The exact wording in the case of NS_ENSURE_SUCCESS is like this:
warning: invoking macro NS_ENSURE_SUCCESS argument 2: empty macro
arguments are undefined in ISO C90 and ISO C++98)

I looked at such usages and found the following.

NS_ENSURE_TRUE and NS_ENSURE_SUCCESS 
returns an error value (the second argument), if the
expression in the first argument position signifies the error value.

However, there are cases where NS_ENSURE_TRUE and NS_ENSURE_SUCCESS 
is being used in a void function. Obviously trying to return a value in a void function from NS_ENSURE_TRUE and NS_ENSURE_SUCCESS would cause 
a compilation error.

So some programmers resorted to leaving the second argument as empty.
In the existing CPP, this resulted in a simple "return ;" in the macro 
expansion, and the compilation didn't fail.
This was certainly clever in the short term.

But this is a very bad practice in my opinion.
We should use a proper one-argument macro that does NOT return
a value for such such situation to reduce the
disturbing warnings. These extra warnings make it difficult
to locate warnings that have REAL PROBLEMS behind them.
Right now, TB3 compilation using comm-central produces tons of warnings :-(

The attached is the fix the problem.
It defines two macros: NS_ENSURE_TRUE_VOID, and NS_ENSURE_SUCCESS_VOID  that take only a single argument and do not return a value.
The patch replaces
the bad usages of NS_ENSURE_TRUE and NS_ENSURE_SUCCESS with empty second argument by the use of these NS_ENSURE_TRUE_VOID and NS_ENSURE_SUCCESS_VOID  to eliminate the disturbing warning messages.

Patches for files only for TB3 in comm-central, and
patches for ./mozilla subdirectory

[1] patches for files under comm-central top directory, i.e., for TB3.

[2] Patches for files under ./mozilla subdirectory.

A few fixes for uninitialized variable and ununsed variable crept in.
I am not sure what the correct initial value should be 
for the next one. Maybe NS_FALSE?

--- a/mailnews/imap/src/nsImapMailFolder.cpp	Tue Nov 02 10:34:06 2010 +0000
    ... the following initial value may have to be PR_FALSE?
+  nsresult rv = 0;              // initial value that is returned sometimes!

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Patches for the TB3 files under comm-central.

This is the patch for the TB3 files under comm-central.

Comment 2

[ISHIKAWA, Chiaki]

Attachment: fixes for ./mozilla subdirectory files of comm-central (actually mozilla-central, too)

After fixing other bugs and rebuild TB3, I found the
same problems are found in the following files under ./mozilla subdirectory.
So I upload the more comprehensive patches.

diff -r b192fa5cfffc accessible/src/base/nsEventShell.cpp
diff -r b192fa5cfffc accessible/src/html/nsHTMLFormControlAccessible.cpp
diff -r b192fa5cfffc accessible/src/xul/nsXULFormControlAccessible.cpp
diff -r b192fa5cfffc content/html/content/src/nsTextEditorState.cpp
diff -r b192fa5cfffc content/html/document/src/nsHTMLDocument.cpp
diff -r b192fa5cfffc dom/indexedDB/IDBTransaction.cpp
diff -r b192fa5cfffc dom/src/threads/nsDOMThreadService.cpp
diff -r b192fa5cfffc editor/libeditor/html/nsHTMLEditor.cpp
diff -r b192fa5cfffc layout/base/nsCaret.cpp
diff -r b192fa5cfffc layout/generic/nsObjectFrame.cpp
diff -r b192fa5cfffc layout/style/nsHTMLStyleSheet.cpp
diff -r b192fa5cfffc xpcom/glue/nsDebug.h

Comment 3

[ISHIKAWA, Chiaki]

Attachment: fix for TB3 files in comm-central.

After fixing other problems, and rebuilding TB3 using comm-central,
I now realize the same problems are found in the following files and
so I am uploading the complete patch for TB3 files in comm-central.

diff -r 55086537a691 mailnews/addrbook/src/nsAbAddressCollector.cpp
diff -r 55086537a691 mailnews/base/src/nsMsgContentPolicy.cpp
diff -r 55086537a691 mailnews/base/src/nsMsgMailSession.cpp
diff -r 55086537a691 mailnews/extensions/bayesian-spam-filter/src/nsBayesianFilter.cpp
diff -r 55086537a691 mailnews/imap/src/nsAutoSyncManager.cpp
diff -r 55086537a691 mailnews/imap/src/nsImapMailFolder.cpp
diff -r 55086537a691 mailnews/local/src/nsLocalMailFolder.cpp


I wonder if I should separate the TB3 patch and ./mozilla patch into
diffrent bugzilla entries.

In any case, the problem appears simply in too many files, but the
files list is now complete.

Comment 4

[Atul Aggarwal]

Are we getting this warning in current tip of m-c as well? I didn't see any such warning in my build logs of firefox (using gcc 4.6.1 on linux).

Comment 5

[ISHIKAWA, Chiaki]

Given that there are still the usage of NS_ENSURE_TRUE macro without any second arguments, I suspect that the
compiler warning options have been changed?

Searching such occurrences  using mxr.mozilla.org in common-central code which should have mozilla-central code under it,
I still see some such occurrences:
e.g.,

mozilla/content/base/src/nsEventSource.cpp (View Hg log or Hg annotations)

    line 881 -- NS_ENSURE_TRUE(mHttpChannel, NS_ERROR_NO_INTERFACE);
    line 1179 -- NS_ENSURE_TRUE(srcToTest, PR_FALSE);
    line 1315 -- NS_ENSURE_TRUE(mMessagesToDispatch.GetSize() == sizeBefore + 1,
    line 1348 -- NS_ENSURE_TRUE(sgo,);
    line 1351 -- NS_ENSURE_TRUE(scriptContext,);
    line 1354 -- NS_ENSURE_TRUE(cx,);
    line 1368 -- NS_ENSURE_TRUE(jsString,); 

So if the compilation doesn't raise any warnings to these, then
it is likely that the warning setting of the compiler may have
been changed.

I also found an interesting code:
mozilla/security/manager/ssl/src/nsCertTree.cpp (View Hg log or Hg annotations)

    line 1485 -- NS_ENSURE_TRUE( (cert!=0 && entry!=0), RETURN_NOTHING );
    line 1540 -- NS_ENSURE_TRUE( (a!=0 && ace!=0 && b!=0 && bce!=0), 0 ); 


Here RETURN_NOTHING is defined by pre-processor to nothing.
At least the coder was trying to be honest about it, but
this should also be fixed IMHO.

BTW, maybe 4.6.1 doesn't warn on this problematic construct by default?

Comment 6

[:aceman]

Hi, I am trying to fix this for mailnews in bug 716278.

Did you come to any good method to fix these warnings?

Is the RETURN_NOTHING solution working?

I can confirm the warnings were there with gcc 4.5 just recently it the time of filing bug 716278, but I also can't see them today.
What happened?

Comment 7

[ISHIKAWA, Chiaki]

For a possible solution, 
please see the introduction of a new macros in
xpcom/glue/nsDebug.h
in my first attachment above: https://bugzilla.mozilla.org/attachment.cgi?id=488481

I introduced a macro WITHOUT the second argument when the
function in question has void return type.

The rest is to replace the offending code left and right, which
may not be liked much by  module owners, but I think
we should strive for an honest code that sticks to defined
behavior of preprocessor and macro.

I am not sure what happened to the GCC compiler now. Maybe
the warning message has been changed? (I am using GCC 4.6.1 now, I think.)

Use of RETURN_NOTHING is not recommended, and if we
add RETURN_NOTHING to everywhere, we might as well
rewrite the offending code using the newly introduced macros everywhere as well.

Just my idea.

Comment 8

[:aceman]

I would gladly use the new 1 argument macros if you can push them through the module owners.

Have you tried reviews recently?

(I don't know what is with the warnings, I did not change the compiler (4.5.3) and they do not show anymore.)

Comment 9

[ISHIKAWA, Chiaki]

(In reply to :aceman from comment #8)
> I would gladly use the new 1 argument macros if you can push them through
> the module owners.
> 
> Have you tried reviews recently?
> 
> (I don't know what is with the warnings, I did not change the compiler
> (4.5.3) and they do not show anymore.)

I have not tried review yet since the problem seems to be
found in many files scattered all over the directories.
(But maybe it is the xpcom/glue/nsDebug.h, i.e., some type of
debug support module or whatever?)

Any idea who to put in the reivewer's field on this matter?
(Or can I simply set a request for a review and without specifying who?)

Comment 10

[:aceman]

I think the main problem is to get the macro definition into xpcom/glue/nsDebug.h . I think one of Benjamin Smedberg, Doug Turner, Mike Shaver would be good for reviewing that reduced patch. (It is not good to kae a review request without specifying a name.) Because you didn't get a comment yet from any mozilla developer with any say on this, if this change is even acceptable.

Only once that is done we should try to find all the usages of the current macro (where the the second argument is missing) and make some patches, potentially splitting them per mozilla component. It is not sure those 3 people would like to review those patches for the various scattered files.

Comment 11

[Benjamin Smedberg]

As I noted on the newsgroups, I am opposed to NS_ENSURE_* in general, and so I oppose this change. I would rather you spend time on implementing

if (NS_WARN_IF_FAILED(rv))
  return;

Comment 12

[:aceman]

What is the problem with NS_ENSURE_* please?

Comment 13

[Benjamin Smedberg]

It hides control flow (return statements).