1397983 - stylo: Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(mServoRestyleRoot, aRoot)

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

The attached testcase causes an assertion in m-c rev 20170907-b4c1ad9565ee

The testcase isn't 100% reliable but works most of the time. Moving the mouse seems to help.

Assertion failure: !mServoRestyleRoot || mServoRestyleRoot == aRoot || nsContentUtils::ContentIsFlattenedTreeDescendantOfForStyle(mServoRestyleRoot, aRoot), at /builds/worker/workspace/build/src/dom/base/nsIDocumentInlines.h:71
#0: nsIDocument::SetServoRestyleRoot, at dom/base/nsIDocumentInlines.h:65
#1: NoteDirtyElement, at dom/base/Element.cpp:4425
#2: style::gecko::wrapper::GeckoElement::maybe_restyle, at servo/components/style/gecko/wrapper.rs:699
#3: core::option::Option<&mut atomic_refcell::AtomicRefMut<style::data::ElementData>>::and_then<&mut atomic_refcell::AtomicRefMut<style::data::ElementData>,&mut style::data::RestyleData,closure>, at
src/libcore/option.rs:605
#4: style::gecko::wrapper::GeckoElement::note_explicit_hints, at servo/components/style/gecko/wrapper.rs:725
#5: geckoservo::glue::Servo_NoteExplicitHints, at servo/ports/geckolib/glue.rs:2974
#6: mozilla::ServoRestyleManager::PostRestyleEvent, at layout/base/ServoRestyleManager.cpp:335
#7: mozilla::EffectCompositor::PostRestyleForAnimation, at dom/animation/EffectCompositor.cpp:340
#8: mozilla::EffectCompositor::PostRestyleForThrottledAnimations, at dom/animation/EffectCompositor.cpp:358
#9: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4185
#10: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4064
#11: nsHideViewer::Run, at layout/base/nsIPresShell.h:562
#12: nsContentUtils::RemoveScriptBlocker, at dom/base/nsContentUtils.cpp:5681
#13: nsAutoScriptBlocker::~nsAutoScriptBlocker, at dom/base/nsContentUtils.h:3421
#14: mozilla::PresShell::DoFlushPendingNotifications, at layout/base/PresShell.cpp:4190
#15: nsRefreshDriver::Tick, at layout/base/nsRefreshDriver.cpp:1921

Comment 1

[Jesse Schwartzentruber (:truber)]

Attachment: testcase.html

Comment 2

[Bobby Holley (:bholley)]

Emilio, can you take this?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Sure

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

In this case we happen to unbind off a runnable that hasn't yet run when the
second call to SetServoRestyleRoot happens.

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

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

https://reviewboard.mozilla.org/r/177718/#review182770

::: dom/base/Element.cpp:1937
(Diff revision 1)
>      // anonymous content that the document is changing.
>      // Unlike XBL, bindings for web components shadow DOM
>      // do not get uninstalled.
>      if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) && !GetShadowRoot()) {
> +      if (nsXBLBinding* binding = GetXBLBinding()) {
> +        if (nsIContent* anonContent = binding->GetAnonymousContent()) {

This gives quite surprising results I think. Doesn't this mean that when XBL's destructors run, you can't get from anonymous content to the bound element ('this') anymore using parentNode links.

Shouldn't we just do the normal unbind from document part

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

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

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

https://reviewboard.mozilla.org/r/177718/#review182770

> This gives quite surprising results I think. Doesn't this mean that when XBL's destructors run, you can't get from anonymous content to the bound element ('this') anymore using parentNode links.
> 
> Shouldn't we just do the normal unbind from document part

Done, let me know if it looks better :)

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

https://reviewboard.mozilla.org/r/177718/#review182796

And again, some good tryserver run would be nice here ;)

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

And look at the talos results too.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] from comment #8)
> Comment on attachment 8905924 [details]
> Bug 1397983: Synchronously unbind XBL anonymous content from UnbindFromTree.
> 
> https://reviewboard.mozilla.org/r/177718/#review182796
> 
> And again, some good tryserver run would be nice here ;)

