If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

standalone glue registers exit routine which is called after module unload

RESOLVED FIXED in mozilla1.9alpha1

Status

()

Core
XPCOM
P1
normal
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Benjamin Smedberg, Assigned: Benjamin Smedberg)

Tracking

Trunk
mozilla1.9alpha1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 4 obsolete attachments)

(Assignee)

Description

12 years ago
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.
At present ActiveX Plug-in uses dependent glue.
Do you mean we should use standalone glue for ActiveX Plug-in instead?
(Assignee)

Comment 2

12 years ago
Hrm, ok... I'll need to do the hard work now I guess.
(Assignee)

Comment 3

12 years ago
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.
Priority: -- → P1
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Comment 4

12 years ago
Created attachment 210394 [details] [diff] [review]
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
Attachment #210394 - Flags: superreview?(dbaron)
Attachment #210394 - Flags: review?(darin)
(Assignee)

Updated

12 years ago
Blocks: 318622
(Assignee)

Updated

12 years ago
Blocks: 317481
(Assignee)

Comment 5

12 years ago
This patch does not stub out NS_RegisterXPCOMExitRoutine, that should be dealth with as a secondary step.
(Assignee)

Comment 6

12 years ago
Created attachment 210474 [details] [diff] [review]
Make debug/tracerefcnt functionality available through C symbols, rev. 1 (-w)

This one makes nsAppRunner.cpp easier to review.

Comment 7

12 years ago
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

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

This enum should follow existing naming conventions.
(Assignee)

Comment 9

12 years ago
> 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

12 years ago
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 ;-)
(Assignee)

Comment 11

12 years ago
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

12 years ago
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.
(Assignee)

Comment 13

12 years ago
Created attachment 210601 [details] [diff] [review]
Make debug/tracerefcnt functionality available through C symbols, rev. 1.1
Attachment #210394 - Attachment is obsolete: true
Attachment #210601 - Flags: superreview?(dbaron)
Attachment #210601 - Flags: review?(darin)
Attachment #210394 - Flags: superreview?(dbaron)
Attachment #210394 - Flags: review?(darin)

Comment 14

12 years ago
> 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

12 years ago
s/NS_InitLog/NS_LogInit/
(Assignee)

Comment 16

12 years ago
> 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.
(Assignee)

Comment 17

12 years ago
Created attachment 210631 [details] [diff] [review]
Make debug/tracerefcnt functionality available through C symbols, rev. 1.2
Attachment #210601 - Attachment is obsolete: true
Attachment #210631 - Flags: review?(darin)
Attachment #210601 - Flags: superreview?(dbaron)
Attachment #210601 - Flags: review?(darin)

Comment 18

12 years ago
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
Attachment #210631 - Flags: review?(darin) → review+
(Assignee)

Comment 19

12 years ago
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 ;-)
(Assignee)

Comment 20

12 years ago
Created attachment 211006 [details] [diff] [review]
Make debug/tracerefcnt functionality available through C symbols, rev. 1.3 [checked in on trunk]
Attachment #210631 - Attachment is obsolete: true
Attachment #211006 - Flags: superreview?(dbaron)
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 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!)
Attachment #211006 - Flags: superreview?(dbaron) → superreview+

Updated

12 years ago
Depends on: 326710
(Assignee)

Comment 23

12 years ago
Created attachment 211425 [details] [diff] [review]
Remove most of the exitroutine support, rev. 1
Attachment #211425 - Flags: review?(darin)
(Assignee)

Updated

12 years ago
Attachment #211006 - Attachment description: Make debug/tracerefcnt functionality available through C symbols, rev. 1.3 → Make debug/tracerefcnt functionality available through C symbols, rev. 1.3 [checked in on trunk]
(Assignee)

Updated

12 years ago
Attachment #210474 - Attachment is obsolete: true

Updated

12 years ago
Attachment #211425 - Flags: review?(darin) → review+

Comment 24

12 years ago
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);
(Assignee)

Comment 25

12 years ago
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).
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

12 years ago
Created attachment 211494 [details] [diff] [review]
Silence bell sound on Windows

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

Comment 28

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

Bug 326981 opened for regression.
Could this have made balsa orange?
(Assignee)

Comment 30

12 years ago
Part 2 fixed on trunk, and also a bell-ifdef.
Status: NEW → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED

Updated

12 years ago
Blocks: 327997

Comment 31

12 years ago
um, you just deprecated nsIDebug which i use from python, perl, javascript, and java  to reach a c++ debugger. that's not cool.
yeah, can we undo that part of the change?

Comment 33

12 years ago
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.

Updated

8 years ago
Blocks: 507604
You need to log in before you can comment on or make changes to this bug.