325229 - standalone glue registers exit routine which is called after module unload

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Benjamin Smedberg]

From bug 320988 comment 11 and codeflow examination, the xpcom standalone glue is registering an XPCOM exit routine but then the module is being unloaded before XPCOM, meaning that the app crashes at shutdown trying to run the exit routine.

This will probably also become an issue with the dependent glue when we start unloading components, but it's easy to fix for the standalone glue (since we have the XPCOMGlueShutdown hook) and harder to fix for dependent glue, so I plan on just fixing the standalone glue here.

Comment 1

[Masatoshi Kimura [:emk]]

At present ActiveX Plug-in uses dependent glue.
Do you mean we should use standalone glue for ActiveX Plug-in instead?

Comment 2

[Benjamin Smedberg]

Hrm, ok... I'll need to do the hard work now I guess.

Comment 3

[Benjamin Smedberg]

I have a patch mostly in-hand, but I'm concerned about components linking against old versions of the glue: when we start actively unloading XPCOM component DLLs, those components will crash. We can fix this by making the NS_RegisterXPCOMExitRoutine a no-op function. This won't matter in practice because the nsITraceRefcnt and nsIDebug objects are becoming statics, so they can't be leaked.

Comment 4

[Benjamin Smedberg]

Attachment: Make debug/tracerefcnt functionality available through C symbols, rev. 1

This patch should fix the bug at hand, as well as incorporating the patch in bug 318622 and the functionality (though not the precise patch) in bug 317481

Comment 5

[Benjamin Smedberg]

This patch does not stub out NS_RegisterXPCOMExitRoutine, that should be dealth with as a secondary step.

Comment 6

[Benjamin Smedberg]

Attachment: Make debug/tracerefcnt functionality available through C symbols, rev. 1 (-w)

This one makes nsAppRunner.cpp easier to review.

Comment 7

[Darin Fisher]

I'm not getting something... what's the advantage to introducing these new XPCOM entry points over simply making the implementation of nsIDebug and nsITraceRefCnt static?

Making NS_RegisterXPCOMExitRoutine a no-op function sounds essential to me since you can't avoid components that are built against "old" xpcom glue.

Comment 8

[Darin Fisher]

> enum NSDebugBreakSeverity {
>     NSDebugWarning = 0,
>     NSDebugAssertion = 1,
>     NSDebugBreak = 2,
>     NSDebugAbort = 3
> };

This enum should follow existing naming conventions.

Comment 9

[Benjamin Smedberg]

> I'm not getting something... what's the advantage to introducing these new
> XPCOM entry points over simply making the implementation of nsIDebug and
> nsITraceRefCnt static?

