1356712 - Assertion failure: !(f->GetStateBits() & NS_FRAME_PART_OF_IBSPLIT) (anonymous ib-split block shouldn't have border/background)

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P1)

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found while fuzzing mozilla-central rev 20170414-1a1069b27f40.

Assertion failure: !(f->GetStateBits() & NS_FRAME_PART_OF_IBSPLIT) (anonymous ib-split block shouldn't have border/background), at /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:539

ASAN:DEADLYSIGNAL
=================================================================
==14644==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f98f6ee980d bp 0x7ffc654483f0 sp 0x7ffc65448320 T0)
==14644==The signal is caused by a WRITE memory access.
==14644==Hint: address points to the zero page.
    #0 0x7f98f6ee980c in JoinBoxesForBlockAxisSlice(nsIFrame*, nsRect const&) /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:530:5
    #1 0x7f98f6ee8324 in JoinBoxesForSlice(nsIFrame*, nsRect const&, InlineBoxOrder) /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:564:10
    #2 0x7f98f6e628c6 in BoxDecorationRectForBorder(nsIFrame*, nsRect const&, mozilla::Sides, nsStyleBorder const*) /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:584:14
    #3 0x7f98f6e72497 in nsCSSRendering::GetImageLayerClip(nsStyleImageLayers::Layer const&, nsIFrame*, nsStyleBorder const&, nsRect const&, nsRect const&, bool, int, nsCSSRendering::ImageLayerClipState*) /home/worker/workspace/build/src/layout/painting/nsCSSRendering.cpp:2180:5
    #4 0x7f98f6ed7682 in ComputeMaskGeometry(nsSVGIntegrationUtils::PaintFramesParams&) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:8126:7
    #5 0x7f98f6ed8443 in nsDisplayMask::PaintAsLayer(nsDisplayListBuilder*, nsRenderingContext*, mozilla::layers::LayerManager*) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:8376:3
    #6 0x7f98f6e5d67f in mozilla::PaintInactiveLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsDisplayItem*, gfxContext*, nsRenderingContext*) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:3733:41
    #7 0x7f98f6e5cfdf in mozilla::FrameLayerBuilder::PaintItems(nsTArray<mozilla::FrameLayerBuilder::ClippedDisplayItem>&, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, gfxContext*, nsRenderingContext*, nsDisplayListBuilder*, nsPresContext*, mozilla::gfx::IntPointTyped<mozilla::gfx::UnknownUnits> const&, float, float, int) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:6063:7
    #8 0x7f98f6e5ef01 in mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*) /home/worker/workspace/build/src/layout/painting/FrameLayerBuilder.cpp:6252:19
    #9 0x7f98f2cf3c8a in mozilla::layers::ClientPaintedLayer::PaintThebes(nsTArray<mozilla::layers::ReadbackProcessor::Update>*) /home/worker/workspace/build/src/gfx/layers/client/ClientPaintedLayer.cpp:86:5
    #10 0x7f98f2cf4550 in mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) /home/worker/workspace/build/src/gfx/layers/client/ClientPaintedLayer.cpp:140:3
    #11 0x7f98f2d16410 in mozilla::layers::ClientContainerLayer::RenderLayer() /home/worker/workspace/build/src/gfx/layers/client/ClientContainerLayer.h:57:29
    #12 0x7f98f2d16410 in mozilla::layers::ClientContainerLayer::RenderLayer() /home/worker/workspace/build/src/gfx/layers/client/ClientContainerLayer.h:57:29
    #13 0x7f98f2cefe5e in mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) /home/worker/workspace/build/src/gfx/layers/client/ClientLayerManager.cpp:363:13
    #14 0x7f98f2cf081d in mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) /home/worker/workspace/build/src/gfx/layers/client/ClientLayerManager.cpp:421:3
    #15 0x7f98f6ead520 in nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) /home/worker/workspace/build/src/layout/painting/nsDisplayList.cpp:2254:17

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Goes back further than mozregression can bisect, unfortunately. Note that the testcase requires modification for builds predating bug 1300895 (s/column-width/-moz-column-width).

Comment 2

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 3

[Tyson Smith [:tsmith]]

We are hitting this frequently while fuzzing.

Comment 4

[Sylvestre Ledru [:Sylvestre]]

Maire, Jessie, could you please help finding someone to fix the fuzz blocker?
It means that it is preventing us to fuzz some big part of the code because we hit this issue pretty quickly.

Comment 5

[Jessie [:jbonisteel] pls NI]

Mats, could you take a look at this? Thanks

