1462412 - Assertion failure: false (Duplicate display item!), at src/layout/painting/nsDisplayList.cpp:141

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Found with m-c:
BuildID=20180517152849
SourceStamp=24bae072acb09114c367e6b9ffde9261b2ad8a58

Assertion failure: false (Duplicate display item!), at src/layout/painting/nsDisplayList.cpp:141

#0 AssertUniqueItem(nsDisplayItem*) src/layout/painting/nsDisplayList.cpp:141:7
#1 MakeDisplayItem<nsDisplayPerspective, nsIFrame *, nsIFrame *, nsDisplayList *> src/layout/painting/nsDisplayList.h:2058:5
#2 nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) src/layout/generic/nsFrame.cpp:3364
#3 nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) src/layout/generic/nsFrame.cpp:3767:12
#4 nsContainerFrame::BuildDisplayListForNonBlockChildren(nsDisplayListBuilder*, nsDisplayListSet const&, unsigned int) src/layout/generic/nsContainerFrame.cpp:389:5
#5 BuildDisplayListForInline src/layout/generic/nsContainerFrame.h:588:5
#6 nsInlineFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) src/layout/generic/nsInlineFrame.cpp:241
#7 nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) src/layout/generic/nsFrame.cpp:3090:5
#8 nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) src/layout/generic/nsFrame.cpp:3767:12
#9 DisplayLine(nsDisplayListBuilder*, nsRect const&, nsLineList_iterator&, int, int&, nsDisplayListSet const&, nsBlockFrame*, mozilla::css::TextOverflow*, unsigned int) src/layout/generic/nsBlockFrame.cpp:6651:13
#10 nsBlockFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) src/layout/generic/nsBlockFrame.cpp:6746:7
#11 nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) src/layout/generic/nsFrame.cpp:3090:5
#12 nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) src/layout/generic/nsFrame.cpp:3767:12
#13 DisplayLine(nsDisplayListBuilder*, nsRect const&, nsLineList_iterator&, int, int&, nsDisplayListSet const&, nsBlockFrame*, mozilla::css::TextOverflow*, unsigned int) src/layout/generic/nsBlockFrame.cpp:6651:13
#14 nsBlockFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) src/layout/generic/nsBlockFrame.cpp:6746:7
#15 nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) src/layout/generic/nsFrame.cpp:3090:5
#16 nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) src/layout/generic/nsFrame.cpp:3767:12
#17 nsCanvasFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) src/layout/generic/nsCanvasFrame.cpp:561:5
#18 nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) src/layout/generic/nsFrame.cpp:3829:14
#19 mozilla::ScrollFrameHelper::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) src/layout/generic/nsGfxScrollFrame.cpp:3627:15
#20 nsIFrame::BuildDisplayListForChild(nsDisplayListBuilder*, nsIFrame*, nsDisplayListSet const&, unsigned int) src/layout/generic/nsFrame.cpp:3829:14
#21 mozilla::ViewportFrame::BuildDisplayList(nsDisplayListBuilder*, nsDisplayListSet const&) src/layout/generic/ViewportFrame.cpp:66:5
#22 nsIFrame::BuildDisplayListForStackingContext(nsDisplayListBuilder*, nsDisplayList*, bool*) src/layout/generic/nsFrame.cpp:3090:5
#23 nsLayoutUtils::PaintFrame(gfxContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) src/layout/base/nsLayoutUtils.cpp:3708:17
#24 mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) src/layout/base/PresShell.cpp:6312:5
#25 nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) src/view/nsViewManager.cpp:480:19
#26 nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) src/view/nsViewManager.cpp:412:33
#27 nsViewManager::ProcessPendingUpdates() src/view/nsViewManager.cpp:1102:5
#28 nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:2039:11
#29 TickDriver src/layout/base/nsRefreshDriver.cpp:328:13
#30 mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301
#31 mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:320:5
#32 RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:760:5
#33 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:673
#34 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:574:9
#35 mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) src/layout/ipc/VsyncChild.cpp:68:16
#36 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20
#37 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1988:28
#38 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2136:25
#39 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2066:17
#40 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:1912:5
#41 mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:1945:15
#42 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1090:14
#43 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#44 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#45 RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
#46 RunHandler src/ipc/chromium/src/base/message_loop.cc:319
#47 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
#48 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:157:27
#49 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:893:22
#50 RunInternal src/ipc/chromium/src/base/message_loop.cc:326:10
#51 RunHandler src/ipc/chromium/src/base/message_loop.cc:319
#52 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:299
#53 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:719:34
#54 content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#55 main src/browser/app/nsBrowserApp.cpp:282
#56 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#57 _start (firefox+0x420f48)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Excellent, this was the assertion that was added in bug 1459997 to help track down some of the RDL crashes we've been seeing.

Comment 2

[Matt Woodrow (:mattwoodrow)]

This is a mismatch between our definitions of a containing block.

