1477847 - Assertion failure: itemClip.GetRoundedRectCount() == 0, at src/layout/painting/FrameLayerBuilder.cpp:5026

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20180723154916
SourceStamp=ff3fab43d24dfdaa8971d92cc4caaf4dc9f54dba

Assertion failure: itemClip.GetRoundedRectCount() == 0, at src/layout/painting/FrameLayerBuilder.cpp:5026

#0 mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) src/layout/painting/FrameLayerBuilder.cpp:5162:9
#1 mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> const*, unsigned int) src/layout/painting/FrameLayerBuilder.cpp:6343:9
#2 nsDisplayFilter::BuildLayer(nsDisplayListBuilder*, mozilla::layers::LayerManager*, mozilla::ContainerLayerParameters const&) src/layout/painting/nsDisplayList.cpp:9571:5
#3 mozilla::FrameLayerBuilder::AddPaintedDisplayItem(mozilla::PaintedLayerData*, mozilla::AssignedDisplayItem&, mozilla::ContainerState&, mozilla::layers::Layer*) src/layout/painting/FrameLayerBuilder.cpp:5439:20
#4 void mozilla::ContainerState::FinishPaintedLayerData<mozilla::PaintedLayerDataNode::PopAllPaintedLayerData()::$_0>(mozilla::PaintedLayerData&, mozilla::PaintedLayerDataNode::PopAllPaintedLayerData()::$_0) src/layout/painting/FrameLayerBuilder.cpp:3618:20
#5 mozilla::PaintedLayerDataNode::PopAllPaintedLayerData() src/layout/painting/FrameLayerBuilder.cpp:3250:23
#6 mozilla::PaintedLayerDataNode::Finish(bool) src/layout/painting/FrameLayerBuilder.cpp:3215:3
#7 mozilla::PaintedLayerDataNode::FinishAllChildren(bool) src/layout/painting/FrameLayerBuilder.cpp:3204:19
#8 mozilla::PaintedLayerDataNode::Finish(bool) src/layout/painting/FrameLayerBuilder.cpp:3213:3
#9 mozilla::PaintedLayerDataTree::Finish() src/layout/painting/FrameLayerBuilder.cpp:3275:12
#10 mozilla::ContainerState::Finish(unsigned int*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, nsDisplayList*) src/layout/painting/FrameLayerBuilder.cpp:5971:25
#11 mozilla::FrameLayerBuilder::BuildContainerLayerFor(nsDisplayListBuilder*, mozilla::layers::LayerManager*, nsIFrame*, nsDisplayItem*, nsDisplayList*, mozilla::ContainerLayerParameters const&, mozilla::gfx::Matrix4x4Typed<mozilla::gfx::UnknownUnits, mozilla::gfx::UnknownUnits> const*, unsigned int) src/layout/painting/FrameLayerBuilder.cpp:6350:9
#12 nsDisplayList::BuildLayers(nsDisplayListBuilder*, mozilla::layers::LayerManager*, unsigned int, bool) src/layout/painting/nsDisplayList.cpp:2525:9
#13 nsDisplayList::PaintRoot(nsDisplayListBuilder*, gfxContext*, unsigned int) src/layout/painting/nsDisplayList.cpp:2736:20
#14 nsLayoutUtils::PaintFrame(gfxContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) src/layout/base/nsLayoutUtils.cpp:3843:12
#15 mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) src/layout/base/PresShell.cpp:6322:5
#16 nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) src/view/nsViewManager.cpp:480:19
#17 nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) src/view/nsViewManager.cpp:412:33
#18 nsViewManager::ProcessPendingUpdates() src/view/nsViewManager.cpp:1102:5
#19 nsRefreshDriver::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:2042:11
#20 mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:299:7
#21 mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:317:5
#22 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:671:16
#23 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NormalPriorityNotify() src/layout/base/nsRefreshDriver.cpp:596:9
#24 mozilla::detail::RunnableMethodImpl<mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver*, void (mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::*)(), true, (mozilla::RunnableKind)0>::Run() src/obj-firefox/dist/include/nsThreadUtils.h:1219:13
#25 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1166:14
#26 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#27 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#28 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#29 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#30 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
#31 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:938:22
#32 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#33 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#34 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#35 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:764:34
#36 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#37 main src/browser/app/nsBrowserApp.cpp:287:18
#38 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#39 _start (firefox+0x423d04)

Comment 1

[Miko Mynttinen]

It seems that this was not related to transforms flattening after all.

