Crash in AssertUniqueItem on devices with touch screens

VERIFIED FIXED in Firefox 61

Status

()

defect
--
critical
VERIFIED FIXED
Last year
Last year

People

(Reporter: calixte, Assigned: mstange)

Tracking

(Blocks 2 bugs, {crash, regression})

Trunk
mozilla62
Unspecified
Windows 10
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61+ fixed, firefox62+ fixed)

Details

(crash signature)

Attachments

(1 attachment)

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
Flags: needinfo?(matt.woodrow)
MOZ_RELEASE_ASSERT(false) (Duplicate display item!)
Blocks: RDLbugs
Assignee: nobody → mstange
Status: NEW → ASSIGNED
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 ?
Flags: needinfo?(bwerth)
Blake, can you still reproduce this reliably?
Flags: needinfo?(bwinton)
Yup. Did someone do something that they think fixed it, or were you just double-checking? ;)
Flags: needinfo?(bwinton)
We determined that in order for this crash to be reproduced, the device must have a touch screen.
Summary: Crash in AssertUniqueItem → Crash in AssertUniqueItem on devices with touch screens
See Also: → 1467856
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.
Flags: needinfo?(matt.woodrow)
Flags: needinfo?(bwerth)
I found a workaround. I wrote a very confident comment but I'm not entirely sure everything in it is accurate.
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?
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 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()`.
Attachment #8984591 - Flags: review?(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.
Attachment #8984591 - Flags: review+
Err, comment 12 is supposed to say:

  if (frame->StyleDisplay()->mTopLayer != NS_STYLE_TOP_LAYER_NONE)
Bug 1157592 contains the right fix now.
Is this good to land? Would be nice if we could get it landed in time for Thursday's b14 build if possible.
Flags: needinfo?(mstange)
I'm applying Emilio's comments at the moment and will land on autoland shortly afterwards.
Flags: needinfo?(mstange)
Pushed by mstange@themasta.com:
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
https://hg.mozilla.org/mozilla-central/rev/6bcd91f7f50a
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla62
Verified that the crash is now gone in the latest Nightly.
Status: RESOLVED → VERIFIED
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
Attachment #8984591 - Flags: approval-mozilla-beta?
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.
Attachment #8984591 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.