1241217 - crash in nsPresContext::GetParentPresContext (from nsRefreshDriver::IsWaitingForPaint())

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

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

This bug was filed from the Socorro interface and is 
report bp-afcbf2df-d834-429d-9fb8-d56922160114.
=============================================================

UAF read (~70 in the last week), called from nsRefreshDriver::IsWaitingForPaint().  Very likely the view has been freed; something isn't holding a ref it needs probably.

Goes back to at least 38, and likely much further - some indications in crashstats indicate 22, though those may be different crashes in the same function:

22 Android crash is nsViewManager::CallWillPaintOnObservers() https://crash-stats.mozilla.com/report/index/ac213ffa-6494-46c9-b694-ccc402160117

22 windows: IsDOMPaintEventWaiting() - https://crash-stats.mozilla.com/report/index/70e6bad5-e19a-4ab9-a3a9-dded82160114

CC/NI Jet for triage

Comment 1

[Jet Villegas (inactive)]

To Matt for a look. Please check the raw dumps to see if there's an URL to test, or if this is a shutdown crash.

Comment 2

[Timothy Nikkel (:tnikkel)]

These all involve GetDisplayRootPresContext which has a weird quirk, and I've had my eyes on removing it for a while. So I filed bug 1241651 to remove it, I have a hunch that it might fix this bug.

Comment 3

[Matt Woodrow (:mattwoodrow)]

This is a low volume crash (only 1 report on 46 so far), so it might take a while to find out if that fixes it or not. That said, it's been around for ages so we can probably afford for the possible fix to ride the trains.

The URLs don't show any obvious patterns so we don't have any other way forward right now.

Comment 4

[Mats Palmgren (inactive)]

It might be worth uplifting bug 1241651 to v45 since v45 will be the next ESR.
Assuming you agree it's a low-risk change.

Comment 5

[Mats Palmgren (inactive)]

(Uplifting will also give us data sooner, to evaluate whether that change fixed this bug or not.)

Comment 6

[Mats Palmgren (inactive)]

=> tn since we hope bug 1241651 will fix this.
What's your thoughts about uplifting that bug?

Comment 7

[Timothy Nikkel (:tnikkel)]

I requested uplift to beta (45) for bug 1241651.

Comment 8

[Daniel Holbert [:dholbert]]

(Bug 1241651's beta45 patch landed last Thursday, BTW. I guess we're waiting for new crash-stats data now.)

Comment 9

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

Adding tracking since this affected 45 and 46 and is sec-critical, even if it may be fixed by now.

Comment 10

[Mats Palmgren (inactive)]

The last build ID on crashstats is 2016012315 so I think we can call this fixed (by bug 1241651).

Comment 11

[Daniel Veditz [:dveditz]]

[Tracking Requested - why for this release]:
This does affect ESR 38.x, based on the regressing check-in and finding this UAF crash in crash-stats. It's not a top-crash, but it's an exploitable crash.

Timothy: is this change safe to make in ESR-38?

Comment 12

[Timothy Nikkel (:tnikkel)]

(In reply to Daniel Veditz [:dveditz] from comment #11)
> [Tracking Requested - why for this release]:
> This does affect ESR 38.x, based on the regressing check-in and finding this
> UAF crash in crash-stats. It's not a top-crash, but it's an exploitable
> crash.
> 
> Timothy: is this change safe to make in ESR-38?

Requested esr38 uplift in bug 1241651.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Approved the uplift request in bug 1241651.