Closed Bug 1397983 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed

People

(Reporter: truber, Assigned: emilio)

References

(Blocks 3 open bugs)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

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
Attached file testcase.html
Emilio, can you take this?
Flags: needinfo?(emilio)
Priority: -- → P2
Sure
Assignee: nobody → 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

::: 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
Attachment #8905924 - Flags: review-
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 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 ;)
Attachment #8905924 - Flags: review+
And look at the talos results too.
(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.
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...
Let's see...
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
Flags: needinfo?(emilio) → needinfo?(bugs)
Sorry, which assertion should I be looking at?
Flags: needinfo?(bugs)
The mParent->IsInComposedDoc() assertion in FlattenedTreeIterator::Init.
Flags: needinfo?(bugs)
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 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
Attachment #8905924 - Flags: review+ → review?(bugs)
oh,  the patches are in wrong order.
Flags: needinfo?(bugs)
Comment on attachment 8906294 [details]
Bug 1397983: Rename nsXBLBinding::{Install,Uninstall}AnonymousContent to {Bind,Unbind}AnonymousContent.

https://reviewboard.mozilla.org/r/178020/#review183876
Attachment #8906294 - Flags: review?(bugs) → review+
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?
Attachment #8905924 - Flags: review?(bugs) → review-
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.
Attachment #8906295 - Flags: review?(bugs)
Attachment #8906295 - Attachment is obsolete: true
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.
(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 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
Attachment #8905924 - Flags: review?(bugs) → review+
Pushed by ecoal95@gmail.com:
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
You need to log in before you can comment on or make changes to this bug.