1221436 - stop replacing css::StyleRule objects when they have dynamic changes

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

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

In bug 978833 I'm changing the object that implements nsIStyleRule for CSS rules from css::StyleRule to css::Declaration.  This is the object whose pointer identity needs to change if the style data it represents change.

As a followup to that (although I was originally working on doing it in the same bug, but it got a bit complicated), I'd like to stop replacing css::StyleRule objects when the data change dynamically.

The difficult part to that is that we need to change what we store for style attributes from the css::StyleRule to the css::Declaration (since otherwise dynamic modification of style attributes would break when we were using the same css::StyleRule for the style attribute of multiple elements that started off with the same string), which in turn requires that we have a new way to get to the HTMLCSSStyleSheet object in order to use the same css::Declaration object for the same strings.

(This work, in turn, might let us merge DOMCSSStyleRule into css::StyleRule, which I think would fix the issue that was requiring a third base class for the conversion of CSS OM to new bindings in bug 898764 / bug 851892.)

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d0d04c00b37

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fd5e19ce074

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21b5b722266b

Comment 4

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

Attachment: patch 1 - Have a pointer back from the css::Declaration for style attributes to the nsHTMLCSSStyleSheet

This is needed for patch 2, since we need a replacement for the
mechanism to get from rules to the style attribute sheet
(nsHTMLCSSStyleSheet) that works instead for declarations rather than
rules.

This pointer will be merged with the owning rule pointer in patch 4, but
we can't do that until after patch 2.

Comment 5

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

Attachment: patch 2 - For style attributes, only store a css::Declaration instead of a css::StyleRule

Now that Declaration implements nsIStyleRule, we don't need the memory
overhead of storing a StyleRule object for style attributes.

We also need to change this prior to patch 5, because the changes in
patch 5 that will allow rules to change (but declarations not) would
otherwise break due to style attribute object merging done by
nsAttrValue::ParseStyleAttribute.

Comment 6

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

Attachment: patch 3 - Remove Rule::SetHTMLCSSStyleSheet and related code, now unused

This was made unused by patch 2.

Comment 7

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

Attachment: patch 4 - Use the same pointer to store the owning rule and the nsHTMLCSSStyleSheet on css::Declaration, since we never need both

The pointer to nsHTMLCSSStyleSheet was added in patch 1 (which was
needed before patch 2), but it was only patch 2 that created the
invariant that we'd only have one pointer or the other, and never both.
Thus this needs to be done separately, after patch 1.

Comment 8

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

Attachment: patch 5 - Stop generating new style rules in DeclarationChanged, since we no longer need a new style rule for nsIStyleRule identity rules

(I think the change DOMCSSDeclarationImpl::GetCSSDeclaration fixes a
purely theoretical bug that would happen if it were possible to get
ahold of a CSS rule without calling either
CSSStyleSheet::EnsureUniqueInner (which is called by
CSSRuleListImpl::IndexedGetter) or Declaration::SetImmutable (which is
called by rule matching, including that triggered by
inIDomUtils::GetCSSStyleRules).  If that were possible, then mutating
such a declaration would mutate the shared Declaration, and then call
WillDirty which would clone the now-mutated Declaration, leading to the
mutation applying to both sheets that shared it rather than just the one
that should have been modified.)

This is the simplification allowed by bug 978833 patch 11 (and part of
what we would otherwise have needed to duplicate in @page and keyframe
rules; we'd also have needed to duplicate the object split between the
internal object and the DOM-exposed object, but for style rules that's
probably worth keeping for the memory savings).

Comment 9

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

Attachment: patch 6 - Remove mechanism for replacing style rules

This is no longer used, thanks to patch 5.

Comment 10

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

Attachment: patch 7 - Remove ReplaceStyleRule/ReplaceRuleInGroup mechanism

This is no longer used, thanks to patch 5.

Comment 11

[Cameron McCormack (:heycam)]

Comment on attachment 8683117 [details] [diff] [review]
patch 2 - For style attributes, only store a css::Declaration instead of a css::StyleRule