We have the 'perspective' property set on a <span> element, which according to the spec creates a containing block for position:fixed elements (and nsStyleDisplay::HasFixedPosContainingBlockStyleInternal returns true).

We also have perspective on the <body>, and a block inside the <span>, so we hit the block-inside-inline case and get a frame tree that looks like:

nsBlockFrame(body) - perspective
  line
    nsBlockFrame(span, wrapper) - perspective
      nsBlockFrame() - transformed
  line
    nsInlineFrame(span) - perspective
      nsBlockFrame() - transformed

Full frame tree: https://pastebin.mozilla.org/9085741

The spec also says that transformed frames read their perspective value from their containing block. When we construct nsDisplayPerspective for each of the transformed frames, we call nsIFrame::GetContainingBlock, which returns the <body>'s nsBlockFrame (since neither nsInlineFrame, nor the nsBlockFrame wrapper are blocks at all, let alone containing blocks).

The mFrame of the nsDisplayPerspective is set to the containing block, so we get two identical perspective items (one for each of the transformed inner blocks) and hit this assertion.

Normally we avoid this by counting the number of perspective items created, and including the index into the value of GetPerFrameKey() to keep them unique.

This counter gets reset in this case, since we call BuildDisplayListForStackingContext for the block-wrapper and the inline, and these frames both think they are a containing block for perspective, so we start a new count for their subtree.

