1397223 - Crash in ExpirationTrackerImpl<T>::RemoveObjectLocked

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is 
report bp-875c3e4f-d371-4700-8655-84dee0170906.
=============================================================

There are 16 crashes in nightly 57, 167 in beta 56, 168 in release 55 and 227 in esr 52.

:aosmond, could you investigate please ?

Comment 1

[Andrew Osmond [:aosmond] (he/him)]

I can guess what happened. ExpirationTrackerImpl::AddObjectLocked can fail due to a number of conditions. If it fails even once, it can cause a signature similar to this crash later on.

Comment 2

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Gracefully handle failures in SurfaceCacheImpl::StartTracking., v1

This patch discards the surface if it cannot track it (only can happen if it is unlocked, so in theory, safe to discard even if premature).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=26d2e2d084091eccbe8f56875aeb11d661a7dc2e

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8905159 [details] [diff] [review]
Gracefully handle failures in SurfaceCacheImpl::StartTracking., v1

Hmm, just realized that MarkUsed suffers from the same problem. Hold on.

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Gracefully handle failures in SurfaceCacheImpl::StartTracking., v2

Let's try that again...

https://treeherder.mozilla.org/#/jobs?repo=try&revision=84ff209245173549bb463f8c2ab6ec781ad3a522

Comment 5

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Andrew Osmond [:aosmond] from comment #4)
> Created attachment 8905199 [details] [diff] [review]
> Gracefully handle failures in SurfaceCacheImpl::StartTracking., v2
> 
> Let's try that again...
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=84ff209245173549bb463f8c2ab6ec781ad3a522

Dammit...thinking once more... Handling cache insert and unlock failures with removals was probably okay. Handling *mark used* the same way might be not the best thing to do, as we clearly *just accessed* the surface. Timothy, maybe we are better off just adding some extra state which indicates whether or not we *expect* the surface to be in the tracker (simply so we can assert on it to catch any coding errors...) rather than removing it or asserting it is in shutdown (could easily be an OOM)?

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #5)
> (In reply to Andrew Osmond [:aosmond] from comment #4)
> > Created attachment 8905199 [details] [diff] [review]
> > Gracefully handle failures in SurfaceCacheImpl::StartTracking., v2
> > 
> > Let's try that again...
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=84ff209245173549bb463f8c2ab6ec781ad3a522
> 
> Dammit...thinking once more... Handling cache insert and unlock failures
> with removals was probably okay. Handling *mark used* the same way might be
> not the best thing to do, as we clearly *just accessed* the surface.
> Timothy, maybe we are better off just adding some extra state which
> indicates whether or not we *expect* the surface to be in the tracker
> (simply so we can assert on it to catch any coding errors...) rather than
> removing it or asserting it is in shutdown (could easily be an OOM)?

I think it's reasonable to handle MarkUsed that way even if we just accessed the surface. If it fails we are in bad shape and we shouldn't try to pretend to be doing well.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6956da301f
Gracefully handle failures in SurfaceCacheImpl::StartTracking. r=tnikkel

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1e6956da301f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Going off crash-stats, it looks like this patch was effective. Can you please provide a rebased patch for Beta (doesn't graft cleanly) and nominate it for approval? Thanks!

Comment 10

[Andrew Osmond [:aosmond] (he/him)]

Attachment: [uplift for beta] Gracefully handle failures in SurfaceCacheImpl::StartTracking., v1 [carries r=tnikkel]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=50faf92d5a8b1239afb6f86a089c96f146b1eda2

Rebased for mozilla-beta. Waiting on the try results before requesting uplift to make sure I didn't make any mistakes.

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8906608 [details] [diff] [review]
[uplift for beta] Gracefully handle failures in SurfaceCacheImpl::StartTracking., v1 [carries r=tnikkel]

Approval Request Comment
[Feature/Bug causing the regression]: N/A
[User impact if declined]: Occasional crashes due to OOM and/or during shutdown, mostly in content process.
[Is this code covered by automated tests?]: No. We test the surface cache which are where the changes are, but we never seem to hit the OOM or shutdown condition necessary to crash.
[Has the fix been verified in Nightly?]: Yes. Crash reports stopped as soon as the fix landed in nightly.
[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?]: It is well contained in one module and boils down to actually handling previously ignored error codes.
[String changes made/needed]: None.

Comment 12

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8906608 [details] [diff] [review]
[uplift for beta] Gracefully handle failures in SurfaceCacheImpl::StartTracking., v1 [carries r=tnikkel]

Crash fix, looks useful from nightly results, let's take this for beta 11.

Comment 13

[Cornel Ionce [:noni] [Hubs QA]]

(In reply to Andrew Osmond [:aosmond] from comment #11)
> [Needs manual test from QE? If yes, steps to reproduce]: No.
Marking this issue as qe-verify -, per Andrew's assessment on manual qa needs.

Comment 14

[Calixte Denizet (:calixte)]

No more crash on 57.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Bah, I uplifted this yesterday but forgot to mark the bug afterward. This made it in for b11.
https://hg.mozilla.org/releases/mozilla-beta/rev/0b5841c7c399

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

I looked at the crashes with the given signatures on 56.0b11 -- they are completely different issues from this one, so I wouldn't expect the crash rate to go to zero.