Review of attachment 8683117 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/FragmentOrElement.h
@@ +285,5 @@
>       */
>      nsCOMPtr<nsICSSDeclaration> mSMILOverrideStyle;
>  
>      /**
>       * Holds any SMIL override style rules for this element.

s/rules/declaration/?

::: dom/base/nsAttrValueInlines.h
@@ +19,5 @@
>    typedef nsAttrValue::ValueType ValueType;
>  
>    ValueType mType;
>    // mStringBits points to either nsIAtom* or nsStringBuffer* and is used when
> +  // mType isn't mCSSDeclaration.

eCSSDeclaration

::: dom/base/nsTreeSanitizer.cpp
@@ +1066,5 @@
>  }
>  
>  bool
> +nsTreeSanitizer::SanitizeStyleDeclaration(mozilla::css::Declaration *aDeclaration,
> +                                          nsAutoString &aRuleText)

Nit: "*"s and "&"s.

::: dom/base/nsTreeSanitizer.h
@@ +153,5 @@
>       * Checks a style rule for the presence of the 'binding' CSS property and
>       * removes that property from the rule and reserializes in case the
>       * property was found.
>       *
>       * @param aRule The style rule to check

This needs updating.

@@ +158,5 @@
>       * @param aRuleText the serialized mutated rule if the method returns true
>       * @return true if the rule was modified and false otherwise
>       */
> +    bool SanitizeStyleDeclaration(mozilla::css::Declaration* aDeclaration,
> +                                  nsAutoString &aRuleText);

Nit: move the "&" while you're here.

::: dom/canvas/CanvasRenderingContext2D.cpp
@@ +2138,5 @@
>  static already_AddRefed<Declaration>
>  CreateStyleRule(nsINode* aNode,
>    const nsCSSProperty aProp1, const nsAString& aValue1, bool* aChanged1,
>    const nsCSSProperty aProp2, const nsAString& aValue2, bool* aChanged2,
>    ErrorResult& error)

Remove the ErrorResult arg now that there's no error condition?

::: layout/style/nsCSSParser.cpp
@@ +1658,4 @@
>  
>    ReleaseScanner();
>  
>    // XXX check for low level errors

This comment doesn't add much and should probably just go.

::: layout/style/nsHTMLCSSStyleSheet.h
@@ +14,5 @@
>  #include "mozilla/MemoryReporting.h"
>  
>  #include "nsDataHashtable.h"
>  #include "nsIStyleRuleProcessor.h"
> +#include "nsCSSPseudoElements.h"

Nit: please keep the #includes sorted.

Comment 12

[Cameron McCormack (:heycam)]

Comment on attachment 8683118 [details] [diff] [review]
patch 3 - Remove Rule::SetHTMLCSSStyleSheet and related code, now unused

Review of attachment 8683118 [details] [diff] [review]:
-----------------------------------------------------------------

Yay.

Comment 13

[Cameron McCormack (:heycam)]

Comment on attachment 8683119 [details] [diff] [review]
patch 4 - Use the same pointer to store the owning rule and the nsHTMLCSSStyleSheet on css::Declaration, since we never need both