It offended my architectural sensibilities to have the "strong" refs to the debug/tracerefcnt objects which were "leaked", even if the underlying object is not refcounted. I suppose we could do that :-(

I was very happy to finally unify all the variant codepaths in nsDebugImpl.cpp, though: I really like the current approach.

> Making NS_RegisterXPCOMExitRoutine a no-op function sounds essential to me
> since you can't avoid components that are built against "old" xpcom glue.

I agree, but I'd still like to do it in a separate patch.

I'll remake this patch with the enum NS_ALL_CAPS style.

Comment 10

[Darin Fisher]

I was thinking that since these methods are low-frequency enough, it might be nice to have a simple getter function that returns a table of function pointers for the various NS_Log functions.  (Having NS_DebugBreak as an exported function is nice as it is similar to MS's DebugBreak function.)

For the NS_Log functions, you could do:

  NS_GetLogAPI()->LogAddRef(...);

...provided you had a NS_GetLogAPI that promised to always return a non-null struct pointer ;-)

Comment 11

[Benjamin Smedberg]

I'm not sure how that's more efficient than just exporting (and glueing) the functions directly: otherwise we're going to end up with different codepaths for dependent and standalone linkage, which in general I'm trying to avoid.

Comment 12

[Darin Fisher]

I wasn't suggesting that it would be more efficient ;-)  I was suggesting it as a way to reduce the number of exported symbols, which has a certain appeal by itself, especially considering that these aren't APIs that are meant to be used "explicitly" by consumers.

Since you need to keep NS_GetDebug and NS_GetTraceRefcnt around, the function table (for NS_Log methods) will need to exist somewhere for NS_GetTraceRefcnt, right?  So, my NS_GetLogAPI suggestion is really just a variant on NS_GetTraceRefcnt.

Anyways, your goal of avoiding separate code paths is something that resonates with me too.  So, there you have it... if you want to stick with NS_Log exports, that's fine by me.

Comment 13

[Benjamin Smedberg]

Attachment: Make debug/tracerefcnt functionality available through C symbols, rev. 1.1

Comment 14

[Darin Fisher]

> enum nsAssertBehavior {
>   NSAssertUninitialized,
>   NSAssertWarn,
>   NSAssertSuspend,
>   NSAssertStack,
>   NSAssertTrap,
>   NSAssertAbort
> };

nit: naming convention


> char *assertString = PR_GetEnv("XPCOM_DEBUG_BREAK");

nit: change that to |const char *|


> static nsAssertBehavior GetAssertBehavior()

This function is missing a return statement in the "unrecognized value" case.


In NS_DebugBreak, should we be concerned about minimizing stack usage?  If so, we could use PR_sxprintf with PL_strcatn and just the single 1000 byte stack buffer.


> #if defined(XP_WIN) && PR_BYTES_PER_LONG == 4
> typedef unsigned long nsrefcnt;
> #else
> typedef PRUint32 nsrefcnt;
> #endif

Why aren't we just using PRUint32 for all platforms?


> NS_LogInitStatistics

I think you should completely document all of the NS_Log functions.

what do you think about renaming NS_LogInitStatistics to just NS_InitLog?


> typedef void       (* VoidFunc)();

Good idea adding this, but it seems like it might stand a chance of conflicting with a consumer defined typedef.  Perhaps we should namespace this typedef (and maybe the rest of them as well)?  Though, it looks like this header file should normally not be included by anyone except our glue implementation.


It'd be really nice if logging didn't require explicit initialization and termination (via NS_LogInitStatistics and NS_LogTermStatistics).


Also, we should make it clear what the behavior of these functions is in a non-debug build.

Comment 15

[Darin Fisher]

s/NS_InitLog/NS_LogInit/

Comment 16

[Benjamin Smedberg]

> In NS_DebugBreak, should we be concerned about minimizing stack usage?  If so,
> we could use PR_sxprintf with PL_strcatn and just the single 1000 byte stack
> buffer.

Too bad we don't have universal alloca support. Let me look at what that would involve, I'm not sure I've used those APIs before.


> > #if defined(XP_WIN) && PR_BYTES_PER_LONG == 4
> > typedef unsigned long nsrefcnt;
> > #else
> > typedef PRUint32 nsrefcnt;
> > #endif
> 
> Why aren't we just using PRUint32 for all platforms?

I just cut-n-pasted that from nsISupportsBase.h so that it was available in nsXPCOM.h... binary compatibility with MSCOM is the goal.

> It'd be really nice if logging didn't require explicit initialization and
> termination (via NS_LogInitStatistics and NS_LogTermStatistics).

It's not required, see bug 318622 comment 6. NS_InitXPCOM3 and NS_ShutdownXPCOM call the init/term functions, so you only need to call them explicitly if you want to delay tracerefcnt log dumping past XPCOM shutdown (which the toolkit apprunner does so that it can release various nsIFiles and do cleanup).

> Also, we should make it clear what the behavior of these functions is in a
> non-debug build.

Yes, "they work" as long as you have symbols.

Comment 17

[Benjamin Smedberg]

Attachment: Make debug/tracerefcnt functionality available through C symbols, rev. 1.2

Comment 18

[Darin Fisher]

Comment on attachment 210631 [details] [diff] [review]
Make debug/tracerefcnt functionality available through C symbols, rev. 1.2

Any reason why you didn't want to use PL_strcatn?  The implicit strlen isn't costly enough to worry about right?  I thought PL_strcatn would simply the PR_sxprintf callback function greatly.  But, whatever works.  You might want to put an "if (len) {" test around the memcpy and the next two lines of code.


> refcount statistics are are printed at NS_ShutdownXPCOM

"are are"


> NSVoidFunc 

Seriously, what's with the "NS" prefix? ;-)  Should we worry about collisions with Apple's toolkit?


r=darin

Comment 19

[Benjamin Smedberg]

Yes: looking at the PR_sxprintf code it doesn't look like the data passed to the callback func is guaranteed to be null-terminated.

xpcomVoidFunc ;-)

