Closed Bug 80084 Opened 21 years ago Closed 21 years ago

Assert on exit deleting a nsILanguateAtom in nsStyleDisplay

Categories

(Core :: CSS Parsing and Computation, defect, P2)

x86
Windows 2000
defect

Tracking

()

VERIFIED FIXED
mozilla0.9.2

People

(Reporter: mscott, Assigned: hyatt)

References

Details

Attachments

(3 files)

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?
Assignee: jst → pierre
Component: DOM Style → Style System
Hmm, reassgning to attinasi for real this time.
Assignee: pierre → attinasi
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.
Status: NEW → ASSIGNED
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.
Priority: -- → P2
Target Milestone: --- → mozilla0.9.1
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.
Blocks: 78766
Moving to m0.9.2
Target Milestone: mozilla0.9.1 → mozilla0.9.2
Reassigning to hyatt.
Assignee: attinasi → hyatt
Status: ASSIGNED → NEW
Depends on: 78695
Fixed.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
I haven't seen _this_ particular assertion recently...
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.