1294572 - stylo: Consider skipping eager traversal on subtrees with an XBL binding

Status: RESOLVED FIXED

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

Description

[Bobby Holley (:bholley)]

When we install XBL, we generally shuffling around all of descendants of the bound element, adding anonymous content and moving explicit children to insertion points. This means that we need to re-cascade everything, and so the eager cascading that we do with Servo is wasted work.

I don't know if this is significant in practice, especially with non-chrome documents. But we could probably detect this case and stop traversing children if an element ends up with a non-trivial -moz-binding in its computed values.

Comment 2

[Bobby Holley (:bholley)]

I have a patch for this, which fixes the !el.has_dirty_descendants() assertion we're getting on layout/reftests/bugs/404553-1.html .

However, I think we'll also want a patch to explicitly clear the ElementData any time the XBL insertion parent changes, because it's difficult to handle the restyle case (removing/applying a binding) during the servo traversal.

This in turn means that we'll need to be sure that we re-invoke TraverseNewChildren whenever we add _or_ remove bindings from an Element.

I'll work on this second part now.

Comment 3

[Bobby Holley (:bholley)]

Per bug 1323356, it looks like there's no uninstallation path for bindings aside from LoadBindings, so that should be a sufficient place to perform our re-traversal on the new flattened tree. I'll write up a patch.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 1 - Avoid traversing children of XBL-bound elements during initial styling. v1

MozReview-Commit-ID: JHABvLnMYco

Comment 5

[Bobby Holley (:bholley)]

Attachment: Part 2 - Drop Servo data in SetXBLInsertionParent, and call StyleNewSubtree after all bindings have been removed and applied. v1

MozReview-Commit-ID: Iv7uyq4uQye

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8818437 [details] [diff] [review]
Part 2 - Drop Servo data in SetXBLInsertionParent, and call StyleNewSubtree after all bindings have been removed and applied. v1

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

::: dom/xbl/nsXBLBinding.cpp
@@ +420,5 @@
>        mContent->UnsetAttr(namespaceID, name, false);
>    }
>  
>    // Now that we've finished shuffling the tree around, go ahead and restyle it
>    // since frame construction is about to happen.

I think this comment can be removed too.

::: dom/xbl/nsXBLService.cpp
@@ +414,5 @@
> +public:
> +  AutoStyleNewChildren(Element* aElement) : mElement(aElement) { MOZ_ASSERT(mElement); }
> +  ~AutoStyleNewChildren()
> +  {
> +    nsIPresShell* presShell = mElement->OwnerDoc()->GetShell();

Is it true that loading a binding can cause the pres shell to go away (due to the script that it runs)?  If so, we should null check presShell here.

Comment 8

[Bobby Holley (:bholley)]

Servo PR: https://github.com/servo/servo/pull/14581

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/62ad968c5c5d
Drop Servo data in SetXBLInsertionParent, and call StyleNewSubtree after all bindings have been removed and applied. r=heycam

Comment 10

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/dac601a343ca for crashing pretty much everything on every platform, though oddly only Marionette and Android seem capable of pointing to where on the dolly it hurts, https://treeherder.mozilla.org/logviewer.html#?job_id=40563406&repo=mozilla-inbound / https://treeherder.mozilla.org/logviewer.html#?job_id=40563200&repo=mozilla-inbound

Comment 11

[Bobby Holley (:bholley)]

Doh, I keep forgetting that HasServoData asserts instead of returning false on non-stylo builds (which explains why this didn't bust on my try push above).

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

Comment 12

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #11)
> Doh, I keep forgetting that HasServoData asserts instead of returning false
> on non-stylo builds (which explains why this didn't bust on my try push
> above).
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=51081e419ad537cd4142ee2ce0d3b6afaa7dbfac

Looks green modulo a static analysis failure for a missing 'explicit'. Added that and pushed.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/824c4af81844
Drop Servo data in SetXBLInsertionParent, and call StyleNewSubtree after all bindings have been removed and applied. r=heycam

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/824c4af81844