1324983 - stylo: we still sometimes fail to clear out ServoData

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

We have a bunch of intermittent test failures that are caused documents being torn down during cycle collection, where those documents have elements that still have ServoData attached to them.  The assertion in ~Element() then makes the current test fail, although the document with the problem is a document from some previous test.

Comment 1

[Cameron McCormack (:heycam)]

layout/generic/crashtests/641724.html is one test that triggers this, and it's due to calling getComputedStyle() with an element that is not in the document.  We compute styles for it, store the ServoData on it, but since we're not in the document we never get a chance to clear it out either in UnbindFromTree (since we were never in the tree) or under ServoRestyleManager::ClearServoDataFromSubtree (again since we won't find it in the tree).

Maybe we should immediately clear out the ServoData after a getComputedStyle() for an element not in the document.

Comment 2

[Bobby Holley (:bholley)]

Yeah, I'm rewriting the getComputedStyle stuff in bug 1324627, so we should be able to fix this on top of that. Setting the dep.

Comment 3

[Cameron McCormack (:heycam)]

Attachment: Bug 1324983 - Don't persist styles on elements not in the document.

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

Comment 4

[Cameron McCormack (:heycam)]

Attachment: Don't persist styles on elements not in the document.

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

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822336 [details]
Bug 1324983 - Don't persist styles on elements not in the document.

https://reviewboard.mozilla.org/r/101284/#review101808

::: dom/xbl/nsXBLService.cpp:481
(Diff revision 1)
> -  AutoStyleNewChildren styleNewChildren(aContent->AsElement());
> +  //
> +  // However, we skip this styling if aContent is not in the document, since we
> +  // should keep such elements unstyled.  (There are some odd cases where we do
> +  // apply bindings to elements not in the document.)
> +  AutoStyleNewChildren styleNewChildren(aContent->AsElement(),
> +                                        aContent->IsInComposedDoc());

nit: It may be worth to just use `Maybe<AutoStyleNewChildren>` instead of the `mDoIt` flag?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822337 [details]
Don't persist styles on elements not in the document.

https://reviewboard.mozilla.org/r/101286/#review101810

::: servo/components/style/traversal.rs:335
(Diff revision 1)
> +    // the styles on the display:none root, but for subtrees not in the document,
> +    // we clear styles all the way up to the root of the disconnected subtree.
> +    let in_doc = element.as_node().is_in_doc();
> +    if !in_doc || display_none_root.is_some() {
>          let mut curr = element;
>          loop {

What about `while curr.has_dirty_descendants() || curr.borrow_data().is_some()`? Seems like we could stop early if that's the case.

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8822337 [details]
Don't persist styles on elements not in the document.

https://reviewboard.mozilla.org/r/101286/#review101810

> What about `while curr.has_dirty_descendants() || curr.borrow_data().is_some()`? Seems like we could stop early if that's the case.

I think that condition is right, but it is redundant with the existing checks for the display:none root (for the existing case) and encountering a null parent (for the new case), and we won't break the loop any earlier than the existing checks.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39824e84c40
Don't persist styles on elements not in the document. r=emilio

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8822337 [details]
Don't persist styles on elements not in the document.

https://reviewboard.mozilla.org/r/101286/#review101810

> I think that condition is right, but it is redundant with the existing checks for the display:none root (for the existing case) and encountering a null parent (for the new case), and we won't break the loop any earlier than the existing checks.

Oh, of course. I only looked at the loop in isolation, and didn't realise we had just resolved style, so all the parent chain would have data.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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