Closed Bug 73553 Opened 23 years ago Closed 23 years ago

[Patch] Setting the CSS style display property to none causes Mozilla to crash

Categories

(Core :: DOM: CSS Object Model, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla0.9

People

(Reporter: kjrogers, Assigned: attinasi)

Details

(Keywords: crash)

Attachments

(5 files)

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).
Assignee: jst → attinasi
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows NT → All
Hardware: PC → All
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)
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
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.
Keywords: patch
Summary: Setting the CSS style display property to none causes Mozilla to crash → [Patch] Setting the CSS style display property to none causes Mozilla to crash
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
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: