326837 - invalid assertions on startup [XPCOM objects created/destroyed from static ctor/dtor] (gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_Get ThreadPrivate(gActivityTLS)) == 0)

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Christian Persch (GNOME) (away; not receiving bug mail)]

With the patch from bug 318622 (checked in as part of patch from bug 325229), gtkmozembed embedders get loads of assertions on startup:

###!!! ASSERTION: XPCOM objects created/destroyed from static ctor/dtor: 'gActivityTLS != BAD_TLS_INDEX && NS_PTR_TO_INT32(PR_GetThreadPrivate(gActivityTLS)) == 0', file /opt/source/mozilla/trunk/mozilla/xpcom/base/nsTraceRefcntImpl.cpp, line 969

Adding NS_InitLog before using gtk_moz_embed_set_profile_path (and NS_TermLog after the final gtk_moz_embed_pop_startup) makes them go away, but IMHO gtkmozembed embedders shouldn't have to fiddle with that in order not to get those incorrect assertions.

They happen:
- below gtk_moz_embed_set_profile_path
- when I instantiate my nsIDirectoryServiceProvider[2] implementation class, and then use
- gtk_moz_embed_set_directory_service_provider
- gtk_moz_embed_push_startup

For example, they happen below NS_NewNativeLocalFile which is explicitly marked as ok to call before XPCOM startup.

And on shutdown they happen too, I haven't gdb'd those though.

Traces from the assertions on startup (and one on shutdown, then I quit gdb) @ http://www.gnome.org/~chpe/gdblog .

Comment 1

[Benjamin Smedberg]

I agree that gtk_moz_embed_set_profile_path and friends shouldn't assert, and I can write a patch to fix that.

I'm not so certain that instantiating your own nsIDirectoryServiceProvider shouldn't assert; I rather think it should, if you're using the NS_IMPL_ISUPPORTS* macros (which with DEBUG defined will call NS_LogAddRef/Release).

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This happens on starting a trunk seamonkey too. The assert is from GRE_Startup calling nsGREDirServiceProvider::AddRef.

Comment 3

[Christian Persch (GNOME) (away; not receiving bug mail)]

