1359233 - Crash in mozilla::ActiveScrolledRoot::PickDescendant

Status: RESOLVED INCOMPLETE

(Core :: Web Painting, defect, P2)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-ba0b2e93-755f-47a1-8b8d-865f40170424.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::ActiveScrolledRoot::PickDescendant(mozilla::ActiveScrolledRoot const*, mozilla::ActiveScrolledRoot const*) 	layout/painting/nsDisplayList.h:197
1 	xul.dll 	mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) 	layout/painting/FrameLayerBuilder.cpp:4082
2 	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:5668
3 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) 	layout/painting/nsDisplayList.cpp:2120
4 	xul.dll 	nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) 	layout/base/nsLayoutUtils.cpp:3683
5 	xul.dll 	mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) 	layout/base/PresShell.cpp:6490
6 	xul.dll 	nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) 	view/nsViewManager.cpp:483
7 	xul.dll 	nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) 	view/nsViewManager.cpp:415
8 	xul.dll 	nsViewManager::ProcessPendingUpdates() 	view/nsViewManager.cpp:1104

this crash signature is newly showing up on firefox 54 and above - so far only on windows & with rather low volume on 54.0b1. most of those crashes are reported in the content process.

Comment 1

[Gerry Chang [:gchang]]

Hi Astley,
Can you help take a look at this one?

Comment 2

[Peter Chang[:pchang]]

Markus, do you have any idea about this crash?

Comment 3

[Astley Chen (inactive)]

From the crash report in [1], the crash address was at 0xffffffffe5e5e5ed which probably suggested it's using a memory already freed by jemalloc.

Markus, is it similar to bug 1141089 ?

[1] https://crash-stats.mozilla.com/report/index/34faf7ef-55d1-4bb4-aea7-362b80170507

Comment 4

[Daniel Veditz [:dveditz]]

Definitely looks like a UAF. Only about 1/6 of the Beta crashes show the poisoned e5e5e5e5 marker which means there's enough time between the free and the re-use for real data to get stored in the freed hole. That ups the odds that this could be exploitable.

Interesting that one of the crashes looks like a "frame poisoning" address. That was a use-after-free memory partitioning defense we developed for objects in the frame tree.
bp-c4ff10be-adc8-4073-9a25-c3b2b0170508

Comment 5

[Markus Stange [:mstange]]

(In reply to Astley Chen [:astley] (UTC+8) from comment #3)
> Markus, is it similar to bug 1141089 ?

I don't know.

We're crashing here:

    const ActiveScrolledRoot* scrollMetadataASR =
        layerClipChain ? ActiveScrolledRoot::PickDescendant(itemASR, layerClipChain->mASR) : itemASR;

itemASR is 0x136ffe18, layerClipChain is 0x136fff30, and layerClipChain->mASR is 0xe5e5e5e5.

So that must mean that layerClipChain has already been deallocated, or that the layerClipChain pointer is wrong.
layerClipChain is a DisplayItemClipChain*, and DisplayItemClipChains only get deallocated when the display list builder's arena goes away, which happens after layerization code runs.
So I think it's more likely that the layerClipChain pointer is bad.

layerClipChain comes from item->GetClipChain()[->mParent]*.

So that means the item's mClipChain is wrong, or any of the mParent pointers along its mParent chain is wrong.

Bug 1359569 is another instance where we have a bad mClipChain pointer.

Comment 6

[Peter Chang[:pchang]]

 Markus, do you have any update for this? So far I only saw crashes from FF 54 only.

Comment 7

[Markus Stange [:mstange]]

Not yet.

Comment 8

[Markus Stange [:mstange]]

I had a look at https://crash-stats.mozilla.com/report/index/7d918037-d44e-44d7-bb0c-eaf6c0170511 today. It has different values, but the crash is basically the same.

itemASR is 0x1cc645c0, layerClipChain is 0x1dc64698, and layerClipChain->mASR is 0xffffffff.

Comment 9

[Gerry Chang [:gchang]]

Track 56+ as sec-high.

Comment 10

[Miko Mynttinen]

One possible cause for this crash is us passing a pointer to stack allocated DisplayItemClipChain in a call to nsDisplayItem::IntersectClip() and nsDisplayListBuilder::CreateClipChainIntersection(). Like the comment at http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/painting/nsDisplayList.h#774 says, this is not good.

We do this in at least two places:
- http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/base/PresShell.cpp#4810
- http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/generic/nsGfxScrollFrame.cpp#3242 (clip allocation: http://searchfox.org/mozilla-central/rev/e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/generic/nsGfxScrollFrame.cpp#3437)

Comment 11

[Miko Mynttinen]

(In reply to Miko Mynttinen [:miko] from comment #10)
> One possible cause for this crash is us passing a pointer to stack allocated
> DisplayItemClipChain in a call to nsDisplayItem::IntersectClip() and
> nsDisplayListBuilder::CreateClipChainIntersection(). Like the comment at
> http://searchfox.org/mozilla-central/rev/
> e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/painting/nsDisplayList.h#774
> says, this is not good.
> 
> We do this in at least two places:
> -
> http://searchfox.org/mozilla-central/rev/
> e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/base/PresShell.cpp#4810
> -
> http://searchfox.org/mozilla-central/rev/
> e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/generic/nsGfxScrollFrame.
> cpp#3242 (clip allocation:
> http://searchfox.org/mozilla-central/rev/
> e76c0fee79a34a3612740b33508276f4b37c3ee8/layout/generic/nsGfxScrollFrame.
> cpp#3437)

I filed a bug 1399977 and submitted a patch fixing these problems. Hopefully we will start seeing this crash signature less.

Comment 12

[Marcia Knous [:marcia]]

Track 58+ as sec high.

Comment 13

[Daniel Veditz [:dveditz]]

(In reply to Miko Mynttinen [:miko] from comment #11)
> I filed a bug 1399977 and submitted a patch fixing these problems. Hopefully
> we will start seeing this crash signature less.

Still seeing a lot of UAFs in 57 beta. Can't tell if it helped a little, but the problem is still there.

Comment 14

[Daniel Veditz [:dveditz]]

Probably not a good idea to try speculative fixes in 57 beta.

Comment 15

[Frederik Braun [:freddy]]

(In reply to Miko Mynttinen [:miko] from comment #11)
> I filed a bug 1399977 and submitted a patch fixing these problems. Hopefully
> we will start seeing this crash signature less.


Unfortunately, this didn't turn out to be true. Any other ideas for this bug?

Comment 16

[Frederik Braun [:freddy]]

Forgot the needinfo, apologies.


(In reply to Frederik Braun [:freddyb] from comment #15)
> (In reply to Miko Mynttinen [:miko] from comment #11)
> > I filed a bug 1399977 and submitted a patch fixing these problems. Hopefully
> > we will start seeing this crash signature less.
> 
> 
> Unfortunately, this didn't turn out to be true. Any other ideas for this bug?

Comment 17

[Miko Mynttinen]

(In reply to Frederik Braun [:freddyb] from comment #15)
> (In reply to Miko Mynttinen [:miko] from comment #11)
> > I filed a bug 1399977 and submitted a patch fixing these problems. Hopefully
> > we will start seeing this crash signature less.
> 
> 
> Unfortunately, this didn't turn out to be true. Any other ideas for this bug?

Sadly, no.

Comment 18

[Julien Cristau [:jcristau]]

No crashes with this signature in 61 so far.  Could this have been fixed somehow?

Comment 19

[Julien Cristau [:jcristau]]

Still nothing on 61 (a few crashes on esr60); resolving as incomplete.