Using a fresh build from this morning (5/10), I assert on exit here: nsDebug::PreCondition(const char * 0x100e243c, const char * 0x100e2420, const char * 0x100e23fc, int 81) line 434 + 21 bytes AtomImpl::~AtomImpl() line 81 + 31 bytes AtomImpl::`scalar deleting destructor'(unsigned int 1) + 15 bytes AtomImpl::Release(AtomImpl * const 0x02879a00) line 95 + 127 bytes nsCOMPtr<nsIAtom>::~nsCOMPtr<nsIAtom>() line 490 nsLanguageAtom::~nsLanguageAtom() line 59 + 14 bytes nsLanguageAtom::`scalar deleting destructor'(unsigned int 1) + 15 bytes nsLanguageAtom::Release(nsLanguageAtom * const 0x028796a8) line 50 + 151 bytes nsCOMPtr<nsILanguageAtom>::~nsCOMPtr<nsILanguageAtom>() line 490 nsStyleDisplay::~nsStyleDisplay() line 408 + 18 bytes

I see atoms held on too long assertions all the time. it's usually because someone has a static nsCOMPtr<nsIAtom>

This happened on the trunk even before the XPCDOM landing and I know nothing about the code on the stack here, this needs to be reassigned, don't know to who yet.

Looks like the style system is to blame for this, over to attinasi. Mark, does the stylesystem hold nsStyleDisplay's as global or static data?

Hmm, reassgning to attinasi for real this time.

The style system has not changed in over a week, afaIk. I'll take a look though. Does anybody know when this started?

Looks like the atom that's causing the assert when released is being released when the layout library is unloaded, that's way too late. Why I don't knnow.

I wonder if Pierre's changes to Style Data Sharing could cause this - I think he had some global style data structures or something, maybe they are getting destroyed on shutdown and it is acting like a static atom - It would also change if the use of the style system changed because I think those globals are set based on the access patterns. CC'ing waterson since he reviewed that code (I'm looking too).

It looks like there is a static StyleDisplay structure (an array of them, actually) introduced by Pierre's recent changes for Style Data Sharing. Since the StyleDisplay has a NSComPtr<nsILanguageAtom> data member, my guess is that this is causing the assertion. So, now to figure out how to fix it. I'm guessing that we will have to reset the structure to some null-state when it is done being used, but I'm just learning that code now.

Waterson threw me a nice juicy bone: I'm gonna try hooking into the module shutdown routine and clear out the global structs there - this should avoid the cost of resetting after each GetMutable sequence, and still prevent the problem of lingering references in the global temporaries.

I implemented a Shutdown method to clear the global style structs used in the GetMutable scheme, but it was still dying. There are yet more statuc style data instances (in this case, styleBlobs) in the WrapRemapStyle method. These are static to the function, so they cannot be cleaned up in the shutdown method unless I move them out to file-static scope. This is really really ugly. Another approach is to clear these static blobs at the end of the function - why are they even static I wonder?

Oh brother, what a mess. How long until hyatt lands his stuff? :-/ I'd say try to make the style blobs use automatic storage for now.

I just attached a patch that keeps the blobs as static (but file-static now) and cleans them up. I'll try making them automatic, but I have not yet obtained full-enlightenment on the WrapRemapStyle call, so I cannot tell if they really NEED to be static, or if it is just a trick to keep the stack small, or if it was a cut-copy-past thingy, or what. I'll try it, but I don't want to spend the next five days testing this, I jsut want to get rid of the assertion (and spend the next five days praying for Hyatt's changes) ;)

The later two patches (id 34165 and 34166) are needed regardless of how we deal with the WrapRemap call - they provide the Module Shutdown hook into the style context code.

I have this completely backed out on my branch. Fixing this prior to my landing could create havoc for me merging, but I guess it's a crash.... argh.

It's an assertion, not a crash, and it happens on exit. What if we just converted the offending nsCOMPtr<nsILanguageAtom> member into a raw COM ptr and manage the ref counting (or even let it leak)? that would be less to merge, and it would fix the assertion. We could also just live with it, but it could be a while and the raw-ptr hack would fix the assert. What do you think, David?

IIRC, pierre made these static because he saw some performance win over auto, on Mac. Hard to believe, not plausible on most architectures, and not worth the hacking-with-globals pain? /be

I seem to remember that there was some non-trivial performance penalty for constructing those things every time you enter the function. Can we move them into a singleton class that manages their creation and destruction?

My understanding is that Hyatt's new style changes make this obsolete, so I don't want to put much effort into this - I just want the assertion to go away.

Moving to m0.9.2

Reassigning to hyatt.

Fixed.

I haven't seen _this_ particular assertion recently...