From Bugzilla Helper: User-Agent: Mozilla/4.0 (compatible; MSIE 5.5; Windows NT 4.0) BuildID: 2001032604 I have been trying to set the display property to none in a stylesheet via javascript. It is very effictive. It not only hides the text for the chosen class, it also hides Mozilla as well. Ie Mozilla crashes. Reproducible: Always Steps to Reproduce: 1. Start the enclosed test file "style.html" 2. Press the toggle 1 button - This displays the hidden text. 3. Press the toggle 1 button again - Mozilla crashes. Actual Results: Mozilla crashes the 2nd time that the toggle 1 button is pressed. Expected Results: The toggle 1 button should toggle the visibility of the div.annotation class. This works in IE5. The toggle 1 button uses the style.display property directly. Ie style.display = "inline" should display the text. style.display = "none" should hide the text. According to the DOM2 style recommendation the style properties should be set using the CSSStyleDeclaration.setProperty method. This technique is used for the toggle 2 button. However, this does nothing at all.

Confirming and reassigning to attinasi, Mark, the attached patch fixes setProperty() to do the right thing but that makes it show the same crash that setting style.display shows. The crash is in StyleSetImpl::FindMatchingContext() and that code has your name all over it, looks like this crash is due to refering to deleted memory. Feel free to check in the attached patch when you fix the crash, or reassign back to me once the crash is fixed. Since this is a crash in the stylecontext code I'm nominating this for nsbeta1 and mozilla0.9. Kevin, both style.display="none" and style.setProperty("display", "none", "important") are per the DOM spec, both should work in Mozilla, and with the attached patch they both do the same thing (appart from the priority difference in your testcase).

Accepting: I'm seeing an assertion first, from the ReconstructDocElementHierarchy call, because there is an entry in the UndisplayedMap that has a style context that cannot be found in the style context cache. Then later, when we crash, it is in the style context cache too. I believe that this is related to the crash when a pref is changed: that causes a full reframe too (bug 68208 and bug 64025, which are the same problem I think)

I think I have figured this one out now. What appears to be happening is that in RemapStyle, an existing style context is getting reevaluated and the CRC is changing, but the style context cache (in teh StyleSet) is never being notified, so it ends up with the context mapped to the old CRC and not the new CRC. The fix, which I am testing out, is to remove the style context from the cache when the CRC is changed, and re-add it at when the new CRC is computed. Strangely enough, this seems to happen only very rarely...

Have patch that fixes this and all of the 'change pref and crash' bugs. Problem is that RemapStyle can sometimes change the CRC of a context, in which case we need to update the style context cache for ALL contexts that share the style data of the changed context. How did this slip by for so long I wonder... Anyway, this is my fault - when I did the style sharing I didn;t realizer that this could happen, I thought we always created new contexts for reresolving style. Will attach work-in-progress patch: need to be cleaned up a bit before asking for reviews, although others can certainly test it if they like (and look it over and make suggestions).

Patch is attached. A more tidy one is comig shortly, but this one has the important parts.

