392055 - NS_ENSURE_SUCCESS should print out the error code

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Shawn Wilsher :sdwilsh]

As per discussion on irc.

Comment 1

[Shawn Wilsher :sdwilsh]

Attachment: v1.0

Comment 2

[Shawn Wilsher :sdwilsh]

Comment on attachment 276503 [details] [diff] [review]
v1.0

oops - s/%X/%#X/g

Comment 3

[Shawn Wilsher :sdwilsh]

oh fun, NSPR's doesn't support the %#X, so we'll have to manually add it.

Comment 4

[Benjamin Smedberg]

Comment on attachment 276503 [details] [diff] [review]
v1.0

1) You need to do this only #ifdef DEBUG...

2) I don't like hardcoding a random dyndns.org address into our codebase... can you file a mozilla IT bug to get the nserror script hosted somewhere in the mozilla.org domain, or else leave out the URL?)

Comment 5

[Shawn Wilsher :sdwilsh]

Attachment: v1.1

* No more http link (revist in a followup?)
* Just does |if (NS_FAILED(res)) return ret;| in non-debug builds.
* uses |const char[]| instead of |const char *|

Comment 6

[Shawn Wilsher :sdwilsh]

Attachment: v1.2

This one actually compiles.  Sorry about that...

Comment 7

[Shawn Wilsher :sdwilsh]

bz isn't around for a while - any suggestions for an sr?

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 277001 [details] [diff] [review]
v1.2

>Index: xpcom/glue/nsDebug.h
>+  nsresult __rv = res;                                                    

Add a comment before the macro def that says that you're doing this to avoid evaluating |res| more than once, ok?

>+    char *msg = PR_smprintf(format, __rv, __rv);                          \

Only need one __rv there; the format only has one placeholder.

With those two nits picked, sr+a=bzbarsky

Comment 9

[Shawn Wilsher :sdwilsh]

Attachment: v1.3

for checkin

Comment 10

[Shawn Wilsher :sdwilsh]

Checking in xpcom/glue/nsDebug.h;
new revision: 1.27; previous revision: 1.26

Comment 11

[Shawn Wilsher :sdwilsh]

Bustage fix on linux

Checking in embedding/browser/gtk/tests/Makefile.in;
new revision: 1.38; previous revision: 1.37

Comment 12

[Jesse Ruderman]

Won't this blow up terribly if the expression |res| happens to contain a % symbol?

Comment 13

[Shawn Wilsher :sdwilsh]

In theory, yeah, but I'm not sure that would ever compile.  res and ret should be nsresults for things to compile nicely.

Comment 14

[Jesse Ruderman]

There are examples of function-call expressions inside NS_ENSURE_SUCCESS in our tree...

Comment 15

[:Gavin Sharp [email: [e-mail]]]

Backed this out at Shawn's request because of bustage.

Comment 16

[Shawn Wilsher :sdwilsh]

Really not sure what is up with this - I figured my fix would have solved the issue :/

/tools/gcc-4.1.1/bin/g++   -fno-rtti -fno-exceptions -Wall -Wconversion -Wpointer-arith -Wcast-align -Woverloaded-virtual -Wsynth -Wno-ctor-dtor-privacy -Wno-non-virtual-dtor -Wno-long-long -pedantic -fshort-wchar -pthread -pipe  -DDEBUG -D_DEBUG -DDEBUG_cltbld -DTRACING -g -fno-inline -I/usr/include/gtk-2.0 -I/usr/lib/gtk-2.0/include -I/usr/include/atk-1.0 -I/usr/include/cairo -I/usr/include/pango-1.0 -I/usr/include/glib-2.0 -I/usr/lib/glib-2.0/include -I/usr/include/freetype2 -I/usr/include/libpng12   -o TestGtkEmbed TestGtkEmbed.o  -lpthread          -L../../../../dist/bin -L../../../../dist/lib  -lX11   ../../../../dist/lib/libxpcomglue.a -ldl -lm  -L/lib -lgtk-x11-2.0 -latk-1.0 -lgdk-x11-2.0 -lgdk_pixbuf-2.0 -lm -lpangocairo-1.0 -lpango-1.0 -lcairo -lgmodule-2.0 -ldl -lgobject-2.0 -lglib-2.0       
../../../../dist/lib/libxpcomglue.a(nsINIParser.o): In function `nsINIParser::Init(nsILocalFile*)':
/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/xpcom/glue/standalone/nsINIParser.cpp:89: undefined reference to `PR_smprintf'
/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/xpcom/glue/standalone/nsINIParser.cpp:89: undefined reference to `PR_smprintf_free'
collect2: ld returned 1 exit status
gmake[5]: *** [TestGtkEmbed] Error 1
gmake[5]: Leaving directory `/builds/tinderbox/Fx-Trunk-Memtest/Linux_2.6.18-8.el5_Depend/mozilla/obj/embedding/browser/gtk/tests'