mozregression gives the range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=b9c75d921ae0ee6336d727f0ba29117c4e3d3e0f&tochange=a86ac3dafdf5b4fc5e874dce2be0c1ab87a2ec72 for failing this assertion. The most likely change to cause this is bug 1382534.

Previous builds fail another assertion: ###!!! ASSERTION: Bounds computation mismatch: 'mContainerBounds.IsEqualInterior(mAccumulatedChildBounds).

Botond, would you mind taking a look at this?

Comment 2

[Botond Ballo [:botond]]

Sorry, Miko, I didn't get a chance to look at this before going on PTO. It's one of the first things I plan to look at when I'm back.

Comment 3

[Pascal Chevrel:pascalc]

(In reply to Botond Ballo [:botond] [on PTO, back Aug 27] from comment #2)
> Sorry, Miko, I didn't get a chance to look at this before going on PTO. It's
> one of the first things I plan to look at when I'm back.

Matt, Botond indicates in his bugzilla handle that he is on PTO until Aug 27, that is one week before the merge to beta, I'd prefer avoiding uplifts to beta if we can plan differently. Is there somebody in the team that could take this bug while Botond is on PTO and land it during the 63 nightly cycle? Thanks

Comment 4

[Matt Woodrow (:mattwoodrow)]

This is an assertion failure from a fuzzer testcase, we're not aware of any real-world content being negatively affected by it.

Given that, I don't think we need to rush to get it uplifted, so I'm happy to wait for Botond to be back.

Still want it fixed though :)

Comment 5

[Pascal Chevrel:pascalc]

marking as fix-optional given that we are in Beta + low impact.

Comment 6

[Intermittent Failures Robot]

26 failures in 553 pushes (0.047 failures/push) were associated with this bug yesterday.

Repository breakdown:
* mozilla-central: 2
* mozilla-inbound: 22
* autoland: 2

Platform breakdown:
* osx-10-10: 26

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1477847&startday=2018-10-04&endday=2018-10-04&tree=all

Comment 7

[Dorel Luca [:dluca]]

The recent spike in failures was caused by miss classification because of Bug 1495055. It should return to normal.

Comment 8

[Intermittent Failures Robot]

35 failures in 2708 pushes (0.013 failures/push) were associated with this bug in the last 7 days.

** This failure happened more than 30 times this week! Resolving this bug is a high priority. **

** Try to resolve this bug as soon as possible. If unresolved for 2 weeks, the affected test(s) may be disabled. ** 

Repository breakdown:
* autoland: 6
* mozilla-central: 4
* mozilla-inbound: 25

Platform breakdown:
* osx-10-10: 35

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1477847&startday=2018-10-01&endday=2018-10-07&tree=all

Comment 9

[Botond Ballo [:botond]]

Hmm, I can't reproduce this in a recent m-c debug build. Perhaps it was fixed by something else along the way?

Comment 10

[Botond Ballo [:botond]]

I tried to use mozregression to find what might have fixed this, like so:

  mozregression --find-fix -B debug --bad 20180723154916

(the argument for --bad being the build ID from comment 0). However, I could not trigger the assertion even on the supposed "bad" build.

Tyson, what platform were you testing this on? Can you still repro it with a recent nightly or m-c build? Can you repro it on the "bad" build with the above mozregression command?

Comment 11

[Tyson Smith [:tsmith]]

