1265453 - crash in mozilla::InnermostScrollClipApplicableToAGR

Status: RESOLVED WORKSFORME

(Core :: Web Painting, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-cd4f2402-2793-47f9-a420-50d2f2160201.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::InnermostScrollClipApplicableToAGR 	layout/base/FrameLayerBuilder.cpp
1 	xul.dll 	mozilla::ContainerState::ProcessDisplayItems(nsDisplayList*) 	layout/base/FrameLayerBuilder.cpp
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/base/FrameLayerBuilder.cpp
3 		@0x5fe010f 	
4 		@0x4b4001b

Comment 1

[[:philipp]]

this signature seems to be regressing in builds since firefox 46 & related to the work in bug 1147673.

Comment 2

[Markus Stange [:mstange]]

We're crashing when we try to get the vtable of scrollClip->mScrollableFrame. So we can't read from the memory that the mScrollableFrame pointer points at. So either mScrollableFrame has been freed and the memory that it occupied has been taken away, or the mScrollableFrame pointer itself is garbage. The latter seems more likely. If the mScrollableFrame pointer is garbage, either the scrollClip object's contents have been overwritten with garbage, or the scrollClip pointer is not pointing at a valid scroll clip object. The latter seems unlikely because we get the scrollClip pointer by walking mParent pointers from other scroll clips, and all scroll clips have the same life time (they're stored in the nsDisplayListBuilder's arena).
If something has overwritten the scrollClip object's contents, I don't think we have much to go on for finding out where this happened.

Comment 3

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

Crash volume for signature 'mozilla::InnermostScrollClipApplicableToAGR':
 - nightly (version 50): 5 crashes from 2016-06-06.
 - aurora  (version 49): 13 crashes from 2016-06-07.
 - beta    (version 48): 372 crashes from 2016-06-06.
 - release (version 47): 749 crashes from 2016-05-31.
 - esr     (version 45): 0 crashes from 2016-04-07.

Crash volume on the last weeks:
            W. N-1  W. N-2  W. N-3  W. N-4  W. N-5  W. N-6  W. N-7
 - nightly       0       0       1       1       1       1       1
 - aurora        0       1       2       1       3       2       3
 - beta         55      66      48      36      61      43      42
 - release     113      78      97     105     102      99      92
 - esr           0       0       0       0       0       0       0

Affected platform: Windows

Comment 4

[David Bolter [:davidb] (NeedInfo me for attention)]

Jet could we get more diagnostics here somehow in hopes this can become more actionable?

Comment 5

[Markus Stange [:mstange]]

I have an idea for what might be happening.

At http://searchfox.org/mozilla-central/rev/b35975ec17c8227e827800fa2d9642655cb103a8/layout/base/nsDisplayList.cpp#889 we store a pointer to a DisplayItemClip in a frame property, and this pointer is only valid for the current paint (it points into the display list builder's arena). We are supposed to remove the frame property with this pointer during nsDisplayListBuilder::LeavePresShell (which calls ResetMarkedFramesForDisplayList()), but maybe that isn't happening reliably?

Timothy, what do you think of, instead of using a frame property, having a hash table inside the display list builder that maps from nsIFrame* to OutOfFlowDisplayData?

Comment 6

[Timothy Nikkel (:tnikkel)]

Hmm, the mechanism to set and delete the frame property isn't all the complicated. I'd hope it isn't hiding bugs, but if you want to write the patch to move away from frame properties that's acceptable.

Comment 7

[Andrew Overholt [:overholt]]

Markus, should we keep tracking this for upcoming releases? Did you ever write the patch proposed in comment 5?

Comment 8

[Markus Stange [:mstange]]

I did not, and tn's comment makes me think it wouldn't help. I don't really know where to go from here.

Comment 9

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

Crash volume for signature 'mozilla::InnermostScrollClipApplicableToAGR':
 - nightly (version 52): 2 crashes from 2016-09-19.
 - aurora  (version 51): 2 crashes from 2016-09-19.
 - beta    (version 50): 129 crashes from 2016-09-20.
 - release (version 49): 334 crashes from 2016-09-05.
 - esr     (version 45): 0 crashes from 2016-07-25.

Crash volume on the last weeks (Week N is from 10-17 to 10-23):
            W. N-1  W. N-2  W. N-3  W. N-4
 - nightly       0       0       2       0
 - aurora        2       0       0       0
 - beta         37      41      33       9
 - release      90      98      84      26
 - esr           0       0       0       0

Affected platform: Windows

Crash rank on the last 7 days:
           Browser   Content     Plugin
 - nightly
 - aurora            #425
 - beta    #915      #189
 - release #1071     #219
 - esr

Comment 10

[Andrew Overholt [:overholt]]

Given lack of a potential fix here and the low volume of crashes, 50->wontfix.

Comment 11

[Marcia Knous [:marcia]]

This signature still is pretty low volume - 1 crash on 53 and even on the last 50 release it only hit 130 crashes.

Comment 12

[Randell Jesup [:jesup] (needinfo me)]

The 53 crash in the last week was an EXEC crash with a random address.  Other crashes are clear UAF signatures.  Given an EXEC UAF, this is at least a sec-high, might even be sec-crit.

Comment 13

[Frederik Braun [:freddy]]

Timothy, do you have any other suggestions how to make this bug progress? It seems Markus's analysis in comment 5 may still be useful?

Comment 14

[Timothy Nikkel (:tnikkel)]

Markus _just_ landed a giant re-write of most of this code in bug 1298218. Whether this bug remains or goes away after that, either would be interesting.

Comment 5 could be implmented, but what makes me wary of doing that is if we aren't handling removing the frame propertly properly, then we also use similar code for tracking this frame property as we do for tracking other things. So it would potentially fix this bug, but it would leave other bugs.

Comment 15

[Frederik Braun [:freddy]]

That sounds great. For the sake of getting our security bugs not sit around unfixed, this needs a clear owner.

Timothy, as a module peer you have obviously much greater insight as to what can be done about this.
I will assign you, but feel free to find someone else to take this. If you're unsure, you could wait abit and monitor the crash rates with the rewrite landed. The link in comment 0 will lead you in the right direction.

Comment 16

[Timothy Nikkel (:tnikkel)]

Bug 1298218 deleted this entire function, so there is nothing to do here.