1375455 - Crash in mozilla::ActiveScrolledRoot::IsAncestor

Status: RESOLVED WORKSFORME

(Core :: Web Painting, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-8db71cae-6a7b-46d5-b1f0-dc04d0170616.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::ActiveScrolledRoot::IsAncestor(mozilla::ActiveScrolledRoot const*, mozilla::ActiveScrolledRoot const*) 	layout/painting/nsDisplayList.cpp:135
1 	xul.dll 	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) 	layout/painting/FrameLayerBuilder.cpp:5613
2 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) 	layout/painting/nsDisplayList.cpp:2127
3 	xul.dll 	nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) 	layout/base/nsLayoutUtils.cpp:3683
4 	xul.dll 	mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) 	layout/base/PresShell.cpp:6496
5 	xul.dll 	nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) 	view/nsViewManager.cpp:483
6 	xul.dll 	nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) 	view/nsViewManager.cpp:415
7 	xul.dll 	nsViewManager::ProcessPendingUpdates() 	view/nsViewManager.cpp:1104

this cross-platform crash signature is showing up since firefox 54 and later, which looks related to 1298218.
a number of reports seem to crash with an address indicating a UAF situation.

Comment 1

[Andrew McCreight [:mccr8]]

There are only a couple of crashes, so I don't know if there's anything we can do here. Matt, any ideas? Thanks.

Comment 2

[Matt Woodrow (:mattwoodrow)]

I suspect this is similar to other ActiveScrolledRoot crashes we have on file.

Thinking about this, we only allocate ActiveScrolledRoots on the stack, or within the display list builder arena.

It seems unlikely that we'd poison the stack, or the current arena, so my guess is that the dead ASR belongs to a different (deleted) builder.

That could be a builder from a previous frame, or possibly a builder for a nested call to PaintFrame.

If it's the former, then the only way I can see us retaining ASR pointers is if we managed to have an OutOfFlowDisplayData survive longer than it was meant to.

We could try moving OutOfFlowDisplayData into the arena (so that it'll be poisoned too), counting ctors and dtors, and release asserting that we've deleted them all when we destroy the builder. We can also release assert that retrieving an OutOfFlowDisplayData happens on the same builder as it was created with.

I can't see any way this would actually happen, but this should give us more information if it is.

Comment 3

[Markus Stange [:mstange]]

That sounds good. I came to similar conclusions.

The other idea I had was to add some instrumentation in debug builds that makes sure that we don't have any pointers to ASR objects or to a DisplayItemClipChain objects from arenas of display list builders that no longer exist. We could do that by replacing all our regular pointers with special smart pointers that automatically notify the correct display list builder of their creation and destruction, so that we can check an "alive pointers" counter at display list builder destruction.
But this instrumentation would only help us if the problem happens in debug builds in our tests.

Comment 4

[Daniel Veditz [:dveditz]]

why related to bug 1298218? It looks like that one landed in early February and the earliest I see this signature is mid-April (a fairly steady couple of crashes a day since then). I guess that does match up with 54 going to beta. Maybe there just weren't enough users on nightly/aurora to trigger this (more than the one April 13 aurora crash).

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Sounds like you might want a diagnostic patch of some kind but we can hold off on it until 55 if so.

Comment 6

[Frederik Braun [:freddy]]

Matt, from comment 2 and comment 3 it sounds like you didn't continue your conversation
So, do you want a diagnostic patch for this?
Getting more information sounds like the obvious next step.

Comment 7

[Mike Taylor [:miketaylr]]

So far no crashes on 57, but just a few on 56.

Comment 8

[Matt Woodrow (:mattwoodrow)]

Nathan Froyd had something that looked similar to this (DisplayItemClipChain UAF) that he could reproduce locally.

Markus was going to try debug it next week, and I can also have a go next week if he's busy.

Comment 9

[Matt Woodrow (:mattwoodrow)]

Low volume, and we don't have STR yet, so fix-optional for 57.

Comment 10

[Wennie]

Hi Matt:

I have assigned these security bugs to you to reassign them to appropriate developers in your team to investigate and fix them.

Thanks!

Wennie

Comment 11

[Daniel Veditz [:dveditz]]

use-after-free crashes continue to happen in recent release and nightly builds

Comment 12

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee didn't login in Bugzilla in the last 7 months and this bug has priority 'P2'/severity 'critical'.
:miko, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 13

[Miko Mynttinen]

We have had total 26 crashes with this signature in the last half a year, 80% of them have come from Win7. Last crash was in 99.0.1.

Comment 14

[BugBot [:suhaib / :marco/ :calixte]]

Since the bug is closed, the stalled keyword is now meaningless.
For more information, please visit auto_nag documentation.