(In reply to Botond Ballo [:botond] from comment #10)

> Tyson, what platform were you testing this on? Can you still repro it with a
> recent nightly or m-c build? Can you repro it on the "bad" build with the
> above mozregression command?

I can reproduce this locally on my Ubuntu 16.04 machine with a debug build:
Version=65.0a1
BuildID=20181025160949
SourceStamp=37d240a1d498bd1662e0f9d6053ee75ccdb90786

Comment 12

[Botond Ballo [:botond]]

Huh. I wonder what is different between our setups.

Could you post the about:support of the build in which you're reproing this please?

Comment 13

[Tyson Smith [:tsmith]]

Attachment: about-support.txt

No sure where the problem could be. Can you reproduce it if you launch the build on it's own and then open the test case?

Comment 14

[Botond Ballo [:botond]]

Ok, I could repro this by doing Ctrl+mousewheel to change the browser zoom a few times.

Comment 15

[Botond Ballo [:botond]]

Attachment: Display list before bug 1382534

Comment 16

[Botond Ballo [:botond]]

Attachment: Display list after bug 1382534

Comment 17

[Botond Ballo [:botond]]

The mask item which triggers the assertion is 0x7f68c0bf9220 in the "after" display list.

It initially ("before optimization") has an empty clip, but a clip chain that includes a clip with a rounded rect, associated with the ASR for the root scroll frame.

However, it eventually ("after optimization") picks up that clip, via a call to FuseClipChainUpTo().

I don't really understand the expectations around FuseClipChainUpTo(). Markus, any insights are appreciated.

Comment 18

[Botond Ballo [:botond]]

Actually... something else is going on here.

Whether we get the problematic display list or not, seems to depend _not_ on bug 1382534, but on whether the scrollable subframe on the page happens to be active. If it's active, we get the assertion failure even without bug 1382534.

Comment 19

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #18)
> Actually... something else is going on here.
> 
> Whether we get the problematic display list or not, seems to depend _not_ on
> bug 1382534, but on whether the scrollable subframe on the page happens to
> be active. If it's active, we get the assertion failure even without bug
> 1382534.

I should clarify that I made this determination by backing out the "core" part of bug 1382534 (the change to the ASR of mask items) locally. Bug 1382534 also included other, "peripheral" changes. It's possible that the regression is actually caused by one of the peripheral changes.

Comment 20

[Botond Ballo [:botond]]

Would be nice to bisect locally to find out which but 1-year-old code is difficult to coax into building. Currently getting rust errors like:

 0:35.57 error: use of deprecated item 'std::ascii::AsciiExt': use inherent methods instead
 0:35.57   --> /home/botond/dev/mozilla/refactoring/servo/components/style/gecko_string_cache/mod.rs:16:5
 0:35.57    |
 0:35.57 16 | use std::ascii::AsciiExt;
 0:35.57    |     ^^^^^^^^^^^^^^^^^^^^
 0:35.58    |

Comment 21

[Emilio Cobos Álvarez (:emilio)]

You can:

 * figure out which rust version we were using, from the taskcluster/ files and downgrade to that, or alternatively:
 * just remove the #[deny(warnings)] in servo/components/style/lib.rs, or alternatively:
 * build with RUSTFLAGS="--cap-lints allow".

Hope it helps :)

Comment 22

[Botond Ballo [:botond]]

Thanks for the suggestions :)

I ended up looking up the Rust version here [1] and downgrading to it - so far, so good.

[1] https://wiki.mozilla.org/Rust_Update_Policy_for_Firefox

Comment 23

[Botond Ballo [:botond]]

The culprit is https://hg.mozilla.org/mozilla-central/rev/98355059c8d1.

Comment 24

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #23)
> The culprit is https://hg.mozilla.org/mozilla-central/rev/98355059c8d1.

This tells us that the issue is with layerizing the mask itself, rather than with the clip we compute for the mask.

That makes this more of a latent issue uncovered by bug 1382534, than a regression caused by it.

Here's an attempt at summarizing the issue, based on a discussion with Markus:
 
  * The assertion is telling us that FrameLayerBuilder doesn't implement
    intersecting a rounded-rect mask with an image mask.

  * WebRender does support this, so there's not much motivation to
    implement this functionality in FrameLayerBuilder.

  * Normally, performing such an intersection doesn't come up in the
    first place, becaue the rounded-rect clip ends up inside or
    outside the mask item, rather than on it. However, in this case
    it comes up because the mask is inside a filter which creates a
    BasicLayerManager.

  * A simple solution is therefore to avoid layerizing a mask if
    we're inside a BasicLayerManager.

Comment 25

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #14)
> Ok, I could repro this by doing Ctrl+mousewheel to change the browser zoom a
> few times.

The reason this helped repro the issue is that the scroll frame for the <div id="a"> has a zero scroll range at some zoom levels. Adding some heights to the testcase to ensure the scroll frame always has a nonzero scroll range makes the assertion happen reliably on a page load, which allows me to turn the test case into a crashtest.

Comment 26

[Botond Ballo [:botond]]

Attachment: Bug 1477847 - Do not layerize mask items if we are inside a BasicLayerManager already. r?mstange

Comment 27

[Botond Ballo [:botond]]

Attachment: Bug 1477847 - Add a crashtest. r?mstange

Depends on D10414

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b52fd8300b0
Do not layerize mask items if we are inside a BasicLayerManager already. r=mstange
https://hg.mozilla.org/integration/autoland/rev/3f06cfca9259
Add a crashtest. r=mstange

Comment 29

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/mozilla-central/rev/0b52fd8300b0
https://hg.mozilla.org/mozilla-central/rev/3f06cfca9259

Comment 30

[Botond Ballo [:botond]]

I won't be uplifting this fix.