This mismatch (transformed leaves think the inline isn't a containing block, inline itself thinks it is) causes the bug.

David, any idea what the right behaviour here is? Should we be converting the <span> to a block level element in order to respect the request that it comes a containing block?
I suspect the transform property has the same bug/behaviour, since I can't find any code that would make it into a real block.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Matt Woodrow (:mattwoodrow) from comment #2)
> David, any idea what the right behaviour here is? Should we be converting
> the <span> to a block level element in order to respect the request that it
> comes a containing block?

No.  The containing block for absolute/fixed-positioned elements can be an inline rather than a block.  But that doesn't mean such an inline would be the containing block for anything else; in this case, it's not the containing block for anything, though it *could* be if there were an abspos or fixpos element inside.

> This counter gets reset in this case, since we call
> BuildDisplayListForStackingContext for the block-wrapper and the inline, and
> these frames both think they are a containing block for perspective, so we
> start a new count for their subtree.

It seems like this mechanism for resetting the counter is the problem; it seems to assume that frames within a given subtree have a single containing block.  When relatively positioned inlines (or other inlines that establish a containing block for abspos/fixpos elements, like inlines with perspective) are involved this isn't true, since the containing block for abspos and fixpos descendants is different from the containing block for other things.

(That said, those abspos/fixpos descendants are in a separate child list of the inline so should be easy to distinguish if that would help.)

Comment 4

[Matt Woodrow (:mattwoodrow)]

Interesting, learning a lot about containing blocks!

Does that mean that for a page like:

<body>
  <div style="perspective:10px">
    <div id="a" style="transform:rotateX(45deg); position:absolute"></div>
    <div id="b" style="transform:rotateX(45deg);"></div>
  </div>
</body>

Is the correct behaviour that "a" has perspective applied to the transform (since it's position:absolute, and thus the perspective div is its containing block), and "b" does not (since the body is its containing block, and has no perspective)?

Is it possible that the spec is just wrong here, since I believe the intent is that both of them get perspective?

The relevant spec [1] bits:

4.1.4.3: Let ancestor block be the element that establishes the transformed element’s containing block.
4.4.4.4.3: If ancestor block has a value for perspective which is not none, pre-multiply the ancestor block’s perspective matrix into the transform.
8:  It also establishes a containing block (somewhat similar to position: relative), just like the transform property does.

The current behaviour was implemented in bug 1202029, maybe that was the wrong thing to do.


[1] https://drafts.csswg.org/css-transforms-2

Comment 5

[Matt Woodrow (:mattwoodrow)]

The other thing to consider is that nsInlineFrame returns for IsFrameOfType(eSupportsCSSTransforms), should we not be allowing perspective for inlines either?

Comment 6

[Matt Woodrow (:mattwoodrow)]

Ok, did some more research, and think I understand this a bit better now.

As per [1], transformed elements are supposed to be containing blocks for *all* descendants, and if we can't support doing that, then we must not be a transformable element [2] (the definition of transformable element explicitly excludes non-replaced inlines). Are there exceptions missed by this definition?

The definition of perspective [3] says that it only applies to transformable elements, so I think our bug here is that nsIFrame::HasPerspective() returns true for the <span>/nsInlineFrame, despite it returning false for IsFrameOfType(eSupportsCSSTransforms).

I think what we effectively want, is the nearest frame tree ancestor that could potentially returning true for HasPerspective. I believe this is the intent of the spec referring to "the element that establishes the transformed element’s containing block" [4] and should be the same as nsIFrame::GetContainingBlock (I think, more auditing would be worthwhile), once we restrict perspective to just frames that support transforms.

It's possibly an issue that the logic used to decide if we can support perspective (IsFrameOfType(eSupportsCSSTransforms)) and the logic used to find the 'containing block' ancestor (nsIFrame::GetContainingBlock) are entirely distinct, and we have a bug if they ever diverge. It might be worth trying to figure out spec wording (revisit the suggestion in [4]) that defines at least one of these terms with respect to the other, so that we avoid this.


[1] https://github.com/w3c/csswg-drafts/issues/913
[2] https://drafts.csswg.org/css-transforms-1/#terminology
[3] https://drafts.csswg.org/css-transforms-2/#perspective-property
[4] https://www.w3.org/Bugs/Public/show_bug.cgi?id=16328#c4

Comment 7

[Matt Woodrow (:mattwoodrow)]

Attachment: Correctly ignore the perspective property for frames that aren't transformable

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Based on https://dbaron.org/css/test/2018/perspective-inline it seems like we're already ignoring perspective on inlines -- but maybe not completely?

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8979172 [details] [diff] [review]
Correctly ignore the perspective property for frames that aren't transformable

This seems fine to me, although it would probably be good to add an actual reftest (maybe to layout/reftests/w3c-css/submitted/).

Comment 10

[Matt Woodrow (:mattwoodrow)]

Attachment: Correctly ignore the perspective property for frames that aren't transformable v2

The previous patch wasn't actually sufficient to prevent the stacking context, since we checked the perspective from multiple locations.

This adds a test, and does the same handing as we do for transform.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8979415 [details] [diff] [review]
Correctly ignore the perspective property for frames that aren't transformable v2

> bool
> ScrollFrameHelper::HasPerspective() const
> {
>   const nsStyleDisplay* disp = mOuter->StyleDisplay();
>-  return disp->mChildPerspective.GetUnit() != eStyleUnit_None;
>+  return disp->HasPerspective(mOuter);
> }

Seems like this entire function should just be:
  return mOuter->ChildrenHavePerspective()
and that it should probably be inline as well.

In nsStyleDisplay::HasPerspective:
>+  NS_ASSERTION(aContextFrame->StyleDisplay() == this, "unexpected aContextFrame");
may as well just be MOZ_ASSERT.

in nsStyleStruct.h:
>+   * Returns true when the element has the perspective property
>+   * or a related property, and supports CSS transforms.

Drop "or a related property".


I think the removal from nsStyleDisplay::HasFixedPosContainingBlockStyleInternal means
you also need to add a perspective check to
nsStyleDisplay::IsAbsPosContainingBlock.




That said -- with that these patch seem correct -- but can't you get into the same
assertion situation with 'will-change'?

Comment 12

[Matt Woodrow (:mattwoodrow)]

(In reply to David Baron :dbaron: ⌚UTC-7 from comment #11)
 
> That said -- with that these patch seem correct -- but can't you get into
> the same
> assertion situation with 'will-change'?

We don't support will-change:perspective, but it does seem like we have a similar issue with will-change:transform.

As above, the frame constructor doesn't check for this when constructing an inline, so we should be fine in terms of not-creating a containing block for fixed/absolute.

nsIFrame::IsTransformed checks if the frame is transformable, so we should correctly return false there for an inline/non-transformable element.

nsIFrame::IsStackingContext doesn't though, and only checks for the NS_STYLE_WILL_CHANGE_STACKING_CONTEXT bit (regardless of frame type), so we would create a stacking context for the inline.

nsStyleDisplay::Is{Abs|Fixed}PosContainingBlock() would also return true when they shouldn't, but it doesn't look like this should break much.

Comment 13

[Matt Woodrow (:mattwoodrow)]

Filed bug 1463588 for fixing will-change.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/96c45411c051
Correctly ignore the perspective property for frames that aren't transformable. r=dbaron

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbf710609955
Followup - Fix reference test file to match properly.

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/96c45411c051
https://hg.mozilla.org/mozilla-central/rev/cbf710609955

Comment 17

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance. Would be great to get it into 61.0b8 tomorrow.

Comment 18

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8979415 [details] [diff] [review]
Correctly ignore the perspective property for frames that aren't transformable v2

Approval Request Comment
[Feature/Bug causing the regression]: Long standing bug (since initial implementation of 3d transforms), caught by assertions added for retained-dl.
[User impact if declined]: Incorrect 3d transforms rendering.
[Is this code covered by automated tests?]: Yes, new crashtest and reftest added.
[Has the fix been verified in Nightly?]: By me.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: Low-ish.
[Why is the change risky/not risky?]: Changes a bit of code around our handling of stacking contexts/abs/fix pos containing blocks, but should only affect perspective (rare), copies the code for transform (well tested), and makes us match the spec/webkit/blink (unlikely to get webcompat fallout).
[String changes made/needed]: None.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8979415 [details] [diff] [review]
Correctly ignore the perspective property for frames that aren't transformable v2

RDL fix. Approved for 61.0b8.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4289269f9465