Comment 17

[Shawn Wilsher :sdwilsh]

Attachment: v1.4

OK, I *think* this works...just a bit difficult to test since I don't have linux...

We could also change those NS_ENSURE_SUCCESS's to something else, or doing a #define to disable the debug version in that file...

Comment 18

[jag (Peter Annema)]

If the idea of libxpcomglue is that it doesn't depend on NSPR, then it'd be good if it could #define something to turn this enhanced behaviour off. Or, maybe there's already something everyone else defines that xpcomglue doesn't that you could check for.

Comment 19

[Boris Zbarsky [:bzbarsky]]

Hmm.  Jesse's question about '%'. Worries me.  Can we address that somehow?

Comment 20

[Shawn Wilsher :sdwilsh]

Nothing really comes to mind as a quick way to deal with it :/

Comment 21

[Shawn Wilsher :sdwilsh]

Of course right after I say that I came up with something

Not sure if it'd work, but we could rephrase the expression to something like "Ox#### was returned (NS_ENSURE_SUCCESS(#res, #ret))"

Clearly, that string sucks, but it demonstrates the point.

Comment 22

[Benjamin Smedberg]

Comment on attachment 282110 [details] [diff] [review]
v1.4

Yeah, we need to deal better with the standalone glue case. When linking with the standalone glue you don't want to link with NSPR (because the whole point is that you don't know where mozilla/NSPR are yet). So the NSPR gunk here should be disabled if XPCOM_GLUE_AVOID_NSPR is defined (which comes from nsISupportsImpl.h currently).

Comment 23

[Shawn Wilsher :sdwilsh]

Suggestions on what to do then?

Comment 24

[jag (Peter Annema)]

Attachment: Address % in res, add XPCOM_GLUE_AVOID_NSPR check

So if XPCOM_GLUE_AVOID_NSPR we'll use the simpler NS_WARNING, and if !DEBUG the NS_WARNING evaluates do an empty do/while.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 282359 [details] [diff] [review]
Address % in res, add XPCOM_GLUE_AVOID_NSPR check

sr=bzbarsky

Comment 26

[jag (Peter Annema)]

Checking in nsDebug.h;
/cvsroot/mozilla/xpcom/glue/nsDebug.h,v  <--  nsDebug.h
new revision: 1.29; previous revision: 1.28
done

Comment 27

[jag (Peter Annema)]

Attachment: Move XPCOM_GLUE_AVOID_NSPR define into nscore.h

Comment 28

[jag (Peter Annema)]

Checking in glue/nsISupportsImpl.h;
/cvsroot/mozilla/xpcom/glue/nsISupportsImpl.h,v  <--  nsISupportsImpl.h
new revision: 3.52; previous revision: 3.51
done
Checking in base/nscore.h;
/cvsroot/mozilla/xpcom/base/nscore.h,v  <--  nscore.h
new revision: 1.103; previous revision: 1.102
done

Comment 29

[[e-mail]]

Comment on attachment 282359 [details] [diff] [review]
Address % in res, add XPCOM_GLUE_AVOID_NSPR check

>+#define NS_ENSURE_SUCCESS_BODY(res, ret)                                  \
>+    char *msg = PR_smprintf("NS_ENSURE_SUCCESS(%s, %s) failed with "      \
>+                            "result 0x%X", #res, #ret, __rv);             \
So, this references __rv, instead of passing it as a macro argument...