208098 - XPCOM_DEBUG_BREAK=stack no longer works

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

On linux, setting XPCOM_DEBUG_BREAK to "stack" no longer seems to work... there
is an #ifndef XPCOM_GLUE at
http://lxr.mozilla.org/seamonkey/source/xpcom/glue/nsDebug.cpp#317 but it
doesn't seem to be honored. I put an debugging assertion in nsHashtable.cpp and
it says "unrecognized value of XPCOM_DEBUG_BREAK env var!"

Comment 1

[Doug Turner (:dougt)]

in xpfe/bootstrap/Makefile.in, if you set GRE_BUILD to be zero and rebuild, do
you still see this problem?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I don't, which is why I've had that change in my xpfe/bootstrap/Makefile.in
since it landed.  (And I've mentioned this to dougt before.)

Comment 3

[Benjamin Smedberg]

Well, it looks to me that this can work even when BUILD_GRE...
nsStackFrameUnix.o is statically linked into the mozilla binary, so it's not a
question of linking libxpcom.so...

Perhaps another #if !defined(XPCOM_GLUE) || defined(MOZ_BOOTSTRAP) would work?

Comment 4

[Doug Turner (:dougt)]

nsDebug.cpp gets built into the glue library and into the xpcom lib.  There are
only two copies.  Adding this extra define would require another pass to build a
gluelib that supports this.

Comment 5

[Doug Turner (:dougt)]

Attachment: patch v.1

this patch looks scarier than it is.  

The basic idea is to create a nsIDebug interface that the glue code can use to
implement a nsDebug object.  This is very similar to what we did with nsMemory.
 

The bulk of the change was moving nsDebug.[c|h] from xpcom/glue to xpcom/base. 
Then i removed the cruft that wasn't being used.  it turned out that all of the
methods declared in nsDebug were either unused or could be boiled down to three
core.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I'm worried that this might not work on all platforms when using the glue
library.  It creates two different instances of nsDebug::Assertion, etc., one of
which calls the other.  With the dynamic linker doing what it sometimes does on
Linux, can't this lead to an infinite loop?  (It's also confusing.)

It seems cleaner, anyway, to do what nsMemory does -- which would mean to move
the guts of nsDebug directly into nsDebugImpl.cpp in xpcom/base (rather than
have that be a thin wrapper around nsDebug) and then have what you put in
glue/standalone/ in glue/ directly.  (Roughly, anyway.)  We really don't need to
worry about the performance of these functions since they're called quite rarely
(only when assertions fail).  (With nsTraceRefcnt that could be a little more of
an issue, although it's probably not worth worrying about.)

Comment 7

[Doug Turner (:dougt)]

Attachment: patch v.2

with dbaron's suggestion.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 127021 [details] [diff] [review]
patch v.2

>Index: glue/standalone/Makefile.in

No need for any changes to this file anymore.

>Index: glue/standalone/nsXPCOMGlue.cpp
>+    rv = GlueStartupDebug();
>+
>+    if (NS_FAILED(rv)) {
>+        free(xpcomFunctions);
>+        xpcomFunctions = nsnull;  
>+        PR_UnloadLibrary(xpcomLib);
>+        xpcomLib = nsnull;
>+        return NS_ERROR_FAILURE;
>+    }
>+
>     // startup the nsMemory
>     return GlueStartupMemory();
> #endif

Why no error handling for GlueStartupMemory?

(I need to apply the patch and diff between moved files to review the rest of
the patch...)

Comment 9

[Doug Turner (:dougt)]

I will revert glue/standalone/Makefile.in.

The caller will take care of shutting down if GlueStartupMemory fails.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The license on nsDebugImpl.cpp should probably come from nsDebug.cpp, since it's
pretty much the same code.

Removing the |Break(aFile, aLine)| at the end of nsDebug::Assertion seems like a
major change.  It seems like that would break XPCOM_DEBUG_BREAK for Assertions
and break dropping into the debugger on Windows.  I think that should be readded.

You've also made a major change to NS_WARNING.  I think you should probably
readd nsIDebug/nsDebugImpl::Warning to restore backwards compatibility.

nsDebugImpl::Create should either null out |*aInstancePtr| at the beginning or
not bother at all.  If QI fails, the QI implementation will do it anyway.

patch thinks your glue/nsDebug.cpp changes are a malformed diff, but, if I'm
reading them correctly, you need to write ENSURE_DEBUGOBJECT (to call
SetupDebugObject, I presume).

GlueStartupDebug/GlueShutdownDebug and FreeDebugObject/SetupDebugObject both
keep an owning reference in the same variable, so I'd think we're guarantd to
leak the debug service in the XPCOM_GLUE case (we current leak the nsMemory
object, I think, so don't trust that code as reliable).  Perhaps the
GlueStartupDebug and GlueShutdownDebug are unnecessary?

Other than that (including comment 8), this looks good to me.

Comment 11

[Doug Turner (:dougt)]

Fixed above comments.

>GlueStartupDebug/GlueShutdownDebug and FreeDebugObject/SetupDebugObject both
keep an owning reference in the same variable, so I'd think we're guarantd to
leak the debug service in the XPCOM_GLUE case (we current leak the nsMemory
object, I think, so don't trust that code as reliable).  Perhaps the
GlueStartupDebug and GlueShutdownDebug are unnecessary?

The leak is a know issue.  The Glue versions are to ensure that a standalone
component (ones that don't link to xpcom) does correctly resolve its own global
variable.  

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But why bother with GlueStartupDebug and GlueShutdownDebug when you can just get
it lazily when you need it and free it at shutdown?

Comment 13

[Doug Turner (:dougt)]

well, suppose you have one of these components that are built outside of
mozilla.  these components can be (and some are) unloaded before "app shutdown".
 So, these componets need to have a way to free up memory when the component
shutdown.  When these componets are unloaded, the xpcom-glue is shutdown for
them, resulting in calling GlueShutdownDebug.

Comment 14

[Doug Turner (:dougt)]

Attachment: includes more dbaron feedback

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 127022 [details] [diff] [review]
includes more dbaron feedback

r=dbaron, although I still don't see why you shouldn't do the same cleanup for
GlueStartupMemory failure as for the other failure cases.

Comment 16

[Doug Turner (:dougt)]

yeah.  will add the extra clean up.  

Comment 17

[Doug Turner (:dougt)]

I cleaned up the patch that dbaron r'ed.  Specifically, i made it so that
nsDebug could be used before InitXPCOM.  The changes were minor.

Checking in base/Makefile.in;
/cvsroot/mozilla/xpcom/base/Makefile.in,v  <--  Makefile.in
new revision: 1.59; previous revision: 1.58
done
RCS file: /cvsroot/mozilla/xpcom/base/nsDebugImpl.cpp,v
done
Checking in base/nsDebugImpl.cpp;
/cvsroot/mozilla/xpcom/base/nsDebugImpl.cpp,v  <--  nsDebugImpl.cpp
initial revision: 3.1
done
RCS file: /cvsroot/mozilla/xpcom/base/nsDebugImpl.h,v
done
Checking in base/nsDebugImpl.h;
/cvsroot/mozilla/xpcom/base/nsDebugImpl.h,v  <--  nsDebugImpl.h
initial revision: 3.1
done
RCS file: /cvsroot/mozilla/xpcom/base/nsIDebug.idl,v
done
Checking in base/nsIDebug.idl;
/cvsroot/mozilla/xpcom/base/nsIDebug.idl,v  <--  nsIDebug.idl
initial revision: 3.1
done
Checking in base/nsISupportsObsolete.h;
/cvsroot/mozilla/xpcom/base/nsISupportsObsolete.h,v  <--  nsISupportsObsolete.h
new revision: 3.7; previous revision: 3.6
done
Checking in build/nsXPComInit.cpp;
/cvsroot/mozilla/xpcom/build/nsXPComInit.cpp,v  <--  nsXPComInit.cpp
new revision: 1.181; previous revision: 1.180
done
Checking in components/nsComponentManager.h;
/cvsroot/mozilla/xpcom/components/nsComponentManager.h,v  <--  nsComponentManager.h
new revision: 1.90; previous revision: 1.89
done
Checking in glue/nsDebug.cpp;
/cvsroot/mozilla/xpcom/glue/nsDebug.cpp,v  <--  nsDebug.cpp
new revision: 3.74; previous revision: 3.73
done
Checking in glue/nsDebug.h;
/cvsroot/mozilla/xpcom/glue/nsDebug.h,v  <--  nsDebug.h
new revision: 1.11; previous revision: 1.10
done
Checking in glue/nsISupportsImpl.h;
/cvsroot/mozilla/xpcom/glue/nsISupportsImpl.h,v  <--  nsISupportsImpl.h
new revision: 3.21; previous revision: 3.20
done
Checking in glue/standalone/nsXPCOMGlue.cpp;
/cvsroot/mozilla/xpcom/glue/standalone/nsXPCOMGlue.cpp,v  <--  nsXPCOMGlue.cpp
new revision: 1.12; previous revision: 1.11
done
Checking in build/nsXPCOM.h;
/cvsroot/mozilla/xpcom/build/nsXPCOM.h,v  <--  nsXPCOM.h
new revision: 1.10; previous revision: 1.9
done
Checking in build/nsXPCOMPrivate.h;
/cvsroot/mozilla/xpcom/build/nsXPCOMPrivate.h,v  <--  nsXPCOMPrivate.h
new revision: 1.11; previous revision: 1.10
done

I have left nsIDebug as "UNDER_REVIEW".  Of course, this will be frozen for 1.5
and any changes to this interface from today forward will also need to modify
the IID.