1462742 - Crash in AssertUniqueItem on devices with touch screens

Status: VERIFIED FIXED

(Core :: Web Painting, defect)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-ad6efb82-a644-4f3b-908d-06ae10180518.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll AssertUniqueItem layout/painting/nsDisplayList.cpp:141
1 xul.dll mozilla::SVGGeometryFrame::BuildDisplayList layout/svg/SVGGeometryFrame.cpp:262
2 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:3821
3 xul.dll nsContainerFrame::BuildDisplayListForNonBlockChildren layout/generic/nsContainerFrame.cpp:389
4 xul.dll nsSVGDisplayContainerFrame::BuildDisplayList layout/svg/nsSVGContainerFrame.cpp:154
5 xul.dll nsIFrame::BuildDisplayListForStackingContext layout/generic/nsFrame.cpp:3082
6 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:3759
7 xul.dll nsContainerFrame::BuildDisplayListForNonBlockChildren layout/generic/nsContainerFrame.cpp:389
8 xul.dll nsSVGDisplayContainerFrame::BuildDisplayList layout/svg/nsSVGContainerFrame.cpp:154
9 xul.dll nsIFrame::BuildDisplayListForChild layout/generic/nsFrame.cpp:3821

=============================================================

There is 1 crash in nightly 62 with buildid 20180518100150. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1459997.

[1] https://hg.mozilla.org/mozilla-central/rev?node=488b7be0348b

Comment 1

[Ryan VanderMeulen [:RyanVM]]

MOZ_RELEASE_ASSERT(false) (Duplicate display item!)

Comment 2

[Markus Stange [:mstange]]

Bwinton is able to reliably reproduce this crash when using the devtools inspector to inspect an SVG document. As soon as he hovers the inspector's DOM tree (which attempts to show a highlighter), the content process crashes.

I cannot reproduce this crash by doing that. The highlighters never show for me on SVG documents, and the existence of bug 1157592 seems to indicate that this is expected. So I don't know why things are different for bwinton.

Brad, since you have worked on bug 1157592 before, do you know how it could happen that we build the display list items for the canvas anonymous content? Do you know if it's possible that we visit the same frame twice during display list construction? Some of the comments in that bug say that we sometimes put the frame for the anonymous content in the wrong place in the frame tree. Is it possible that display list building traverses the frame tree during normal child list traversal once, and then again when building the display list for the "top layer" in https://searchfox.org/mozilla-central/rev/eb51d734a8c06ac5973291be9bcc74ed4e999bca/layout/generic/ViewportFrame.cpp#187 ?

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Blake, can you still reproduce this reliably?

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Yup. Did someone do something that they think fixed it, or were you just double-checking? ;)

Comment 5

[(no longer active)]

We determined that in order for this crash to be reproduced, the device must have a touch screen.

Comment 6

[Markus Stange [:mstange]]

Alternatively, you can set the pref layout.accessiblecaret.enabled to true in order to reproduce the crash.

The accessible caret causes us to always create anonymous content for the canvas frame. So if we then want to insert the anonymous content for the highlighter, there's already anonymous content present, and bug 1157592 comment 4 comes in:

