1461285 - Put the new behavior of setProperty behind pref and only enable it in Nightly

Status: VERIFIED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[wernicke14]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.13; rv:61.0) Gecko/20100101 Firefox/61.0
Build ID: 20180510160705

Steps to reproduce:

Click Login Button at https://developers.kakao.com/ or click Kakao Oauth login button.


Actual results:

The load icon(the thing at favicon position) continues to animate, the screen remains white, and CPU usage rises up to 100%.


Expected results:

The login page should appear but doesn't. And browser response slows down or sometimes freeze.

Comment 1

[wernicke14]

More precisely, there is a problem with https://accounts.kakao.com/
And even if I turn off the page, the CPU usage of Firefox does not decrease for a long time.

Comment 2

[ovidiu boca[:Ovidiu]]

I tested this on Mac OS X 10.13 with FF Nightly 62.0a1(2018-05-16) and I can't reproduce the issue presented in the description.

Can you please try to reproduce this issue in safe mode? here is a link that can help you https://support.mozilla.org/en-US/kb/troubleshoot-firefox-issues-using-safe-mode

Also, it will be a good idea to download Firefox Nightly from here: https://nightly.mozilla.org/ and retest the problem.

Comment 3

[wernicke14]

I tried all the methods you suggested and it did not work with a error message: "A web page is slowing down your browser."

Is there any way to report the situation in more detail?

Comment 4

[ovidiu boca[:Ovidiu]]

Can you please capture a performance profile? You need to use Firefox Nightly. You can get more info on how to install and use the Cleopatra add-on (that helps you get the performance profile) by going to:
https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Profiling_with_the_Built-in_Profiler
https://perf-html.io/

Is this issue reproducible only with the website mentioned in the description, or you can reproduce it with other websites?

Comment 5

[wernicke14]

Here is the profile
https://perfht.ml/2ItxTbC (Recorded in 62.0a1)

There is another website causing the same problem. but I did not notice it because it did not cause problems in a short time like this.

Comment 6

[wernicke14]

I'm sorry but that's why I don't remember the website.

Comment 7

[ovidiu boca[:Ovidiu]]

Thanks, wernicke14 for this profile.

Mike, can you please take a look at the recorded profile from comment 5? Thanks

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Here's the relevant chunk of the profile:

https://perfht.ml/2kef6Tc

It looks like there's some kind of infinite recursion going on? My read on this is that the page is listening for the (deprecated) DOMSubtreeModified event, and then using that event to modify the DOM... which causes another DOMSubtreeModified event to fire, and we endlessly loop (until we hit the stack limit).

I can reproduce this pretty easily by just visiting the login page directly at https://accounts.kakao.com/login?continue=https://developers.kakao.com/login

I think this is more of a Tech Evangelism bug - this website is just busted.

Comment 9

[Xidorn Quan [:xidorn] UTC+10]

It's very likely something regressed by bug 1415330 / servo/servo#20582, and should have been fixed in nightly by bug 1460295.

But given this situation, I'd like to take this bug to back out bug 1415330 from beta (and maybe nightly as well, because otherwise bug 1460295 is not risk-free to switch off).

Comment 13

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event.

Review commit: https://reviewboard.mozilla.org/r/246294/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/246294/

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8980138 [details]
Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event.

https://reviewboard.mozilla.org/r/246294/#review252412

Per IRC discussion, let's back this out from this beta and at least next one so we can easily revert bug 1460295 if needed, but keep it on Nightly at least.

You presumably also need some test changes, but I'm assuming those would be trivial-ish as well.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8980138 [details]
Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/246294/diff/1-2/

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8980138 [details]
Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1415330
[User impact if declined]: some websites may lock the process due to nested event dispatching
[Is this code covered by automated tests?]: the new behavior introduced in the regressing bug has tests, and they are marked FAIL here
[Has the fix been verified in Nightly?]: no, we are not going to land this backout in nightly. only beta for now.
[Needs manual test from QE? If yes, steps to reproduce]: maybe try visiting the websites in all the duplicates of this bug to confirm
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it's a backout
[String changes made/needed]: n/a

Comment 17

[Xidorn Quan [:xidorn] UTC+10]

try push for this with beta: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac3b61873364a81c05620ff737a1056175d6045f

Comment 18

[Maire Reavy [:mreavy]]

[Tracking Requested - why for this release]:Back out to fix a regression in 61.  See comment 16 for details.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8980138 [details]
Bug 1461285 - Backed out 62908a56c59f (bug 1415330) for causing nested DOMSubtreeModified event.

Approved for 61.0b8.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/88c81330970b

Leaving the bug open for now since it sounds like it's not certain what we want to do for 62 yet.

Comment 21

[Xidorn Quan [:xidorn] UTC+10]

The patch being backed out here is a spec-conformance feature change. It is non-trivial to be controlled behind a pref, but we want to at least keep it being baked in Nightly for a while. 

This regression has been fixed in a different way by bug 1460295 in order to ship this feature change eventually, but that approach has its own compat risk, so we want that to get release first to be sure.

Per discussion with emilio, our current plan is to uplift this backout patch to every beta until bug 1460295 gets to release, and then we get the feature change ride the train. It means 62, and probably 63 would need this backout when they get to beta.

Comment 22

[ovidiu boca[:Ovidiu]]

We verified this issue on Mac OS X 10.13 with the latest Firefox beta 61.0b9 and the issue is not reproducible. 
We also verified the bugs that are marked as a duplicate after this one and we manage to reproduce this issue with Firefox 61.0b5 and on Firefox 61.0b9 the issue is fixed.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Marking this as a blocker, Xidorn, can you plan to do the backout for 62 before the first merge (before June 14)? Thanks.

Comment 24

[Liz Henry (:lizzard) (relman/hg->git project)]

From discussion on IRC, it sounds like this doesn't block 62.0b1, and we will land a fix next week.

Comment 25

[Xidorn Quan [:xidorn] UTC+10]

It seems due to changes in bug 1466963, the backout can no longer be applied cleanly in beta 62. We would need to create a separate patch to reverse the behavior.

Comment 26

[Liz Henry (:lizzard) (relman/hg->git project)]

Xidorn, are you still working on this? I'll email to follow up.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461285 part 1 - Rename DeclarationSource to DeclarationPushMode.

Review commit: https://reviewboard.mozilla.org/r/255882/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255882/

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461285 part 2 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Review commit: https://reviewboard.mozilla.org/r/255884/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/255884/

Comment 29

[Xidorn Quan [:xidorn] UTC+10]

Hold on... it seems the update path is probably wrong :/

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8990880 [details]
Bug 1461285 part 2 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255884/diff/1-2/

Comment 31

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8990879 [details]
Bug 1461285 part 1 - Rename DeclarationSource to DeclarationPushMode.

https://reviewboard.mozilla.org/r/255882/#review262760

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8990880 [details]
Bug 1461285 part 2 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

https://reviewboard.mozilla.org/r/255884/#review262762

::: modules/libpref/init/StaticPrefList.h:281
(Diff revision 2)
>  #undef PREF_VALUE
>  
> +#ifdef RELEASE_OR_BETA
> +# define PREF_VALUE false
> +#else
> +# define PREF_VALUE true

Please add a comment describing this and pointing to this bug maybe?

::: servo/components/style/properties/declaration_block.rs:424
(Diff revision 2)
>      }
>  
> +    /// Returns whether the property is definitely new for this declaration
> +    /// block. It returns true when the declaration is a non-custom longhand
> +    /// and it doesn't exist in the block, and returns false otherwise.
> +    fn is_definitely_new(&self, decl: &PropertyDeclaration) -> bool {

Probably #[inline] it? Not a big deal if not I guess.

::: servo/components/style/properties/declaration_block.rs:434
(Diff revision 2)
> +    }
> +
> +    /// Checks wether extending the declaration block with the given property
> +    /// declarations would cause change.
> +    #[inline]
> +    pub fn check_extend(

Given this only has one caller, what about making this something like:

```rust
/// Returns whether the value and importance for a given declaration is the same as the arguments.
pub fn declaration_and_importance_equal(
    &self,
    decl: &PropertyDeclaration,
    importance: Importance,
) -> bool {
    // The code after PushMode::Update
}
```

And then do, in glue.rs:

```
let mode = if pref_is_set {
    PushMode::Append
} else {
    let will_change = read_...(|block| block.declaration_and_importance_equal(value, importance));
    if !will_change {
        return false;
    }
    PushMode::Update
};

// extend
```

That would avoid the unreachable and make it more obvious in the caller I think.

Comment 33

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8990880 [details]
Bug 1461285 part 2 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255884/diff/2-3/

Comment 34

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8990880 [details]
Bug 1461285 part 2 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/255884/diff/3-4/

Comment 35

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461285 part 2 - Add a declaration iterator to AllShorthand for simplify code.

Review commit: https://reviewboard.mozilla.org/r/256132/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256132/

Comment 36

[Xidorn Quan [:xidorn] UTC+10]

Attachment: Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Review commit: https://reviewboard.mozilla.org/r/256134/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256134/

Comment 37

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8991162 [details]
Bug 1461285 part 2 - Add a declaration iterator to AllShorthand for simplify code.

https://reviewboard.mozilla.org/r/256132/#review263014

::: servo/components/style/properties/properties.mako.rs:2227
(Diff revision 1)
>      WithVariables(Arc<UnparsedValue>)
>  }
>  
> +impl AllShorthand {
> +    /// Iterates property declarations from the given all shorthand value.
> +    fn declarations(&self) -> AllShorthandDeclarationIterator {

nit: #[inline], probably?

::: servo/components/style/properties/properties.mako.rs:2245
(Diff revision 1)
> +impl<'a> Iterator for AllShorthandDeclarationIterator<'a> {
> +    type Item = PropertyDeclaration;
> +
> +    #[inline]
> +    fn next(&mut self) -> Option<Self::Item> {
> +        let id = self.longhands.next()?;

I think it's weird to advance this iterator even in the case where it's `NotSet`, I'd move this into the other two branches.

Comment 38

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8991163 [details]
Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

https://reviewboard.mozilla.org/r/256134/#review263016

::: servo/components/style/properties/declaration_block.rs:453
(Diff revision 1)
> +                .map_or(true, |(i, slot)| {
> +                    let important = self.declarations_importance[i];
> +                    *slot != *decl || important != importance.important()
> +                })
> +        };
> +        source_declarations.declarations.iter().any(needs_update) ||

nit: Maybe you wouldn't need the closure if you did something like:

```
source_declarations.declarations.iter().chain(source_declarations.all_shorthand.declarations()).any(|...| {
})
```

::: servo/ports/geckolib/glue.rs:3588
(Diff revision 1)
>          return false;
>      }
>  
> +    let importance = if is_important { Importance::Important } else { Importance::Normal };
> +    let mode = if unsafe {
> +        structs::StaticPrefs_sVarCache_layout_css_property_append_only

nit: maybe move this to a different variable? Not a big deal but the unsafe { wrapping looks a bit awkward.

Comment 39

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8991163 [details]
Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

https://reviewboard.mozilla.org/r/256134/#review263016

> nit: Maybe you wouldn't need the closure if you did something like:
> 
> ```
> source_declarations.declarations.iter().chain(source_declarations.all_shorthand.declarations()).any(|...| {
> })
> ```

It's not going work because `all_shorthand.declarations()` iterates owned `PropertyDeclaration` while the `declarations.iter()` iterates reference of `PropertyDeclaration`.

Comment 40

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8991162 [details]
Bug 1461285 part 2 - Add a declaration iterator to AllShorthand for simplify code.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256132/diff/1-2/

Comment 41

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8991163 [details]
Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/256134/diff/1-2/

Comment 42

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d43e8d23c2d2
part 1 - Rename DeclarationSource to DeclarationPushMode. r=emilio
https://hg.mozilla.org/integration/autoland/rev/d7a3f2b213a7
part 2 - Add a declaration iterator to AllShorthand for simplify code. r=emilio
https://hg.mozilla.org/integration/autoland/rev/ad6617403345
part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly. r=emilio

Comment 43

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8991163 [details]
Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1415330
[User impact if declined]: see some performance regression and may get into stuck if we revert the change in bug 1460295 (because of its own compatibility risk)
[Is this code covered by automated tests?]: there is a wpt for the new behavior, but this patch is for ensuring our old behavior so probably not really.
[Has the fix been verified in Nightly?]: it is irrelevant, because in this patch nightly and beta use two different code path
[Needs manual test from QE? If yes, steps to reproduce]: it would probably be good. Steps:
1. set layout.css.property-append-only to false
2. set dom.mutation-events.cssom.disabled to false
3. try steps in this bug or in bug 1460295 and see whether they reproduce
[List of other uplifts needed for the feature/fix]: all patches in this bug
[Is the change risky?]: not very risky
[Why is the change risky/not risky?]: common usage of the changed API should have been well checked by our test suite
[String changes made/needed]: n/a

Comment 44

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Xidorn Quan [:xidorn] UTC+10 from comment #43)
> [Is this code covered by automated tests?]: there is a wpt for the new
> behavior, but this patch is for ensuring our old behavior so probably not
> really.

This patch is for getting our old behavior back for beta and release. That behavior can be faster but doesn't match the spec.

Comment 45

[Xidorn Quan [:xidorn] UTC+10]

try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=726939b5b9357b5f126c062383fa4052f21dc21a

try push with beta:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3227733ea2776544c228003218c2ffbf6d336c8e

Comment 46

[ovidiu boca[:Ovidiu]]

I tested this issue on Mac OS X 10.12 and 10.13 using the builds from comment 45 and I wasn't able ro reproduce it. I used the steps from  bug 1460295 and from comment 1. 

If you consider that this issue needs to be tested on other OSes please need info me.

Comment 47

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/d43e8d23c2d2
https://hg.mozilla.org/mozilla-central/rev/d7a3f2b213a7
https://hg.mozilla.org/mozilla-central/rev/ad6617403345

Comment 48

[Kestrel]

Bug 1463791 is now working correctly on Nightly 20180711100118 on Ubuntu 18.04.

Comment 49

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8991163 [details]
Bug 1461285 part 3 - Have the CSSOM appending behavior behind a pref and only enable it on Nightly.

Fix for a perf regression, this is instead of the backout done for 61. 
Verified with test builds; let's uplift for beta 8.

Comment 50

[Xidorn Quan [:xidorn] UTC+10]

Changing the summary to what is actually done in the bug.

Comment 51

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/23ef66f7d81b
https://hg.mozilla.org/releases/mozilla-beta/rev/f43535236299
https://hg.mozilla.org/releases/mozilla-beta/rev/c45e817cdf36

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/905b78afb7da
followup - Remove a unused line to fix a warning.

Comment 53

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/releases/mozilla-beta/rev/02069dab4f3c

Comment 54

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/905b78afb7da

Comment 55

[ovidiu boca[:Ovidiu]]

I verified this issue on Mac OS X 10.12 and Windows 10 with FF Nightly 63.0a1(2018-07-18) and FF beta 62.0b9 following the steps from comment 43 and comment 1 and the issue is not reproducible.
Based on this I will mark this as verified as fixed.