The new patch (http://bugzilla.mozilla.org/showattachment.cgi?attach_id=30613) is ready for a review. This is less critical if Pierre's new style data sharing goes in with Style Context Sharing turned OFF (bug 43457), since the problem is in Style Context Sharing code, but we need to fix this if we ever plan to re-enable the Style Context Sharing. Requesting review from pierre, and super-review from waterson - any other eyes would of course be nice too.

(I presume we're missing the diffs for nsIStyleSet.h?) As a top-line comment, I don't understand why you would move *all* the style contexts from one bucket to another. Isn't the hash -- excuse me, CRC :-) -- of just *one* of the style contexts changing? Why isn't it UpdateStyleContextKeyFor(scKey aOldKey, scKey aNewKey, nsIStyleContext* aContext); Could you elaborate? Is it really necessary to rehash all of the style contexts in a bucket? + void SetCRC32(void) { mCRC = ComputeCRC32(0); if(mCRC==STYLEDATA_NO_CRC) mCRC=STYLEDATA_DEFAULT_CRC; } Consistent whitespace? + PRBool EnforceInvariants(void) { return mEnforceInvariants; } EnforceInvariants() sounds awful imperative (as in, ``enforce your invariants now, buddy!''). Since it's a getter, how about IsEnforcingInvariants() or something. (Is this even called from anywhere?) + // nested class to block invariants within a function call (or scoping block) + // - instantiate a BlockInvariants instance to Suspend/Resume invariants according + // to scoping of the instance + class BlockInvariants { + public: Nice use of helper class. Typically these are named Auto[something] (e.g., nsAutoLock). + PRInt32 i; + for (i=0; i<count;i++) { Waah! I'm going blind! Inconsitent whitespace. Also, since `i' is not used outside the loop, it can be declared inside the `for ('. + nsIStyleContext *pContext = (nsIStyleContext *)contexts->ElementAt(i); Use NS_STATIC_CAST Although it's not in the scope of this change, you should rename ``Tickle'' to ``Verify''. +nsresult StyleContextCache::UpdateCRC(scKey aOldKey, scKey aNewKey) Again, don't we really only care rehashing just *one* of the contexts at any given time? As a complete tangent, I did spend some time understanding this implementation a bit more thoroughly. I think you wrote too much code :-). Specifically, I don't think you really needed to use a hash table to map CRC's to a void array. What you really wanted to do was to have a hashtable that could find an existing style context could be shared. Your ``hash key'' is the CRC, your ``hash value'' is the style context. Both plhash and pldhash have a ``hash callback'' (which corresponds to your CRC computation) and a ``match callback'' (which corresponds to your value comparison calls to nsIStyleContext::StyleDataMatches). Anyway, future task, but we could probably eliminate a ton of code here by re-writing using pldhash. (I say pldhash, because in the end, you've got a table of pointers -- your values are extremely small.)

CW------------------------------------------------------------------------- (I presume we're missing the diffs for nsIStyleSet.h?) yes, I missed them - sorry, I'll attach. CW------------------------------------------------------------------------- As a top-line comment, I don't understand why you would move *all* the style contexts from one bucket to another. Isn't the hash -- excuse me, CRC :-) -- of just *one* of the style contexts changing? Why isn't it UpdateStyleContextKeyFor(scKey aOldKey, scKey aNewKey, nsIStyleContext* aContext); Could you elaborate? Is it really necessary to rehash all of the style contexts in a bucket? Yes, they all have to be remapped. The reason is that there can be multiple StyleContext instances in the cache that all share the same StyleContextData instance, and that is where the CRC lives. So just moving the one style context may still leave others in the hash table at the wrong key (those would be othere style contexts that share the style data). CW------------------------------------------------------------------------- + void SetCRC32(void) { mCRC = ComputeCRC32(0); if(mCRC==STYLEDATA_NO_CRC) mCRC=STYLEDATA_DEFAULT_CRC; } Consistent whitespace? OK - sorry. I'm guessing you want: + void SetCRC32(void) { mCRC = ComputeCRC32(0); if (mCRC==STYLEDATA_NO_CRC) mCRC = STYLEDATA_DEFAULT_CRC; } CW------------------------------------------------------------------------- + PRBool EnforceInvariants(void) { return mEnforceInvariants; } EnforceInvariants() sounds awful imperative (as in, ``enforce your invariants now, buddy!''). Since it's a getter, how about IsEnforcingInvariants() or something. (Is this even called from anywhere?) OK, looks like a better name. It is called from the Tickle method to see if it should do its thing - in fact, this is only private so maybe just using the mEnforceInvariants member is good enough. CW------------------------------------------------------------------------- + // nested class to block invariants within a function call (or scoping block) + // - instantiate a BlockInvariants instance to Suspend/Resume invariants according + // to scoping of the instance + class BlockInvariants { + public: Nice use of helper class. Typically these are named Auto[something] (e.g., nsAutoLock). Thanks, and sure, I'll rename it to nsAutoInvariantBlocker CW------------------------------------------------------------------------- + PRInt32 i; + for (i=0; i<count;i++) { Waah! I'm going blind! Inconsitent whitespace. Also, since `i' is not used outside the loop, it can be declared inside the `for ('. OK, sorry to hurt your eyes. I don't generally like the decl in the for, but whatever, I'll move it (in a previous life, I had to use compilers that treated this inconsistently, but that is ancient history). CW------------------------------------------------------------------------- + nsIStyleContext *pContext = (nsIStyleContext *)contexts->ElementAt(i); Use NS_STATIC_CAST OK - thanks. CW------------------------------------------------------------------------- Although it's not in the scope of this change, you should rename ``Tickle'' to ``Verify''. Agreed. Actually, I renamed it to AssertInvariants but did not want to include it in this (unrelated) patch. CW------------------------------------------------------------------------- +nsresult StyleContextCache::UpdateCRC(scKey aOldKey, scKey aNewKey) Again, don't we really only care rehashing just *one* of the contexts at any given time? No, see previous answer. The confusion is due to the way the style context data is shared, so I'll have to comment that so it is clear. CW------------------------------------------------------------------------- As a complete tangent, I did spend some time understanding this implementation a bit more thoroughly. I think you wrote too much code :-). Specifically, I don't think you really needed to use a hash table to map CRC's to a void array. What you really wanted to do was to have a hashtable that could find an existing style context could be shared. Your ``hash key'' is the CRC, your ``hash value'' is the style context. Both plhash and pldhash have a ``hash callback'' (which corresponds to your CRC computation) and a ``match callback'' (which corresponds to your value comparison calls to nsIStyleContext::StyleDataMatches). Anyway, future task, but we could probably eliminate a ton of code here by re-writing using pldhash. (I say pldhash, because in the end, you've got a table of pointers -- your values are extremely small.) Yea, Brendan told me about this about a month ago. At the time I wrote it (6 months ago) I did not know about the pldhash - I asked several people about existing data structures I could use for non-unique hashing, but nobody mentioned it. Reusing stuff is hard when there is no catalogue of what we have to share. I think it would be worthwhile to rewrite the StyleContextCache to use the pldhash now that I know what it is. Thanks Chris, I'll make your suggested changes and attach a new patch.

> Yes, they all have to be remapped. The reason is that there can be multiple > StyleContext instances in the cache that all share the same StyleContextData > instance, and that is where the CRC lives. Gotcha: sorry to be dense; I was thinking that the StyleContextData were the things in the cache. In the interest of fixing the crasher, let's get this checked in, sr=waterson. We can re-evaluate the details of what should be shared, using pldhash, etc. once we land pierre's stuff.

Fix checked in: nsIStyleSet.h v3.35 nsStyleSet.cpp v3.87 nsStyleContext.cpp v3.158

I retested the test case for the nightly build of 19 April. The toggle 1 button worked, but the toggle 2 button did not. We downloaded revision 0.9 of Mozilla (7 May). The original bug was still present. Should this bug fix be included in the official 0.9 build? It does not appear that the patch for setProperty has been included in either of the above builds.

Sorry, My last comment was partially a false alarm. I was testing the wrong version. However, I have retested the initial test case using version 0.9. The toggle1 button works. This uses style.display = "none"; The toggle2 button does not work. This uses style.setProperty.