1428246 - The attributeChangedCallback is fired twice for the *first* style attribute change

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Edgar Chen [:edgar]]

(See Bug #1419322 comment #14)

This bug is filed for tracking
- There are two attributeChangedCallback fired for the *first* style attribute change [2].

[2] https://software.hixie.ch/utilities/js/live-dom-viewer/?saved=5663

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Attachment: sane_style_attribute_handling.diff

remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/b10b233a562ca28287e8255d8d5e215bade8906d
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=b10b233a562ca28287e8255d8d5e215bade8906d
remote: recorded changegroup in replication log in 0.107s

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: sane_style_attribute_handling_2.diff

This makes style attribute handling from DOM point of view less weird.
I didn't add BeforeSetAttr call, but that is something to consider.

The basic idea is that the initial GetCSSDeclaration (now called GetOrCreateCSSDeclaration) call doesn't anymore do that magical SetAttr(notify=false) call, but just creates declaration if needed.
Then it is up to the caller to call SetCSSDeclaration, either passing the old or new declaration, depending on whether everything succeeded.

Hopefully I managed to remove all the .inis for now passing tests...
remote: 
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/75e19b83e2f83b1067fb5e9c1eed211e5d1e2f43
remote:   https://hg.mozilla.org/try/rev/75da8c403d214252ef8b3ad1af5484e9a1e2b6e1
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=75da8c403d214252ef8b3ad1af5484e9a1e2b6e1
remote: recorded changegroup in replication log in 0.097s

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8982468 [details] [diff] [review]
sane_style_attribute_handling_2.diff

This could really use a commit message describing what's actually getting changed...

The key part that needs to be documented in the commit message is that now we force cloning of the declaration on set if there are mutation observers, so that we can guarantee that we can produce the old value from GetAttr() even after the declaration has been mutated (because the mutation happens on a clone).  We used to only do this for mutation events, because those needed the old value much later down the line, and we handled mutation observers (and custom element data?) via firing AttributeWillChange before the actual declaration mutations, right?

If I understand this part correctly, have you measured the performance impact of this?   Does introducing a mutation observer somewhere in the document now slow down all inline style sets in that document?  By how much?  Does it do that in other UAs?

It feels like we should get this part sorted out before we worry about the details of the patch....

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

ok, it does regress performance when document has mutation observers.
The tricky thing is that right now too many MutationRecords are created - they are created even if style isn't changed at all.
"[Removing non-existing property or setting invalid value on CSS declaration block shouldn't queue mutation record]"
And also, obviously currently attributeChanged handling is all wrong.

the patch does seem to improve performance when there aren't any kind of mutation observers/listeners.


(Blink is a lot faster when setting style.foo, style.color as an example.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

> ok, it does regress performance when document has mutation observers.

How much?

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Too much,  30% or so. And the speed up without mutation observers is just couple of %.
Stylo seems to be very malloc heavy when dealing with DeclarationBlock's.

Blink is 2.5x faster than current Gecko.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a perf testcase

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: a perf testcase

Fixed a bug in the testcase's withattrmutationobserver part.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: sane_style_attribute_handling_complicated.diff

This is annoyingly complicated, but doesn't regress performance.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

remote: 
remote: View your changes here:
remote:   https://hg.mozilla.org/try/rev/23e8077d491b6fabc7b7b3326795d54fa6676b2e
remote:   https://hg.mozilla.org/try/rev/1e4e33efd9b886f37f69bf5fc75d204c4a3d9972
remote:   https://hg.mozilla.org/try/rev/bfb28f706ecd3efae8d632da4568ebe8cd54f2cb
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfb28f706ecd3efae8d632da4568ebe8cd54f2cb
remote: recorded changegroup in replication log in 0.082s

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: sane_style_attribute_handling_complicated_2.diff

+ missing null check

remote:   https://hg.mozilla.org/try/rev/89e768d85c69ed81541e6d9b8e6967cadb2f05cc
remote:   https://hg.mozilla.org/try/rev/d5b6f0c1a9d0ab87b21e33d5d49e39a6977d0db1
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d5b6f0c1a9d0ab87b21e33d5d49e39a6977d0db1
remote: recorded changegroup in replication log in 0.085s

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8985580 [details] [diff] [review]
sane_style_attribute_handling_complicated_2.diff

So, the idea with this patch is that style code will first call
InlineStyleDeclarationWillChange before style declaration has changed, and SetInlineStyleDeclaration once it has changed.

In order to be able to report old attribute value, InlineStyleDeclarationWillChange reads the value and also calls AttributeWillChange (so that DOMMutationObserser can grab the old value). Later SetInlineStyleDeclaration passes the old value to 
SetAttrAndNotify so that mutation events and attributeChanged callbacks are handled correctly.

Because of performance, declaration can't be cloned for reading the old value. And that is why the recently-added callback is used to detect when declaration is about to change (bug 1466963 and followup bug 1468665).

To keep the expected existing behavior, even if declaration isn't changed, but just a new declaration was created (since there wasn't any), we need to still run all these
willchange/set calls. That is when the code has 'if (created)' checks.

Since there are several declaration implementation and only nsDOMCSSAttributeDeclaration needs the about-to-change callback, GetPropertyChangeClosure is the one to initialize the callback closure, and the struct which is then passes as data to the closure.

Apparently we lost mutation event testing on style attribute when the pref was added, so test_style_attr_listener.html is modified to test both pref values.

This is annoyingly complicated.

One note, currently AttributeWillChange handling is very inconsistent,
https://bugzilla.mozilla.org/show_bug.cgi?id=1468851
The patch has the model I think we should always have - always enter doc update before AttributeWillChange call.

peterv, feel free to forward the review request to someone else (perhaps mrbkap?). This is rather core DOM stuff even though lots of the changes are in layout/style.
(bz is about to have vacation, so not accepting requests)

Comment 15

[Peter Van der Beken [:peterv]]

Comment on attachment 8985580 [details] [diff] [review]
sane_style_attribute_handling_complicated_2.diff

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

This really needs a good checkin comment explaining what is changing (comment 14 is a good start :-)).