Yup... This fails a lone crasthest (layout/generic/crashtests/724235.html), so I'll try to investigate it a bit more. I'll push to talos a few times with it meanwhile, to see how it looks.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Well, I got it to avoid trying to iterate from unbound content, and now stylo is green (yay!). But now I get to debug the non-stylo assertion failures...

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Let's see...

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(Whoops, comment above should've included https://treeherder.mozilla.org/#/jobs?repo=try&revision=34ba4468f56b9e5e92d78f16d6fdbfce5cb7293e)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1397983: Rename nsXBLBinding::{Install,Uninstall}AnonymousContent to {Bind,Unbind}AnonymousContent.

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

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1397983: Fix a test that was relying on getting a chrome-only node via activeElement.

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

Comment 16

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

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

Comment 17

[Emilio Cobos Álvarez (:emilio)]

So, I took a look at this, and the reason that crashtest failed is because the element gets rebound to the document, but we don't rebind the XBL NAC.

A nasty hack that got the tree green is avoiding iterating through those.

The (I think) correct fix, which also gets the tree green (caveat below) is the last commit I pushed, which I'd like to ask for feedback.

The only reason this doesn't get the tree green are the IsInComposedDoc assertions, which fail in some devtools tests. However, those assertions _also_ fail on central without my patches.

Question is: Olli, do the asserts in FlattenedChildIterator make sense to you? The assertion failures in [1] and [2] are due to JS API calls by devtools and chrome, not sure if they're exposed to the web (I think not). Are they just misusing the iterators, or should I just remove the assertion, or bypass it from those APIs?

[1]: (w/ patches) https://treeherder.mozilla.org/#/jobs?repo=try&revision=b22ff48b19128d9d7af9cc7e1e50c3259cfae477
[2]: (only mParent assert) https://treeherder.mozilla.org/#/jobs?repo=try&revision=80cd30910c4201c469009082ad75a1f6b44214a3
[3]: (mParent + XBL children assert) https://treeherder.mozilla.org/#/jobs?repo=try&revision=94e83b4cfce6b3769cb4a2c04403e9109714f1d1

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Sorry, which assertion should I be looking at?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

The mParent->IsInComposedDoc() assertion in FlattenedTreeIterator::Init.

Comment 20

[Emilio Cobos Álvarez (:emilio)]

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

Looks good, modulo an addon test which was relying on document.activeElement incorrectly returning a chrome-only access node, given we didn't set those flags in BindToTree before.

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8906294 [details]
Bug 1397983: Rename nsXBLBinding::{Install,Uninstall}AnonymousContent to {Bind,Unbind}AnonymousContent.

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

Comment 22

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

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

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8906295 [details]
Bug 1397983: Fix a test that was relying on getting a chrome-only node via activeElement.

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

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

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

Comment 25

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8906295 [details]
Bug 1397983: Fix a test that was relying on getting a chrome-only node via activeElement.

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

Comment 26

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

Second commit needs re-review, and I can't convince mozreview of it, so doing manually... sigh

Comment 27

[Emilio Cobos Álvarez (:emilio)]

This is green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e6a2d14fc304c42e418956ea475963b31a6072e

Comment 28

[Emilio Cobos Álvarez (:emilio)]

Talos seems pretty much the same, nothing stands out to me, but let me know if you see something worrying.

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=4db19aaa8f642c54ee14bfabdbfc5f90fcc7c1c2&newProject=try&newRevision=98a42f19deb9c4c04a63602e7bade70943b75c30&framework=1&showOnlyImportant=0

Comment 29

[Olli Pettay [:smaug][[e-mail]]]

oh,  the patches are in wrong order.

Comment 30

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8906294 [details]
Bug 1397983: Rename nsXBLBinding::{Install,Uninstall}AnonymousContent to {Bind,Unbind}AnonymousContent.

https://reviewboard.mozilla.org/r/178020/#review183876

Comment 31

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

https://reviewboard.mozilla.org/r/177718/#review183870

::: dom/base/ChildIterator.cpp:172
(Diff revision 5)
>    nsXBLBinding* binding =
>      mParent->OwnerDoc()->BindingManager()->GetBindingWithContent(mParent);
>  
>    if (binding) {
> -    nsIContent* anon = binding->GetAnonymousContent();
> -    if (anon) {
> +    MOZ_ASSERT(binding->GetAnonymousContent());
> +    mParent = binding->GetAnonymousContent();

Why this change?
But should be fine.

::: dom/base/Element.cpp:1685
(Diff revision 5)
>  
> -        editableDescendantCount += EditableInclusiveDescendantCount(child);
> -      }
> +    if (binding) {
> +      binding->BindAnonymousContent(
> +        binding->GetAnonymousContent(),
> +        this,
> +        binding->PrototypeBinding()->IsChrome());

This doesn't look right. Why are you passing IsChrome here?

Comment 32

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8906295 [details]
Bug 1397983: Fix a test that was relying on getting a chrome-only node via activeElement.

https://reviewboard.mozilla.org/r/178022/#review183882

I'd prefer to review this after the actual patch has been fixed to deal with chrome-only anon elements correctly.

Comment 33

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/177718/diff/5-6/

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

https://reviewboard.mozilla.org/r/177718/#review183870

> Why this change?
> But should be fine.

Just noticed it was not needed at all.

> This doesn't look right. Why are you passing IsChrome here?

Because I'm stupid.

Comment 35

[Emilio Cobos Álvarez (:emilio)]

(In reply to Olli Pettay [:smaug] (Currently reviews only for patches which should land to FF57 [2017-09-21]) from comment #32)
> Comment on attachment 8906295 [details]
> Bug 1397983: Fix a test that was relying on getting a chrome-only node via
> activeElement.
> 
> https://reviewboard.mozilla.org/r/178022/#review183882
> 
> I'd prefer to review this after the actual patch has been fixed to deal with
> chrome-only anon elements correctly.

Of course this is not needed after passing ChromeOnlyContent instead of IsChrome, great catch Olli.

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8905924 [details]
Bug 1397983: Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree.

https://reviewboard.mozilla.org/r/177718/#review183988

Comment 37

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bf6fe2138057890b9cf01c2f6a47e0d9f4cf0af&selectedJob=130435922

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fc635ab83267
Rename nsXBLBinding::{Install,Uninstall}AnonymousContent to {Bind,Unbind}AnonymousContent. r=smaug
https://hg.mozilla.org/integration/autoland/rev/d928c61a691f
Synchronously bind/unbind XBL anonymous content from the bound content's Bind/UnbindFromTree. r=smaug

Comment 39

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/fc635ab83267
https://hg.mozilla.org/mozilla-central/rev/d928c61a691f