1377975 - stylo: debug_assert!(cascade_visited.has_rules(primary_inputs)) in cascade_eager_pseudo

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Hiroyuki Ikezoe (:hiro)]

I just confirmed that bug 1376352 fixed the panic bug (bug 1377197) on http://prestodb.rocks/code/simd/ . But the site causes another panic [1] on debug build.
To reproduce this, you need to apply the patch in bug 1374175, otherwise you will see the crash while loading the site.

STR:
1) Load http://prestodb.rocks/code/simd/
2) After finish the loading, move mouse cursor on some links


#6  0x00007fffeadc61ab in std::panicking::begin_panic<&str> (msg=..., file_line=0x0) at /checkout/src/libstd/panicking.rs:511
#7  0x00007fffeaa5d110 in style::matching::PrivateMatchMethods::cascade_eager_pseudo<style::gecko::wrapper::GeckoElement> (self=<optimized out>, context=0x7fffca0f6380, 
    data=<optimized out>, pseudo=0x7fffca0f5ed6, cascade_visited=<optimized out>) at /home/ikezoe/autoland/servo/components/style/matching.rs:618
#8  0x00007fffeaa5eff6 in style::matching::MatchMethods::cascade_pseudos<style::gecko::wrapper::GeckoElement> (self=<optimized out>, context=0x7fffca0f6380, data=0x7fffca2222e8, 
    cascade_visited=<optimized out>) at /home/ikezoe/autoland/servo/components/style/matching.rs:1647
#9  0x00007fffeaa5d9f7 in style::matching::MatchMethods::cascade_primary_and_pseudos<style::gecko::wrapper::GeckoElement> (self=0x7fffca0f6198, context=0x7fffca0f6380, 
    data=0x7fffca2222e8, important_rules_changed=<optimized out>) at /home/ikezoe/autoland/servo/components/style/matching.rs:1094
#10 0x00007fffeaa69f82 in style::traversal::compute_style<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (_traversal=<optimized out>, 
    traversal_data=<optimized out>, context=<optimized out>, element=..., data=<optimized out>) at /home/ikezoe/autoland/servo/components/style/traversal.rs:876
#11 0x00007fffeaa698a6 in style::traversal::recalc_style_at<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (traversal=<optimized out>, 
    traversal_data=<optimized out>, context=<optimized out>, element=..., data=<optimized out>) at /home/ikezoe/autoland/servo/components/style/traversal.rs:707
#12 0x00007fffeaa51a7c in style::gecko::traversal::{{impl}}::process_preorder (self=<optimized out>, traversal_data=0x7fffca0f70f0, thread_local=0x7fffce0f8bd8, node=...)
    at /home/ikezoe/autoland/servo/components/style/gecko/traversal.rs:46
#13 0x00007fffea8d7d6b in style::parallel::top_down_dom<style::gecko::wrapper::GeckoElement,style::gecko::traversal::RecalcStyleOnly> (nodes=..., recursion_depth=9, 
    traversal_data=..., root=..., scope=<optimized out>, pool=<optimized out>, traversal=<optimized out>, tls=<optimized out>)
    at /home/ikezoe/autoland/servo/components/style/parallel.rs:202

[1] https://hg.mozilla.org/mozilla-central/file/a3b192dc8344/servo/components/style/matching.rs#l615

Comment 1

[Hiroyuki Ikezoe (:hiro)]

I got this panic without the patch for bug 1374175 while testing https://www.zdf.de/comedy/neo-magazin-mit-jan-boehmermann/neo-magazin-royale-mit-jan-boehmermann-vom-22-juni-2017-100.html in bug 1376492.

Comment 2

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I'll look into this.

Comment 3

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Well, this was pretty interesting to track down. :)

It's related to a CSS transition on a link ending, which blows away the visited style (partly because that was deferred for transitions) since it's currently not included in the after change style used when a transition update is needed.

Working on a reduced test case.

Comment 4

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1377975 - Pass through visited style for after change.

When a transition update is needed during restyling, `get_after_change_style` is
called to re-cascade without transition rules, and this style is then used for
primary styling instead of the style just computed.

`get_after_change_style` used to ignore visited styles, so this meant that if a
transition update was needed, visited styles were effectively dropped.

To fix the crash here, we pass through the existing visited styles when
re-cascading as part of `get_after_change_style`.

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

Comment 5

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Attachment: Bug 1377975 - Test cascading pseudos during link transitions.

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

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

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

Comment 7

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8884444 [details]
Bug 1377975 - Pass through visited style for after change.

https://reviewboard.mozilla.org/r/155354/#review160524

Comment 8

[Hiroyuki Ikezoe (:hiro)]

Comment on attachment 8884445 [details]
Bug 1377975 - Test cascading pseudos during link transitions.

https://reviewboard.mozilla.org/r/155356/#review160526

::: layout/style/crashtests/link-transition-before.html:20
(Diff revision 1)
> +</style>
> +<a href="http://www.example.com/">example</a>
> +<script>
> +let a0 = document.querySelectorAll("a")[0];
> +a0.classList.add("start");
> +setTimeout(() => {

The reason we can't use requestAnimationFrame is that visited check is asynchronously processed? Can't we use requestIdleCallback instead of timeout?

::: layout/style/crashtests/link-transition-before.html:23
(Diff revision 1)
> +    a0.classList.add("start");
> +    document.documentElement.removeAttribute("class");

I am wondering if the crash happens after this classList.add() call we need to wait for one more restyling process?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8884444 [details]
Bug 1377975 - Pass through visited style for after change.

https://reviewboard.mozilla.org/r/155354/#review160546

::: servo/components/style/matching.rs:719
(Diff revision 1)
>                            new_values: &mut Arc<ComputedValues>,
>                            important_rules_changed: bool) {
>          use context::{CASCADE_RESULTS, CSS_ANIMATIONS, CSS_TRANSITIONS, EFFECT_PROPERTIES};
>          use context::UpdateAnimationsTasks;
>  
> +        // Bug 868975: These steps should examine and update the visited styles

(just a side note, probably worth waiting until bug 1379505 for working on this one, since I'm refactoring a bunch of this file to make it less error-prone).

Comment 10

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8884445 [details]
Bug 1377975 - Test cascading pseudos during link transitions.

https://reviewboard.mozilla.org/r/155356/#review160526

> The reason we can't use requestAnimationFrame is that visited check is asynchronously processed? Can't we use requestIdleCallback instead of timeout?

With `requestAnimationFrame`, it does crash, but only about 50% of the time, so it seems like there's a race there causing it to be intermittent.  With `requestIdleCallback`, it appears to crash consistently, so I'll change to that.

> I am wondering if the crash happens after this classList.add() call we need to wait for one more restyling process?

That actually seems to make the crash less likely, so I'll leave this as-is.

Comment 11

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8884445 [details]
Bug 1377975 - Test cascading pseudos during link transitions.

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

Comment 12

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=015d79e55c34a8333918c11013ab6dbdbbfb309d

Comment 13

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

https://github.com/servo/servo/pull/17650

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1e41e798465
Test cascading pseudos during link transitions. r=hiro

Comment 15

[Carsten Book [:Tomcat]]

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