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

RESOLVED FIXED in Firefox 57

Status

()

Core
CSS Parsing and Computation
P3
normal
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: truber, Assigned: bz)

Tracking

(Blocks: 3 bugs, {assertion, testcase})

Trunk
mozilla58
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

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

Details

(Whiteboard: [fuzzblocker])

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 months ago
Created attachment 8898938 [details]
testcase.html

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?
Blocks: 1388625
status-firefox55: --- → unaffected
status-firefox56: --- → wontfix
status-firefox-esr52: --- → unaffected
Priority: -- → P3
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.
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → WORKSFORME
Chances are, the first hunk of https://hg.mozilla.org/mozilla-central/rev/ca0fcfb8cec2 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.
(Reporter)

Comment 4

3 months ago
Created attachment 8904620 [details]
testcase2.html

Here's another that repros in m-c rev 20170905-3ecda4678c49.
Attachment #8898938 - Attachment is obsolete: true
(Reporter)

Updated

3 months ago
Blocks: 1172704
Status: RESOLVED → REOPENED
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

3 months ago
fyi, bughunter reproduced on Windows and Linux using <http://www.indonesia-tourism.com/forum/showthread.php?528-Kadriah-Palace-Istana-Kadriah-West-Kalimantan-Indonesia>. I reproduced on Linux with a build from this morning.
Blocks: 1402476
Whiteboard: [fuzzblocker]
Created attachment 8914445 [details] [diff] [review]
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor
Attachment #8914445 - Flags: review?(emilio)
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)
> +  a.style.overflow = "scroll"
> +  a.style.columnWidth = "calc(-15px)"
> +  b.style.display = "table-caption"
> +  requestIdleCallback(() => {

I think you need to use reftest-wait here, what guarantees that this callback runs?
Attachment #8914445 - Flags: review?(emilio) → review+
Created attachment 8914450 [details] [diff] [review]
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor

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 months ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3e3c2f1598
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor.  r=emilio
https://hg.mozilla.org/mozilla-central/rev/ce3e3c2f1598
Status: REOPENED → RESOLVED
Last Resolved: 3 months ago2 months ago
status-firefox58: --- → fixed
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 21

2 months ago
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+

Comment 22

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0b73ed645d56
status-firefox57: affected → fixed
You need to log in before you can comment on or make changes to this bug.