::: dom/base/Element.cpp
@@ +2101,5 @@
>  
> +void
> +Element::InlineStyleDeclarationWillChange(MutationClosureData* aData)
> +{
> +  MOZ_ASSERT_UNREACHABLE("Element::SetInlineStyleDeclaration");

s/SetInlineStyleDeclaration/InlineStyleDeclarationWillChange/

::: dom/base/Element.h
@@ +323,5 @@
>    void ClearMappedServoStyle() {
>      mAttrsAndChildren.ClearMappedServoStyle();
>    }
>  
> +  virtual void InlineStyleDeclarationWillChange(MutationClosureData* aData);

Needs documenting.
Looks like aData will always be non-null, so I'd prefer a reference instead of a pointer.

@@ +331,3 @@
>     */
>    virtual nsresult SetInlineStyleDeclaration(DeclarationBlock* aDeclaration,
> +                                             MutationClosureData* aData);

Same thing about the reference.

::: dom/base/nsStyledElement.cpp
@@ +69,5 @@
>  
> +void
> +nsStyledElement::InlineStyleDeclarationWillChange(MutationClosureData* aData)
> +{
> +  MOZ_ASSERT(OwnerDoc()->UpdateNestingLevel(),

I prefer |> 0|.

@@ +96,5 @@
> +        aData->mOldValue->SetTo(oldValueStr);
> +      }
> +    } else {
> +      modification =
> +        !!mAttrsAndChildren.GetAttr(nsGkAtoms::style, kNameSpaceID_None);

HasAttr?

::: layout/style/nsDOMCSSAttrDeclaration.cpp
@@ +74,4 @@
>  {
>    NS_ASSERTION(mElement, "Must have Element to set the declaration!");
> +
> +  MOZ_ASSERT_IF(!mIsSMILOverride, aClosureData);

This should be documented next to the SetCSSDeclaration's declaration (looks a lot like a secret handshake with nsDOMCSSAttributeDeclaration::SetSMILValue :-/).

@@ +124,5 @@
>    // cannot fail
>    RefPtr<DeclarationBlock> decl = new DeclarationBlock();
> +  // Mark the declaration dirty so that it can be reused by the caller.
> +  // Normally SetDirty is called later in SetCSSDeclaration.
> +  //XXXsmaug DeclarationBlock's dirty and immutable flag handling is odd.

Not sure this last line is helpful.

::: layout/style/nsDOMCSSAttrDeclaration.h
@@ +64,5 @@
> +  {
> +    aClosure->function = MutationClosureFunction;
> +    aClosure->data = aClosureData;
> +    aClosureData->mClosure = MutationClosureFunction;
> +    aClosureData->mElement = mElement;

There needs to be a comment somewhere on why we need both function on DeclarationBlockMutationClosure and mClosure on MutationClosureData, even though they point to the same function. I think it is because we need to detect that MutationClosureFunction hasn't been called yet, and function doesn't allow us to detect that?
Could it work to have a boolean member signifying that InlineStyleDeclarationWillChange still needs to be called, set to true here and set to false in the MutationClosureData constructor and once it has been called? Since mClosure is always only ever set to MutationClosureFunction if non-null, we could then just call mElement->InlineStyleDeclarationWillChange directly instead of calling through mClosure and MutationClosureFunction (in nsDOMCSSAttributeDeclaration::SetCSSDeclaration and nsDOMCSSDeclaration::SetCssText).

::: layout/style/nsDOMCSSDeclaration.cpp
@@ +128,5 @@
>    ParsingEnvironment servoEnv =
>      GetParsingEnvironment(aSubjectPrincipal);
>    if (!servoEnv.mUrlExtraData) {
> +    if (created) {
> +      SetCSSDeclaration(olddecl, &closureData);

This (and others below) could use a comment explaining why we do this.

::: layout/style/nsDOMCSSDeclaration.h
@@ +173,4 @@
>      eOperation_Modify,
>  
> +    // We are calling GetOrCreateCSSDeclaration so that we can remove a property
> +    // from it. Does not allocates a new declaration if we don't have one yet;

s/allocates/allocate/

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to Peter Van der Beken [:peterv] from comment #15)
> There needs to be a comment somewhere on why we need both function on
> DeclarationBlockMutationClosure and mClosure on MutationClosureData, even
> though they point to the same function. I think it is because we need to
> detect that MutationClosureFunction hasn't been called yet, and function
> doesn't allow us to detect that?
> Could it work to have a boolean member signifying that
> InlineStyleDeclarationWillChange still needs to be called, set to true here
> and set to false in the MutationClosureData constructor and once it has been
> called? Since mClosure is always only ever set to MutationClosureFunction if
> non-null, we could then just call mElement->InlineStyleDeclarationWillChange
> directly instead of calling through mClosure and MutationClosureFunction (in
> nsDOMCSSAttributeDeclaration::SetCSSDeclaration and
> nsDOMCSSDeclaration::SetCssText).


problem is that SetCSSText doesn't have an element to play with.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Or I guess one could use mElement from the closure data.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: sane_style_attribute_handling_complicated_2_rebased.diff

remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=d17ede513a5d8ecfc28bdfa5fc6078a3d5c1b15e
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=d17ede513a5d8ecfc28bdfa5fc6078a3d5c1b15e
remote: recorded changegroup in replication log in 0.117s

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c0ffefb34f
The attributeChangedCallback is fired twice for the *first* style attribute change, r=peterv

Comment 20

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/e8c0ffefb34f

Comment 22

[Peter Van der Beken [:peterv]]

(In reply to Olli Pettay [:smaug] from comment #17)
> Or I guess one could use mElement from the closure data.

Yeah, exactly. Seemed more elegant than using that mClosure function pointer to essentially record a boolean state.