(In reply to comment #1)
> I'm not so certain that instantiating your own nsIDirectoryServiceProvider
> shouldn't assert; I rather think it should, if you're using the
> NS_IMPL_ISUPPORTS* macros (which with DEBUG defined will call
> NS_LogAddRef/Release).

Well, I *need* to instantiate my directory service provider before XPCOM startup, and I don't see why I shouldn't use the provided convenience macros to simplify my code. Also in this instance the warning is *wrong* (or I'm completely misunderstanding it), since my object is in fact new'd, and has no static ctor.

Comment 4

[Benjamin Smedberg]

To the XPCOM debugging code, any addref/release/ctor/dtor outside NS_LogInit/NS_LogTerm is treated as an expected event. It has to be this way, or how else could we know what a static ctor/dtor was? Events that take place outside the logging code will skew the results and report false-positive leaks.

If you want to use the NS_Log* functions (in this indirectly through a macro), you're going to need to initialize/term the logging code in a predictable manner.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think it might be worth excluding some classes from this warning. It can be expected that a lot of users will want to create an nsIDirectoryServiceProvider before starting XPCOM since you need one to call NS_InitXPCOM. It's a pain for users to have to implement their AddRef and Release manually just to avoid the assertion, and we'd rather that people used the macros anyway.

What we could do is to check if the object being addrefed QIs to nsIDirectoryServiceProvider, and if it does not assert. 

Comment 6

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

(In reply to comment #5)
> What we could do is to check if the object being addrefed QIs to
> nsIDirectoryServiceProvider, and if it does not assert. 

QI addrefs though...

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Hrm.. i spoke too soon. By the time we check ASSERT_ACTIVITY_IS_LEGAL we don't have any pointer we can call QI on (unless we assume that all classes inherit nsISupports as their first baseclass, which i doubt we can rely on).

So the other alternative is to provide macros that implements AddRef and Release that doesn't call NS_LOG_ADDREF. Sort of sucks to not log addref/release for the directory providers, but it doesn't look like there's anything else we can do.

Comment 8

[Benjamin Smedberg]

Sure there is, it involves calling NS_LogInit/LogTerm in your main...

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So we'd pretty much force everyone to call those manually then. Not sure if that is a big deal, but we should be aware of, and document that, that is the proper way to start up XPCOM.

Comment 10

[Benjamin Smedberg]

This is a warning now... I'm pretty sure that I do want to introduce the new behavior here since it is binary-compatible with old release binaries. But we should make XRE_InitEmbedding and the gtkmozembed bits call this automatically.

Comment 11

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

I'd say that NS_New(Native)LocalFile should call it too.

Comment 12

[Jesse Ruderman]

I get tons of these assertions during startup on Mac :(

Comment 13

[Darin Fisher]

This problem may have been made worse by the recent change to log string buffer AddRef and Release.  I think the assertions are somewhat silly.  Why can't we just initialize the TLS the first time we get a call and cleanup from the TLS destructor?  Any problem with that?

Comment 14

[Benjamin Smedberg]

Hrm? The original intent of the assertions was to catch XPCOM object allocations that happen during static module loads, which is exactly what seems to be happening, unless I'm missing something.

Comment 15

[Darin Fisher]

> Hrm? The original intent of the assertions was to catch XPCOM object
> allocations that happen during static module loads...

OK, I see... so, then perhaps the warning should mention NS_LogInit explicitly to help guide embedders in the right direction.

Comment 16

[jag (Peter Annema)]

Do we understand what's going on on Mac?

Comment 17

[Robert Strong (they/them - no direct email)]

*** Bug 349285 has been marked as a duplicate of this bug. ***

Comment 18

[Håkan Waara]

So, can we exempt the directory service provider from this warning in some (at worst hackish) way? 

This bug seems to be especially prevalent on mac, where you get millions of these warnings for every startup.

Comment 19

[Benjamin Smedberg]

We shouldn't be exempting it, we should be adding NS_LogInit/Term pairs around known-good behavior. Do you have stacks for the bogus warnings?

Comment 20

[Håkan Waara]

Attachment: startup-output.txt.gz

Here's the stacks + arguments of all such assertions (96 for this startup).

Comment 21

[Josh Aas]

I don't run Firefox in a debugger on anything but Mac OS X, but I can confirm what hwaara says about this being especially bad on Mac OS X - I see an insane number of assertions every time. Since obviously this assertion is pointless when its set off like this, can we at least disable it if nobody is going to fix this any time soon?

Comment 22

[Ben Fowler]

Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.9a1) Gecko/20061023 Minefield/3.0a1

Further to comment 18, comment 19 and comment 20: Are you asking to be told the caller responsible for each of these NS_WARNINGS on the Mac?

Comment 23

[Håkan Waara]

(In reply to comment #22)
> Further to comment 18, comment 19 and comment 20: Are you asking to be told the
> caller responsible for each of these NS_WARNINGS on the Mac?

That's what attachment 238412 [details] (comment 20) is.

Comment 24

[Ben Fowler]

Thank you. So to take the first one, do we modify the code at 
mozilla/xpcom/io/nsLocalFileOSX.cpp:340 ( I think that that is the macro 
NS_IMPL_THREADSAFE_ISUPPORTS4 ) or 
mozilla/xpcom/base/nsTraceRefcntImpl.cpp:1025 ( the macro ASSERT_ACTIVITY_IS_LEGAL ) or somewhere else entirely?

Comment 25

[Benjamin Smedberg]

Attachment: Fix the errors in XRE_main, hopefully, rev. 1 (fixed on trunk)

This might help. Josh, please test?

Comment 26

[Josh Aas]

Comment on attachment 245017 [details] [diff] [review]
Fix the errors in XRE_main, hopefully, rev. 1 (fixed on trunk)

unfortunately it looks like this doesn't really do anything

Comment 27

[Josh Aas]

Attachment: fix v1.0

Thanks for the help bsmedberg! Since we do string operations inside of nsLocalFile::Load on the mac, we get these illegal operation warnings. Other platforms don't do string operations and thus don't have those problems. We really only need to declare certain operations to be illegal during actual library loading.

Comment 28

[Josh Aas]

One thing we might want to change is whether or not we want to put the nsTraceRefcntImpl calls inside or outside of the timeline calls. I put them on the inside in my patch, now I'm leaning towards the outside, and I'll fix that on checkin.

     NS_TIMELINE_START_TIMER("PR_LoadLibrary");
 
+#ifdef NS_BUILD_REFCNT_LOGGING
+    nsTraceRefcntImpl::SetActivityIsLegal(PR_FALSE);
+#endif
+
     *_retval = PR_LoadLibrary(mPath.get());
 
+#ifdef NS_BUILD_REFCNT_LOGGING
+    nsTraceRefcntImpl::SetActivityIsLegal(PR_TRUE);
+#endif
+
     NS_TIMELINE_STOP_TIMER("PR_LoadLibrary");
     NS_TIMELINE_MARK_TIMER1("PR_LoadLibrary", mPath.get());

Comment 29

[Håkan Waara]

Comment on attachment 245515 [details] [diff] [review]
fix v1.0

bsmedberg is probably a better reviewer for this patch

Comment 30

[Josh Aas]

Comment on attachment 245515 [details] [diff] [review]
fix v1.0

Yeah, but I wanted him as an sr. I just wanted a sanity check for a first review.

Comment 31

[Benjamin Smedberg]

Comment on attachment 245515 [details] [diff] [review]
fix v1.0

>Index: io/nsLocalFileMac.cpp

Please remove this file, it is dead code.

r=me, no need for separate sr since XPCOM is strongly-owned

Comment 32

[Josh Aas]

landed on trunk

Comment 33

[Håkan Waara]

I'm still seeing this. Did it regress, or was the bug ever actually 100% fixed?

AFAICS, it is nsAppRunner code (XRE_main) that is using XPCOM that seems to be triggering all the assertions.

Comment 34

[Håkan Waara]

For example, XRE_GetBinaryPath() asserts a bunch. This line is responsible for the first three assertions: http://tinyurl.com/ycxddh

Comment 35

[Karsten Düsterloh]

> For example, XRE_GetBinaryPath() asserts a bunch. This line is responsible for
> the first three assertions: http://tinyurl.com/ycxddh

TinyURLs are pretty useless for archiving, especially if you're just linking into LXR anyway... -> <http://landfill.mozilla.org/mxr-test/mozilla/source/toolkit/xre/nsAppRunner.cpp#1076>

Comment 36

[Jesse Ruderman]

I don't think this needs to be reopened.  Bug 362318 covers the remaining reasons for this assertion to fire.

Comment 37

[Håkan Waara]

(In reply to comment #36)
> I don't think this needs to be reopened.  Bug 362318 covers the remaining
> reasons for this assertion to fire.

Ok, didn't know about that bug.

Comment 38

[Benjamin Smedberg]

Comment on attachment 245017 [details] [diff] [review]
Fix the errors in XRE_main, hopefully, rev. 1 (fixed on trunk)

Re-requesting review for a change which actually does help fix a class of errors, see bug 362318 for details.

Comment 39

[(not reading, please use [e-mail] instead)]

Comment on attachment 245017 [details] [diff] [review]
Fix the errors in XRE_main, hopefully, rev. 1 (fixed on trunk)

benjamin, did you want me to review this obsolete patch, or the other one?

Comment 40

[Benjamin Smedberg]

Seth, the obsolete one (it's not really).

Comment 41

[(not reading, please use [e-mail] instead)]

Comment on attachment 245017 [details] [diff] [review]
Fix the errors in XRE_main, hopefully, rev. 1 (fixed on trunk)

r=sspitzer, sorry for the delay benjamin!