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

Status: RESOLVED FIXED

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

Description

[Jesse Schwartzentruber (:truber)]

Attachment: 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)

Comment 1

[Brad Werth [:bradwerth]]

I'm looking into it.

Comment 2

[Brad Werth [:bradwerth]]

Can't replicate, though I was able to spot it in a slightly older Nightly. Something has resolved it, apparently.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

[Jesse Schwartzentruber (:truber)]

Attachment: testcase2.html

Here's another that repros in m-c rev 20170905-3ecda4678c49.

Comment 5

[Brad Werth [:bradwerth]]

Yes, this testcase reproduces the error for me. I'll investigate.

Comment 6

[Brad Werth [:bradwerth]]

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).

Comment 7

[Boris Zbarsky [:bzbarsky]]

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?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(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.

Comment 9

[Bobby Holley (:bholley)]

Emilio, can you relax the assertion then?

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

Apparently I'm neither the author nor the reviewer of the fix for bug 1390389.

Comment 11

[Xidorn Quan [:xidorn] UTC+11]

Oh, wait, I was canceling ni? me... Set again for emilio...

Comment 12

[Emilio Cobos Álvarez (: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.

Comment 13

[Bob Clary [:bc] (inactive)]

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.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Attachment: Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor

Comment 15

[Emilio Cobos Álvarez (: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?

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor

Did that; could use another once-over

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce3e3c2f1598
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor.  r=emilio

Comment 18

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

https://hg.mozilla.org/mozilla-central/rev/ce3e3c2f1598

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 20

[Boris Zbarsky [:bzbarsky]]

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.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8914450 [details] [diff] [review]
Relax the ExpectedOwnerForChild assert in the ServoRestyleState constructor

Stylo related, with automated test (awesome!), Beta57+

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0b73ed645d56