Review of attachment 8683119 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/Declaration.h
@@ +321,4 @@
>    }
>  
> +  Rule* GetOwningRule() {
> +    if (mContainer.mRaw & 0x1) {

Oh, the low-bit trick remains. :(

We could avoid this by looking up to the Rule and checking its type.  Not sure whether it's better than what we have here.  I guess this low-bit thing is a bit safer.

Comment 14

[Cameron McCormack (:heycam)]

Comment on attachment 8683120 [details] [diff] [review]
patch 5 - Stop generating new style rules in DeclarationChanged, since we no longer need a new style rule for nsIStyleRule identity rules

Review of attachment 8683120 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/style/StyleRule.cpp
@@ +1166,5 @@
>    if (mRule) {
> +    if (aOperation != eOperation_Read) {
> +      RefPtr<CSSStyleSheet> sheet = mRule->GetStyleSheet();
> +      if (sheet) {
> +        sheet->WillDirty();

Normally I would expect WillDirty and DidDirty calls to happen in pairs, but here I think we will call WillDirty once, and then DidDirty zero or more times for a given rule's declaration.  Is that right?  if so, is it worth having differently named methods on CSSStyleSheet to call, which then do the same work that WillDirty/DidDirty do?

@@ +1217,4 @@
>    }
>  
>    if (owningDoc) {
> +    owningDoc->StyleRuleChanged(sheet, mRule, mRule);

Now that we'll never pass two different rules in, should we remove the last argument?  (That might impact addons though?)

::: layout/style/StyleRule.h
@@ +314,5 @@
>    nsCSSSelectorList* Selector() { return mSelector; }
>  
>    Declaration* GetDeclaration() const { return mDeclaration; }
>  
> +  void DeclarationChanged(Declaration* aDecl);

Should we just rename this to SetDeclaration?

Comment 15

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

This morning I had a try push that showed one web-platform-tests failure on Win7 opt from bug 978833 or this bug:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d4db76510299&group_state=expanded
... which I then bisected to being this bug... and then further bisected to... just having disappeared completely:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0acf6c749b61&group_state=expanded

Comment 16

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

(In reply to Cameron McCormack (:heycam) from comment #14)
> Comment on attachment 8683120 [details] [diff] [review]
> patch 5 - Stop generating new style rules in DeclarationChanged, since we no
> longer need a new style rule for nsIStyleRule identity rules
> 
> Normally I would expect WillDirty and DidDirty calls to happen in pairs, but
> here I think we will call WillDirty once, and then DidDirty zero or more
> times for a given rule's declaration.  Is that right?  if so, is it worth
> having differently named methods on CSSStyleSheet to call, which then do the
> same work that WillDirty/DidDirty do?

I think we'll call DidDirty either zero or one time following a WillDirty.  I don't see how it would be called multiple times.  I think that's certainly OK given what the methods do, although I suppose I could document that WillDirty isn't necessarily followed by DidDirty.

I think moving the WillDirty call earlier (as this patch does) is safer; otherwise we'd call EnsureUniqueInner too late.  It just happens to be true that the inner is currently always already unique by the time we get to that point, so we've been ok.

I'd be ok with adding comments to WillDirty or renaming it to WillProbablyDirty or something like that.

> >    if (owningDoc) {
> > +    owningDoc->StyleRuleChanged(sheet, mRule, mRule);
> 
> Now that we'll never pass two different rules in, should we remove the last
> argument?  (That might impact addons though?)

I'd rather use a bigger hammer here and do bug 1221908.

> > +  void DeclarationChanged(Declaration* aDecl);
> 
> Should we just rename this to SetDeclaration?

Yeah, that seems reasonable.

Comment 17

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] ⌚UTC+8 from comment #16)
> I'd be ok with adding comments to WillDirty or renaming it to
> WillProbablyDirty or something like that.

OK, just a comment will do then.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/2894f200ed038736e06851432e01fbb1e3311bce
Bug 1221436 patch 1 - Have a pointer back from the css::Declaration for style attributes to the nsHTMLCSSStyleSheet.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/84c5d7320a5ccc49dff580d78486d29443e25259
Bug 1221436 patch 2 - For style attributes, only store a css::Declaration instead of a css::StyleRule.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/2638966229a846861165805fe550fa332398d895
Bug 1221436 patch 3 - Remove Rule::SetHTMLCSSStyleSheet and related code, now unused.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/b09af10d01282ddbc27a59f0417d62357ff69c84
Bug 1221436 patch 4 - Use the same pointer to store the owning rule and the nsHTMLCSSStyleSheet on css::Declaration, since we never need both.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ffbb9175ac62505f0af2c0365e9325226d7bcc0
Bug 1221436 patch 5 - Stop generating new style rules in DeclarationChanged (and rename it to SetDeclaration), since we no longer need a new style rule for nsIStyleRule identity rules.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/42d1516bf147f50e1ba61602953687cb180945cf
Bug 1221436 patch 6 - Remove mechanism for replacing style rules.  r=heycam

https://hg.mozilla.org/integration/mozilla-inbound/rev/d49ec59732d62c8973a948491d24c5dd6e2f1d12
Bug 1221436 patch 7 - Remove ReplaceStyleRule/ReplaceRuleInGroup mechanism.  r=heycam

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2894f200ed03
https://hg.mozilla.org/mozilla-central/rev/84c5d7320a5c
https://hg.mozilla.org/mozilla-central/rev/2638966229a8
https://hg.mozilla.org/mozilla-central/rev/b09af10d0128
https://hg.mozilla.org/mozilla-central/rev/0ffbb9175ac6
https://hg.mozilla.org/mozilla-central/rev/42d1516bf147
https://hg.mozilla.org/mozilla-central/rev/d49ec59732d6