1439395 - Consider clearing the Servo style data only when the DOM is on a consistent state.

Status: RESOLVED FIXED

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

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439395: Clear Servo data only when the DOM is in a consistent state.

We used to do it this way effectively until I fixed it in bug 1400936.

Per the list of fuzz bugs that bug has in the "Depends on" field, some of those
without a super-clear fix, and others that aren't listed in there, and all the
complexity we had to deal with while receiving restyle requests mid-unbind, etc,
I think this is the right call.

This clears data on RestyleManager::ContentRemoved for non-anonymous nodes, and
on UnbindFromTree for non-anonymous nodes.

This will hopefully yield enforceable invariants.

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

Comment 2

[Bobby Holley (:bholley)]

Comment on attachment 8952181 [details]
Bug 1439395: Clear Servo data only when the DOM is in a consistent state.

https://reviewboard.mozilla.org/r/221424/#review227274

So I guess the major downside here is that we have to re-traverse the subtree, right? Hopefully that's not such a big deal - the perf was enough of an issue to motivate bug 1378005, but I think this shouldn't regress that bug.

::: commit-message-a7917:11
(Diff revision 1)
> +without a super-clear fix, and others that aren't listed in there, and all the
> +complexity we had to deal with while receiving restyle requests mid-unbind, etc,
> +I think this is the right call.
> +
> +This clears data on RestyleManager::ContentRemoved for non-anonymous nodes, and
> +on UnbindFromTree for non-anonymous nodes.

I think you mean "anonymous nodes" here?

I think the comment is a bit inaccurate though, since ClearServoDataFromSubtree uses a StyleChildrenIterator, which traverses anonymous nodes. Seems like we really mean "subtrees rooted at an anonymous node"?

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3ab4e934cd7f
Clear Servo data only when the DOM is in a consistent state. r=bholley

Comment 4

[Andreea Pavel [:apavel]]

Backed out for failing crashtest at tests/layout/style/crashtests/1400936-2.html

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=3ab4e934cd7f77664d37ff9061c37843597cb0c4

Failure log for android failures: https://treeherder.mozilla.org/logviewer.html#?job_id=163214971&repo=autoland&lineNumber=1781
Failure log for windows failure: https://treeherder.mozilla.org/logviewer.html#?job_id=163222321&repo=autoland&lineNumber=8790
Failure log for linux failure: https://treeherder.mozilla.org/logviewer.html#?job_id=163213816&repo=autoland&lineNumber=95672

Backout: https://hg.mozilla.org/integration/autoland/rev/2abeb62e94dc1d859f2a743aaf5de8a13dd95387

Comment 5

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1439395: Avoid leaving an stale restyle root if there's no servo data on it.

If a single element is inserted in the document, from the lazy frame
construction path we mark it as the restyle root.

It has no restyle data, and we weren't calling ClearServoData when its parent
was being removed from ClearServoDataFromSubtree, thus leaving the stale restyle
root.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8952181 [details]
Bug 1439395: Clear Servo data only when the DOM is in a consistent state.

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

Comment 8

[Bobby Holley (:bholley)]

Comment on attachment 8954094 [details]
Bug 1439395: Avoid leaving an stale restyle root if there's no servo data on it.

https://reviewboard.mozilla.org/r/223248/#review229252

Per IRC discussion, we should add a branch on the existence of mServoData in ClearServoData, and remove any redundant branches in the callers.

::: dom/base/Element.cpp:1930
(Diff revision 1)
>                                      "RemovedFullscreenElement");
>      // Fully exit full-screen.
>      nsIDocument::ExitFullscreenInDocTree(OwnerDoc());
>    }
>  
> +#ifdef DEBUG

This ifdef isn't necessary, since the compiler will optimize it all out. Please remove it to declutter.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8954094 [details]
Bug 1439395: Avoid leaving an stale restyle root if there's no servo data on it.

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

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8954094 [details]
Bug 1439395: Avoid leaving an stale restyle root if there's no servo data on it.

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

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8954094 [details]
Bug 1439395: Avoid leaving an stale restyle root if there's no servo data on it.

Bobby, mind taking a last look to the second patch? Just double-checking you're fine with landing it in that state. The rest of the callers to Clear** look harmless / have the actual intention of checking for the data (specially XBL / nsPresContext::Init / etc.)

Comment 12

[Bobby Holley (:bholley)]

Yeah, sure. Thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/15ba5a05f9ab
Clear Servo data only when the DOM is in a consistent state. r=bholley
https://hg.mozilla.org/integration/autoland/rev/b45ca837446a
Avoid leaving an stale restyle root if there's no servo data on it. r=bholley

Comment 14

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/15ba5a05f9ab
https://hg.mozilla.org/mozilla-central/rev/b45ca837446a