188803 - make style rules immutable?

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I've been thinking about some of the bugs we have in the style system (various
odd issues with dynamic changes and such) in general, and thinking that the code
might end up awfully complicated if we try to fix all of them in the current system.

So an idea I've been thinking about is that perhaps we should make nsIStyleRule
objects immutable.

In other words, DOM rules would be wrappers for nsIStyleRule, and when they
change due to dynamic style rule manipulation, we create a new nsIStyleRule
inner object.  (We need wrappers anyway to handle the many-to-one selectors
issue in bug 98765.)

This would make garbage collecting the rule tree every so often a little bit
more important, but it's not too hard (see bug 117316).

This would have a number of advantages:
 * It would allow us to use comparative data for all types of style changes, and
get rid of the hints in the backend (I think, anyway, although I admit to being
confused every time I look at the number of ways those hints are used).
 * It would remove the need to do clearing of data (I think) and the associated
bugs.
 * It would allow DidSetStyleContext callbacks to be used usefully for handling
style changes (I suspect the places that use them now are buggy, since they
don't get called all the time)

I admit I haven't looked at what dynamic change bugs we have lately, so I'm not
exactly sure what problems need to be solved now.  But I'm not sure the current
system of mutable rules makes sense with the rule tree.

Thoughts?

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

By immutable, I mean that the style data represented by a given style rule
object would not change once it has been walked by
nsIStyleRuleProcessor::RulesMatching.  

(We wouldn't have problems with pointer aliasing since the unused old rules
would have strong references from the rule tree keeping them alive until we
garbage collect the rule tree.)

Comment 2

[Boris Zbarsky [:bzbarsky]]

> It would remove the need to do clearing of data (I think)

Yes, I think it would.  This is basically what I was suggesting for inline
style.... I've tried a gross hack impl of this (for inline style) locally, and
it seems to have some perf cost on "DHTML" testcases.... but it was a _very_
gross hack.  If people want, I can try to hack together a more advanced
proof-of-concept for inline style.

The biggest win for inline style, of course, is that the hinting stuff Just
Works (since we get a new style context), eliminating the need for my kludge
over in bug 171830.  If we can do this without slowing down inline style changes
too much, I think we should.

When I talked to hyatt about this recently he suggested that we could leave the
existing inline style rule-to-rulenode hash in place and use that to selectively
prune the tree when inline style changes; it's not clear whether this would be
faster than a periodic full sweep through the tree or not.

So my suggestion is to see whether we can do this for inline style; if we can, I
would support doing it globally.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Bug 171830 made considerable progress on this.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: the beginnings of a patch

The main thing of interest is the comment.  I really can't go any further until
bug 125246 lands, because otherwise I'll get massive conflicts.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: the beginnings of a patch

This does a bit more -- all that's really left is the changes to
nsCSSStyleRule.cpp to destroy and create rules when DOM rule mutation APIs are
used.  However, doing that correctly depends on both bug 125246 and bug 98765.

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This should be a pretty much complete patch.  I haven't tested it at all yet
(it compiles, but my build isn't done the other directories yet), and it's not
going to work completely right until bug 98765 lands (and when that happens it
will need a little merging).

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

This runs and passes the simple testcase I'm about to attach.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: simple testcase for rule change, removal, and addition

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Merged with bug 98765.

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Made it compile, fixed another problem (merge error, I think).

Comment 11

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125676 [details] [diff] [review]
patch

>Index: content/base/public/nsIDocumentObserver.h

>+ However, the use of this method (rather
>+   * than StyleRuleAdded and StyleRuleRemoved) implies that the new rule
>+   * matches the same elements and has the same priority (weight,
>+   * origin, specificity) as the old one.

That's not strictly true in cases when the priority of one of the declarations
got changed (we may have created an mImportantRule where there was none
before).

This could use some documentation on what it is and is not legal to do with
aOldStyleRule... Otherwise some enterprising soul might try to compare the
declarations of the two rules (bad idea).

As far as I can tell, the only thing you can do with aOldStyleRule here is use
it (or a QIed version of it) as a pointer -- accessing any actual style data
off it is verboten.

>    * @param aDocument The document being observed
>    * @param aStyleSheet the StyleSheet that contians the rule
>    * @param aStyleRule the rule that was modified
>    * @param aHint some possible info about the nature of the change

These last two could use updating

>Index: content/base/public/nsIStyleRule.h

>+ * points to the rule.  (A rule node, |nsRuleNode|, is a node in the
>+ * rule tree, which is a node in the lexicographic tree

"which is a lexicographic tree"

>+   * |nsIStyleRule::MapRuleInfoInto| is a request to copy all stylistic
>+   * data represented by the rule <b>that are not already filled in</b>
>+   * in the rule into the appropriate data struct in |aRuleData| that:
>+   *   + is relevant for |aRuleData->mSID| (the style struct ID)
>+   *   + is not already filled into the data struct

This part looks like it got half-rewritten and then forgotten about... In
particular, "that are not already filled in in the rule" makes no sense.

>Index: content/html/style/src/nsCSSRules.cpp

>+CSSMediaRuleImpl::ReplaceStyleRule(nsICSSRule* aOld, nsICSSRule* aNew)

This function assumes that the caller holds a ref to aOld.  More on this below.

>Index: content/html/style/src/nsCSSStyleRule.cpp


> /*
>  * This is a utility function.  It will only fail if it can't get a
>  * parser.  This means it can return NS_OK without all of aSheet,
>  *  aDocument, aURI, aCSSLoader being initialized
>  */

aDociment and aSheet can be removed from that comment...

>+nsresult 
>+DOMCSSDeclarationImpl::GetParent(nsISupports **aParent)

I have to wonder why this does not return nsICSSStyleRule or nsIStyleRule or
whatever type mRule is.... the only caller seems to immediately QI it.... (I
know you didn't change this; probably material for a separate bug...)

>+nsresult
>+DOMCSSDeclarationImpl::SetCSSDeclaration(nsCSSDeclaration* aDecl)

>+  nsICSSStyleRule *oldRule = mRule;
>+  mRule = oldRule->ReplaceDeclaration(aDecl);

Hmm... shouldn't we hold a strong ref to mRule here?  It's possible that 
the sheet that mRule is in is holding the only ref to it; after this call
oldRule may point to deleted data as currently written...

Also, see comment above about the ReplaceRuleInGroup impl assuming the caller
holds a strong ref to the old rule.

>+CSSStyleRuleImpl::CSSStyleRuleImpl(CSSStyleRuleImpl& aCopy,
>+                                   nsCSSDeclaration* aDeclaration)

Hmm... This code makes the assumption that aDeclaration == aCopy.mDeclaration,
no?  Otherwise it needs to addref aDeclaration and leave aCopy.mDeclaration
be....

Also, please comment why we null out aCopy.mDOMDeclaration.

>Index: content/html/style/src/nsCSSStyleSheet.cpp

>+CSSStyleSheetImpl::ReplaceStyleRule(nsICSSRule* aOld, nsICSSRule* aNew)

This function assumes the caller holds a strong ref to aOld (since otherwise
the only ref may be that held by mOrderedRules).

>+NS_IMETHODIMP
>+CSSStyleSheetImpl::ReplaceRuleInGroup(nsICSSGroupRule* aGroup,
>+                                      nsICSSRule* aOld, nsICSSRule* aNew)
>+{
>+  nsresult result;
>+  NS_ASSERTION(mInner && mInner->mComplete,
>+               "No inserting into an incomplete sheet!");

Could we change the assertion text to reflect what we are really doing?  And
add a similar assert to ReplaceStyleRule?

>Index: content/html/style/src/nsDOMCSSAttrDeclaration.cpp


>@@ -143,53 +108,63 @@ nsDOMCSSAttributeDeclaration::GetCSSDecl
>+      NS_ASSERTION(mContent, "Must have content node to set the 
decl!");

This code is inside a |if (mContent)| block, so this assert is sorta
superfluous....

>Index: content/html/style/src/nsDOMCSSDeclaration.cpp
>+    // RemoveProperty will throw in all sorts of situations -- eg if
>+    // the property is a shorthand one.  Do not propagate its return
>+    // value to callers.

Actually, that's no longer correct since the last nsCSSDeclaration rewrite...
at this point, RemoveProperty _never_ throws.  I'm still not sure that going
back to propagating its return value here is a good idea, though...

One thing that sort of bothers me is that we will end up blowing away and
reresolving all style data for DOM changes that don't actually modify the
declaration... Eg something like setting a value to a value it already has, or
passing in strings that are not valid CSS... it would be good to catch these
no-op cases and not even fire the StyleRuleChanged notifications (or
AttributeChanged notifications in the case of inline style) in those cases.  I
suppose that could be a separate patch...

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Once I make sure it compiles, I'll attach a revised patch that addresses all the
comments above.  I addressed the comments about passing rules with a refcount of
0 to functions I fixed by making sure I don't do that -- thus I didn't add any
comments about it since that's the standard XPCOM rule.

I'll leave the optimization for a later patch.  After all, before this patch,
adding a rule caused the entire frame tree to be rebuilt...

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Per discussion with bz, replace nsDOMCSSDeclaration::SetCSSDeclaration and
nsICSSStyleRule::ReplaceDeclaration with DeclarationChanged (which no longer
takes an |aDeclaration| parameter.  Furthermore, change the latter by adding an
|aHandleContainer| parameter and making it return
|already_AddRefed<nsICSSStyleRule>| so that
nsDOMCSSAttrDeclaration::DeclarationChanged can use
nsICSSStyleRule::DeclarationChanged, and modify
nsDOMCSSAttrDeclaration::DeclarationChanged to use it.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

So my crash (the reason I obsoleted the last patch) was because of a mistake
made back when implementing bug 109261 that nobody's noticed.  PeekStyleData
was being far more invasive than it needed to be.  If there's no cached data on
the style context or rule node and no dependent bits on the rule node then the
data have never been accessed.	(I asked hyatt, and he agrees with me that what
the old code did is not necessary.)  So this fixes a crash calling
MapRuleInfoInto on a "dead" rule in a PeekStyleData on a branch of the rule
tree containing the old rule.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 125848 [details] [diff] [review]
patch

Fun.  r+sr=me

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: testcase for rule change, removal, and addition within @media

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in, 2003-06-17 18:59 -0700.

We're technically not fully to style rule immutability yet -- there are some
rules that aren't immutable.  However, those rules are ones where we currently
make no attempt whatsoever to handle dynamic changes.  I should file separate
bugs on them.  (BodyRule, and perhaps the various table things in
nsHTMLStyleSheet that need to be rewritten anyway (bug 144899)).  And I should
probably tweak syntax / constness for nsHTMLMappedAttributes an HTMLColorRule so
they're harder to mutate.

Comment 19

[Boris Zbarsky [:bzbarsky]]

I just realized something... now that we create new nsIStyleRule objects on each
mutation, we want to make the object implementing nsIDOMCSSStyleRule be a
different object.  Otherwise something like:

var rule = document.styleSheets[0].cssRules[0];
rule.style.setProperty("color", "green", "");
alert(rule.style.getPropertyValue("color"));

will fail...

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2

This is a first attempt at fixing the problem bz mentioned in comment 19.  It
seems OK, except for the change to inspector.  I'm not sure what to do about
inspector.  Any ideas?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2

Ownership model that's more like the current code, except without the leaks. 
(This avoids GC churn at the cost of leaving "dead" objects accessible to the
DOM, which is the same thing that nsDOMCSSAttrDeclaration does.)

Still has inspector problems.

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 125964 [details] [diff] [review]
patch 2

I forgot to include the nsHTMLEditor.cpp changes in this patch -- I'll include
them once I get the inspector fix.

Comment 23

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2

This includes the inspector fix (a new interface, nsICSSStyleRuleDOMWrapper),
and the editor change that I missed before.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126088 [details] [diff] [review]
patch 2 

So.. we should probably keep the comment in DOMCSSDeclarationImpl about mRule
being a weak pointer maintained by the nsICSSStyleRule.  That comment should
probably move to where mRule is declared.

I'm a little worried about using offsetof() in DomRule() like that... the class
we're calling it on has all sorts of fun things, including vtable pointers,
etc.  I guess we don't want the extra 4 bytes of bloat there, though.  I wish
there were a way we could assert that DomRule() gives us the right pointer, but
I cannot think of a way to do it that does not involve addrefing and releaing
the DOMCSSStyleRuleImpl, which is a bad idea in all those cases...

Add an assert that mRule == nsnull to ~DOMCSSDeclarationImpl() please?	Just to
make sure things get destroyed in the right order...

DOMCSSStyleRuleImpl should have a
NS_INTERFACE_MAP_ENTRY(nsICSSStyleRuleDOMWrapper) line.

In DOMCSSStyleRuleImpl::GetParentRule, shouldn't we call GetDOMRule on the
|rule| instead of CallQueryInterface?  It'd be good not to encourage people to
randomly QI nsICSSRule objects to nsIDOMCSSRule, since they could be badly
bitten by it now...

Why does CSSStyleRuleImpl have the |+  friend class DOMCSSStyleRuleImpl;| line?
 All the methods that DOMCSSStyleRuleImpl calls live on nsICSSStyleRule, no?

The license in nsICSSStyleRuleDOMWrapper.h should say "Mozilla.org code"
instead of the long "_____" thing.  I'm also not sure whether the "Initial
Developer" (copyright holder) is you or Netscape (since I am not familiar with
the terms of your employment agreement or contract).

In inDOMUtils::GetRuleLine, GetCSSStyleRule could return null, so we probably
want a null-check there  (Though I guess we got that rule off a sheet, so it
should have an nsICSSStyleRule still...)

The functions StyleRuleView and StyleRuleView.prototype.getRuleAt in
extensions/inspector/resources/content/viewers/styleRules/styleRules.js rely on
being able to QI nsIStyleRule objects (that's what mRules gets stored in it in
StyleRuleView()) to nsIDOMCSSStyleRule.  I suspect the patch, as is, breaks the
"rules applying to node" view in Inspector.

CSSMediaRuleImpl::GetCssText relies on QIing rules in its mRules array to
nsIDOMCSSRule.	It just wants to call GetCSSText on them, so it's probably ok
to QI to nsICSSStyleRule.

In nsCSSRules, all the GetParentRule functions QI mParentRule to nsIDOMCSSRule.
 In all those cases, mParentRule is always null, so this won't lead to any real
bugs, but they should use GetDOMRule.

CSSStyleSheetImpl::StyleSheetLoaded relies on being able to QI from an
nsIDOMCSSRule to an nsIStyleRule.  This should be calling GetCSSStyleRule on
the DOM rule instead.

nsDOMCSSDeclaration::GetParentRule relies on QIing from what GetParent()
returns to nsIDOMCSSRule.  That's not going to work for decls hanging off of
rules in sheets (since the parent is mRule then).  (As a separate matter,
GetParent() should just be ditched and GetParentRule() implemented by the
subclasses, imo.)

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch 2

I think this addresses all of the comments in comment 24.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 126331 [details] [diff] [review]
patch 2

Looks great.  r+sr=me

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 126331 [details] [diff] [review]
patch 2

Fix checked in to trunk, 2003-06-23 22:38/40 -0700.