63118 - Bug in Style Context Sharing

Status: VERIFIED FIXED

Description

[Alan S. Jones]

Win95 Build 2000121608, style conext sharing on.
When I go to 
http://www.brainjar.com/dhtml/menubar/demo.html
and click on a menu item a blured showed effect displays below the file menu.

If I turn off style context sharing with set moz_disable_style_sharing=1
and run mozilla from that window the CSS menu works fine.

FYI, here is a link to the authors page describing his script.
http://www.brainjar.com/dhtml/menubar/index2.html

Comment 1

[Boris Zbarsky [:bzbarsky, bz on IRC]]

this is probably related to the problems seen in bug 63016.  I see this one on
Linux as well, so OS -> All.

Comment 2

[HJ]

:))

Comment 3

[Marc Attinasi]

Investigating... thanks for reporting.

Comment 4

[Marc Attinasi]

So far I have learned a couple of interesting things:
1) if I modify more than one style property at a time the problem occurs. If I
only modify one property at a time, in the testcase it is visibility, then it
looks fine.
2) if I make the menu DIV display:none, and then set it to display:block to show
it, then I can modify all of the properties that I want and it works fine. This
is probably because display:none causes no frames to be created for the element,
so there is no sharing occuring at load time...

I'm still looking, but I'll attach a minimized testcase showing the one-property
vs. multiple-property scenario in case anyone else want to dig in too.

Comment 5

[Marc Attinasi]

Created attachment 20980 [details]
Reduced Testcase: shows that the menu can be displayed OK if no other properties except visibility are modified...

Comment 6

[Marc Attinasi]

I've found the problem. When sibling elements have the same style rules, their
frames will share a style context. This has been the case since before I got
involved in the Style System, presumably it was put in by Peter Linss. Anyway,
the problem happens when a modification is made to a shared style context: the
data is re-resolved correctly, but the fact that the style context is shared is
not known by the context or the context data, so the post-changed style data
ends up being shared as well. I removed the code that does the sharing of
contexts among siblings, and the problem goes away, but I'd like to see if I can
come up with a way to have the context sharing among siblings get along with the
more general style data sharing. At the very least, I need to understand how
much benefit the context sharing among siblings is giving us vs. the more
general style data sharing.

Comment 7

[Marc Attinasi]

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

Comment 8

[Marc Attinasi]

Just a note: disabling sibling sharing is not an option: the space savings we
gain from style data sharing are all but lost if we disable sibling sharing. So,
I'm working on a way to get the shared context to be 'broken off' on the element
that is having it's style rules changed, before the context data is modified, so
that the other frames that are sharing that data are not affected.

Comment 9

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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

Comment 10

[Marc Attinasi]

I have a tentative fix for this now. The problem is that sibling frames with
identical style rules are sharing entire style contexts, and when one of those
gets its style updated it is getting its own unique style context, but only
after the original, shared, context is having its data changed. The solution is
to have the ReResolveStyle method pass TRUE for the aForceUnique parameter, thus
forcing the style context to be unique before the style changes are applied to
the style data. I'll attach the patch for preliminary review, but I am still
testing it.

Comment 11

[Marc Attinasi]

Created attachment 21089 [details] [diff] [review]
Patch to nsFrameManager to for unique style contexts in ReResolvingStyleContext

Comment 12

[Marc Attinasi]

Created attachment 21090 [details]
Simpler testcase with the same problem

Comment 13

[buster]

sr=buster
just for the record, also r=karnaze

Comment 14

[Marc Attinasi]

Fix checked in: nsFrameManager.cpp

Comment 15

[Marc Attinasi]

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

Comment 16

[Hixie (not reading bugmail)]

Netscape's standard compliance QA team reorganised itself once again, so taking 
remaining non-tables style bugs. Sorry about the spam. I tried to get this done 
directly at the database level, but apparently that is "not easy because of the 
shadow db", "plus it screws up the audit trail", so no can do...

Comment 17

[Hixie (not reading bugmail)]

verified that http://bugzilla.mozilla.org/attachment.cgi?id=21090&action=view
works on windows trunk

Comment 18

[Hixie (not reading bugmail)]

...no really