Comment 20

[Benjamin Smedberg]

Attachment: Make debug/tracerefcnt functionality available through C symbols, rev. 1.3 [checked in on trunk]

Comment 21

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

Comment on attachment 211006 [details] [diff] [review]
Make debug/tracerefcnt functionality available through C symbols, rev. 1.3 [checked in on trunk]

>+nsDebugImpl::AddRef()
>+{
>+  return 1;
>+}

Dummy AddRef implementations should probably return 2.  (Release returning 1 is good.)

Comment 22

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

Comment on attachment 211006 [details] [diff] [review]
Make debug/tracerefcnt functionality available through C symbols, rev. 1.3 [checked in on trunk]

sr=dbaron, based on a pretty quick look, and completely ignoring the nsAppRunner.cpp changes (no diff -b!)

Comment 23

[Benjamin Smedberg]

Attachment: Remove most of the exitroutine support, rev. 1

Comment 24

[Bernd]

Could you please remove the \07 from the assert strings under windows there is allready a system sound for asserts ( its a pain...)
D:\moz_src\mozilla\xpcom\base>cvs diff -u8p  nsDebugImpl.cpp
Index: nsDebugImpl.cpp
===================================================================
RCS file: /cvsroot/mozilla/xpcom/base/nsDebugImpl.cpp,v
retrieving revision 3.17
diff -u -8 -p -r3.17 nsDebugImpl.cpp
--- nsDebugImpl.cpp     10 Feb 2006 15:00:25 -0000      3.17
+++ nsDebugImpl.cpp     11 Feb 2006 14:29:50 -0000
@@ -260,27 +260,27 @@ NS_DebugBreak(PRUint32 aSeverity, const
    InitLog();

    FixedBuffer buf;
    PRLogModuleLevel ll = PR_LOG_WARNING;
    const char *sevString = "WARNING";

    switch (aSeverity) {
    case NS_DEBUG_ASSERTION:
-     sevString = "\07###!!! ASSERTION";
+     sevString = "###!!! ASSERTION";
      ll = PR_LOG_ERROR;
      break;

    case NS_DEBUG_BREAK:
-     sevString = "\07###!!! BREAK";
+     sevString = "###!!! BREAK";
      ll = PR_LOG_ALWAYS;
      break;

    case NS_DEBUG_ABORT:
-     sevString = "\07###!!! ABORT";
+     sevString = "###!!! ABORT";
      ll = PR_LOG_ALWAYS;
      break;

    default:
      aSeverity = NS_DEBUG_WARNING;
    };

    PR_sxprintf(StuffFixedBuffer, &buf, "%s: ", sevString);

Comment 25

[Benjamin Smedberg]

The bells were copied from the existing unix codepath; if they need to be removed it will need some sort of platform ifdef (I'm not convinced the extra noise is a bad thing).

Comment 26

[Frank Wein [:mcsmurf]]

It is a pain, especially since you already get notified via a dialog box on Windows about a assertion (and the Windows chord.wav sound). The internal PC speaker is really annoying compared to this wav file.

Comment 27

[James Ross]

Attachment: Silence bell sound on Windows

This is an untested (still building after yesterdays redness) patch to stop the bell sound on Windows.

Comment 28

[Doug Shelton]

these fixes appear to have broken BeOS building in mozilla/xpcom/base/nsDebugImpl.cpp

Bug 326981 opened for regression.

Comment 29

[Boris Zbarsky [:bzbarsky]]

Could this have made balsa orange?

Comment 30

[Benjamin Smedberg]

Part 2 fixed on trunk, and also a bell-ifdef.

Comment 31

[timeless]

um, you just deprecated nsIDebug which i use from python, perl, javascript, and java  to reach a c++ debugger. that's not cool.

Comment 32

[Christian :Biesinger (don't email me, ping me on IRC)]

yeah, can we undo that part of the change?

Comment 33

[Darin Fisher]

It still works, right?  You just want it to be non-deprecated?  If we do that then we should also document the API.  Remember that it was a private API created only for the purpose of supporting the XPCOM glue.  Please file a new bug if you want a scriptable API to nsIDebug.