(In reply to Boris Zbarsky [:bz] from bug 1157592 comment #4)
> And incidentally, it's a bit weird that we construct the frame differently
> depending on whether we already have anonymous content when we're
> constructing the canvas or not.  If we already have anonymous content when
> we construct the canvas, then we put the placeholder for the anon div as a
> child of the CanvasFrame directly.  But if we add the anon content after the
> canvas has been constructed already, the placeholder ends up as a child of
> the document element's frame, because we take the normal content-insertion
> codepath.

Comment 7

[Markus Stange [:mstange]]

Attachment: Bug 1462742 - Don't build display items for the custom content container frame twice during the same paint.

Review commit: https://reviewboard.mozilla.org/r/250464/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/250464/

Comment 8

[Markus Stange [:mstange]]

I found a workaround. I wrote a very confident comment but I'm not entirely sure everything in it is accurate.

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8984591 [details]
Bug 1462742 - Don't build display items for the custom content container frame twice during the same paint.

https://reviewboard.mozilla.org/r/250464/#review256870

::: layout/generic/ViewportFrame.cpp:195
(Diff revision 1)
> +        // list. That's a bug in the way we construct frames for the custom
> +        // content container, to be fixed in bug 1157592.
> +        // The ContainsFrame check is going to be cheap because the canvas
> +        // frame usually only has a single principal child: the primary frame
> +        // of the document's root element.
> +        if (!canvasFrame->PrincipalChildList().ContainsFrame(frame)) {

So I took a deeper look at this and the underlying issue here is that this frame is not out of flow. The underlying reason for that is that the `-moz-top-layer: top` declaration in:

https://searchfox.org/mozilla-central/rev/e6a0a192ea8691f7eb466506301da73fabe8fd23/layout/style/res/ua.css#481

Which would force absolute position in:

  https://searchfox.org/mozilla-central/rev/e6a0a192ea8691f7eb466506301da73fabe8fd23/servo/components/style/style_adjuster.rs#120

Is not getting applied (or is getting undone by something else).

I need to find why. This looks like an acceptable workaround (probably just checking `frame->StyleDisplay()->mTopLayer != NS_STYLE_TOP_LAYER_TOP` is the right condition here, pointing to that bug.

Mind if I try to figure out out why before stamping such thing?

Comment 10

[Markus Stange [:mstange]]

Thanks for taking such a detailed look!

My goal was to try to fix this crash for Beta, which turns into RC on Monday after the all hands. I'm not sure if people are going to be available for approvals during the all hands, so it's possible that I've already missed my window. So I'm not sure which trade-off between "finding the right fix" and "stopping the crash quickly" we should be going for.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8984591 [details]
Bug 1462742 - Don't build display items for the custom content container frame twice during the same paint.

https://reviewboard.mozilla.org/r/250464/#review256880

Ok, let me write a patch that fixes the underlying issue and I think is upliftable. If we deem it not upliftable or not safe enough, we can just check `mTopLayer` there instead of `PrincipalChildList()`.

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8984591 [details]
Bug 1462742 - Don't build display items for the custom content container frame twice during the same paint.

https://reviewboard.mozilla.org/r/250464/#review256882

::: layout/generic/ViewportFrame.cpp:195
(Diff revision 1)
> +        // list. That's a bug in the way we construct frames for the custom
> +        // content container, to be fixed in bug 1157592.
> +        // The ContainsFrame check is going to be cheap because the canvas
> +        // frame usually only has a single principal child: the primary frame
> +        // of the document's root element.
> +        if (!canvasFrame->PrincipalChildList().ContainsFrame(frame)) {

Actually, if we plan to uplift, it's better to just land the check.

Please change this condition to:

```
if (frame->StyleDisplay()->mTopLayer != NS_STYLE_TOP_LAYER_TOP)
```

And mention in the comment that the root cause for this happening and bug 1157592 is because SVG documents don't load `ua.css` when we create the custom content container, which contains the rule that makes this a top layer frame.

r=me with that.

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Err, comment 12 is supposed to say:

  if (frame->StyleDisplay()->mTopLayer != NS_STYLE_TOP_LAYER_NONE)

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Bug 1157592 contains the right fix now.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Is this good to land? Would be nice if we could get it landed in time for Thursday's b14 build if possible.

Comment 16

[Markus Stange [:mstange]]

I'm applying Emilio's comments at the moment and will land on autoland shortly afterwards.

Comment 17

[Markus Stange [:mstange]]

Comment on attachment 8984591 [details]
Bug 1462742 - Don't build display items for the custom content container frame twice during the same paint.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/250464/diff/1-2/

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6bcd91f7f50a
Don't build display items for the custom content container frame twice during the same paint. r=emilio

Comment 19

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/6bcd91f7f50a

Comment 20

[(no longer active)]

Verified that the crash is now gone in the latest Nightly.

Comment 21

[Markus Stange [:mstange]]

Comment on attachment 8984591 [details]
Bug 1462742 - Don't build display items for the custom content container frame twice during the same paint.

Approval Request Comment
[Feature/Bug causing the regression]: retained display lists
[User impact if declined]: crashes on devices with touch screens when using developer tools on SVG documents
[Is this code covered by automated tests?]: yes, but this specific interaction is not, and this patch doesn't add a crashtest
[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
[Is the change risky?]: no
[Why is the change risky/not risky?]: the bug is well-understood and the patch is very small
[String changes made/needed]: none

Comment 22

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8984591 [details]
Bug 1462742 - Don't build display items for the custom content container frame twice during the same paint.

Fixes a crash on touchscreen devices with RDL enabled. Approved for 61.0rc1.

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/39e52996f332