Comment 6

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/rNt4Jwec6Q7lJUYh85_59w/index.html

It will expire in 7 days.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Looking at the pernosco link, I think the assertion is just invalid. We have mask inherited into the ib-split anonymous block, and the mask code does compute the box decoration rect. The ib-split sibling still doesn't have borders or backgrounds of course.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Oh, though I guess if it can be reached for those, the idea is that it should use the GetNextContinuationOrIBSplit* stuff?

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1356712 - Remove invalid assertion. r=mats

While IB-split block wrappers cannot indeed have borders or backgrounds, we
inherit mask from the inline into them1, and the masking code can reach this
code too.

I think just using the block continuations rather than walking inlines is the
right thing to do here.

Comment 10

[Mats Palmgren (inactive)]

Right, I came to the same conclusion and I'm working on adding some WPTs for the actual layout too, so I should probably take this. https://treeherder.mozilla.org/#/jobs?repo=try&revision=911fe70c1dc415577a544a48d2bf12e3eb86dcad

That said, I'm not really sure there is a spec that defines this clearly...

Comment 11

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (for comparing the layout with Chrome)

The spec doesn't really address the ib-split case AFAICT: https://drafts.csswg.org/css-break-3/#joining-boxes
I'm guessing the assumption is that block-in-inline splitting is done per CSS2: https://www.w3.org/TR/CSS2/visuren.html#img-anon-block which seems to suggest the embedded block shouldn't be included in the continuation chain at all, i.e. something like this:

<p><span>...<div></div>...</span></p>

becomes:
<p>
  <p:anon-block>
    <span frag1>...</>
  </p:anon-block>
  <div></div>
  <p:anon-block>
    <span frag2>...</>
  </p:anon-block>
</p>

(I think that would violate our box tree invariants though. We'd need to link the two <p:anon-block>s as continuations.)

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Yeah, that is probably a no-go. Having such a frame structure would also mean that there'd be no proper way to do opacity on the <span> apply to the <div>, which is required per spec (but Chrome/WebKit don't do it, IIRC).

Comment 13

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mats Palmgren (:mats) from comment #10)

Right, I came to the same conclusion and I'm working on adding some WPTs for the actual layout too, so I should probably take this.

Thanks!

Comment 14

[Mats Palmgren (inactive)]

Yeah, the box tree that CSS2 suggests is impractical. I think we want the <div> boxes as children of some <span> box, which is what we have.

no proper way to do opacity on the <span> apply to the <div>, which is required per spec

Do you have a spec/issue link? That strongly suggests that mask-image, filters etc should also be applied on the <div>.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

See all the discussion in bug 660682, but basically https://drafts.csswg.org/css-color/#transparency mentions element + descendants, not boxes.

We apply opacity also to stuff like fixed pos and such:

<!doctype html>
<div style="opacity: 0.5">
  <div style="position: fixed; height: 100px; width: 100px; background: green"></div>
</div>

Comment 16

[Mats Palmgren (inactive)]

Attachment: Testcase #3 (mask-image and filter with position:fixed child)

Yeah, it seems Chrome has no problem applying these on fixed pos child boxes.

Comment 17

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (for comparing the layout with Chrome)

Chrome is at least consistent in NOT applying them to block-in-inline child boxes.
(Whereas Gecko do apply these consistently.)

Comment 18

[Mats Palmgren (inactive)]

The spec language at:
https://drafts.fxtf.org/css-masking-1/#the-mask-image

A computed value of other than none results in the creation of a stacking context [CSS21] the same way that CSS opacity [CSS3COLOR] does for values other than 1.

seems clear that all these properties should be handled the same as opacity, so they should be applied also for the block-in-inline case.

Which leaves my original question unanswered: how are these fragments supposed to be joined together?

I think our current layout is reasonable. Basically, treat the inline fragments as if the blocks don't exist, and the blocks (each block element separately though) as if the inlines don't exist. I'll file a spec issue though, because the spec needs to define this more precisely...

Comment 19

[Mats Palmgren (inactive)]

Attachment: Testcase #2 (for comparing the layout with Chrome)

Comment 20

[Mats Palmgren (inactive)]

Filed https://bugs.chromium.org/p/chromium/issues/detail?id=1023690

Comment 21

[Mats Palmgren (inactive)]

Attachment: Bug 1356712 - Remove bogus assertions. r=emilio

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b30cd570f78d
Remove bogus assertions. r=emilio

Comment 23

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/20219 for changes under testing/web-platform/tests

Comment 24

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 25

[Oana Pop-Rus]

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

Comment 26

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot