stylo: Assertion failure: ExpectedOwnerForChild(*mOwner) == aParent.mOwner

RESOLVED FIXED in Firefox 57



2 years ago
2 years ago


(Reporter: truber, Assigned: bzbarsky)


(Blocks 3 bugs, {assertion, testcase})

Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 wontfix, firefox57 fixed, firefox58 fixed)


(Whiteboard: [fuzzblocker])


(2 attachments, 2 obsolete attachments)

Posted file testcase.html (obsolete) —
The attached testcase causes an assertion in m-c rev a6a1f5c1d971 with stylo enabled by pref.

Assertion failure: ExpectedOwnerForChild(*mOwner) == aParent.mOwner, at /home/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:109
#0: mozilla::ServoRestyleState::AssertOwner (layout/base/ServoRestyleManager.cpp:105)
#1: mozilla::ServoRestyleState::ProcessMaybeNestedWrapperRestyle (mfbt/Maybe.h:459)
#2: mozilla::ServoRestyleState::ProcessWrapperRestyles (layout/base/ServoRestyleManager.cpp:168)
#3: mozilla::ServoRestyleManager::ProcessPostTraversal (layout/base/ServoRestyleManager.cpp:889)
#4: mozilla::ServoRestyleManager::ProcessPostTraversal (layout/base/ServoRestyleManager.cpp:874)
#5: mozilla::ServoRestyleManager::DoProcessPendingRestyles (layout/base/ServoRestyleManager.cpp:1095)
#6: mozilla::PresShell::DoFlushPendingNotifications (layout/base/PresShell.cpp:4200)
Flags: in-testsuite?
Priority: P3 → P2
I'm looking into it.
Assignee: nobody → bwerth
Can't replicate, though I was able to spot it in a slightly older Nightly. Something has resolved it, apparently.
Closed: 2 years ago
Resolution: --- → WORKSFORME
Chances are, the first hunk of fixed this, but getting the first continuation correctly when getting the parent of an anon box.  In this case, the parent of the anon table wrapping the <tr>.  And we have multiple continuations here because of the bidi bits.
Posted file testcase2.html
Here's another that repros in m-c rev 20170905-3ecda4678c49.
Attachment #8898938 - Attachment is obsolete: true


2 years ago
Blocks: 1172704
Resolution: WORKSFORME → ---
Yes, this testcase reproduces the error for me. I'll investigate.
Here's a bit more information on what's going on:

The assert is firing on a ServoRestyleState with an mOwner of an nsBlockFrame which is an anonymous box. The parent of this frame is an nsColumnSetFrame, and that is what is being returned as the ExpectedOwnerForChild. The aParent ServoRestyleState mOwner however is an nsHTMLScrollFrame.

My understanding of this is poor. I'm taking myself off the bug and needinfo-ing Xidorn (author of the fix for Bug 1390389, likely related).
Assignee: bwerth → nobody
Flags: needinfo?(xidorn+moz)
So with testcase2.html we have a scrollable columnset.  When the assert fires, we have:

  mOwner == :-moz-column-content block inside the columnset (0x7fa2f34f3450)
  aParent.mOwner == scrollframe for the scrollable columnset (0x7fa2f34f2ca0)
  ExpectedOwnerForChild(*mOwner) == :-moz-scrolled-content columnset (0x7fa2f34f3500)

Relevant frametree dump bit:

          HTMLScroll(div)(3)@7fa2f34f2ca0 {0,480,76800,3480} [state=0080020000000010] [content=7fa2f33df820] [sc=7fa2cde85f68]<
            ColumnSet(div)(3)@7fa2f34f3500 {0,0,76080,0} vis-overflow=0,0,76080,2760 scr-overflow=0,0,76080,2760 [state=0080000000400220] [content=7fa2f33df820] [sc=7fa2cde87288:-moz-scrolled-content]<
              Block(div)(3)@7fa2f34f3450 {0,0,3,0} [state=0000100008d00000] [content=7fa2f33df820] [sc=7fa2e7a994a8:-moz-column-content]<
                line 7fa2f34f3808: count=1 state=block,clean,prevmarginclean,not impacted,not wrapped,before:nobr,after:nobr[0x108] {0,0,0,0} <
                  TableWrapper(div)(3)@7fa2f34f3590 {0,0,0,0} [state=0000000000000200] [content=7fa2f33df820] [sc=7fa2e7a9a8e8:-moz-table-wrapper]<
                    Table(div)(3)@7fa2f34f3628 {0,0,0,0} [state=0080000000000000] [content=7fa2f33df820] [sc=7fa2e7a9a7c8:-moz-table]<>

This is all happening from mozilla::ServoRestyleState::ProcessMaybeNestedWrapperRestyle where aParent is the scrollframe.

This all sort of makes sense: we have a table-caption kid inside the scrollable columnset.  So when we process the columnset's DOM kids we queue up the anonymous table wrapping the table caption as a thing that needs recomputation of its style.  We look up the "parent" for that anonymous table in ProcessMaybeNestedWrapperRestyle and find the column-content block.  This does not match aParent, so we try to construct a ServoRestyleState for the column-content block.  But that has the columnset as its ExpectedOwnerForChild, not the scrollframe.

I guess we could try to build up a stack of ServoRestyleStates here somehow.  Or somehow loosen the assertion to allow this case...  It seems like it shouldn't actually matter thatn we never created a ServoRestyleState for the columnset itself in this case, right?
Assignee: nobody → bwerth
Flags: needinfo?(emilio)
Assignee: bwerth → nobody
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #7)
> I guess we could try to build up a stack of ServoRestyleStates here somehow.
> Or somehow loosen the assertion to allow this case...  It seems like it
> shouldn't actually matter thatn we never created a ServoRestyleState for the
> columnset itself in this case, right?

Yeah, I don't think it matters at all in this case, so relaxing the assertion sounds ok.
Flags: needinfo?(emilio)
Emilio, can you relax the assertion then?
Assignee: nobody → emilio
Flags: needinfo?(xidorn+moz) → needinfo?(emilio)
Priority: P2 → P3
Apparently I'm neither the author nor the reviewer of the fix for bug 1390389.
Flags: needinfo?(emilio)
Oh, wait, I was canceling ni? me... Set again for emilio...
Flags: needinfo?(emilio)
Boris said yesterday that he could look into relaxing the assertion. I'd appreciate it since he's looked at the test-case more closely, and has all the anon-box wrapper restyle stuff that I only skimmed.
Flags: needinfo?(emilio)
Assignee: emilio → bzbarsky

Comment 13

2 years ago
fyi, bughunter reproduced on Windows and Linux using <>. I reproduced on Linux with a build from this morning.
Blocks: 1402476
Whiteboard: [fuzzblocker]
Comment on attachment 8914445 [details] [diff] [review]
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor

Review of attachment 8914445 [details] [diff] [review]:

::: layout/base/ServoRestyleManager.cpp
@@ +120,5 @@
> +  // the root of the tree.  We also allow aParent.mOwner to be somewhere up our
> +  // expected owner chain not our immediate owner, which allows us creating long
> +  // chains of ServoRestyleStates in some cases where it's just not worth it.
> +#ifdef DEBUG
> +  if (aParent.mOwner) {

Can we assert that if it isn't the first, there is an anon box involved? Don't worry too much if you think it's not worth it.

::: layout/base/crashtests/1391736.html
@@ +10,5 @@
> +  document.documentElement.appendChild(a)
> + = "scroll"
> + = "calc(-15px)"
> + = "table-caption"
> +  requestIdleCallback(() => {

I think you need to use reftest-wait here, what guarantees that this callback runs?
Attachment #8914445 - Flags: review?(emilio) → review+
Did that; could use another once-over
Attachment #8914450 - Flags: review?(emilio)
Attachment #8914445 - Attachment is obsolete: true
Attachment #8914450 - Flags: review?(emilio) → review+

Comment 17

2 years ago
Pushed by
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor.  r=emilio
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Please request Beta approval on this when you get a chance.
Flags: needinfo?(bzbarsky)
Flags: in-testsuite?
Flags: in-testsuite+
Comment on attachment 8914450 [details] [diff] [review]
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor

Approval Request Comment
[Feature/Bug causing the regression]: Stylo
[User impact if declined]: None, this is debug-only code.  But this is
   interfering with fuzzing.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.  This patch is on top
   of the patch for bug 1402476 just in terms of simpler merging.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Debug-only code.
[String changes made/needed]: None.
Flags: needinfo?(bzbarsky)
Attachment #8914450 - Flags: approval-mozilla-beta?
Comment on attachment 8914450 [details] [diff] [review]
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor

Stylo related, with automated test (awesome!), Beta57+
Attachment #8914450 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.