1401524 - Crash in mozilla::image::ImageSurfaceCache::SuggestedSize (again)

Status: RESOLVED FIXED

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

Description

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

Looks like there is a new, low volume variant of the SuggestedSize crash even after bug 1397235 landed. The image surface cache is empty somehow. Perhaps as a result of Prune or CollectSizeOfSurfaces.

https://crash-stats.mozilla.com/report/index/b13117d4-f2d3-488f-b6ef-eb9fd0170920
https://crash-stats.mozilla.com/report/index/634ca4bd-2019-4e20-b5ff-f8fb80170920
https://crash-stats.mozilla.com/report/index/b13117d4-f2d3-488f-b6ef-eb9fd0170920

Comment 1

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

Attachment: Ensure SurfaceCache state coherency whenever we perform an operation that may discard surfaces., v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9308df4712f085c72bb9d9fe67a31f302a14714e

Comment 2

[Andrew McCreight [:mccr8]]

This is showing up on Android as [@ mozilla::image::ImageSurfaceCache::SuggestedSize ]

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/220a9b1bed87
Ensure SurfaceCache state coherency whenever we perform an operation that may discard surfaces. r=tnikkel

Comment 4

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

https://hg.mozilla.org/mozilla-central/rev/220a9b1bed87

Comment 5

[Sylvestre Ledru [:Sylvestre]]

I guess we want to uplift this to 57, right?

Comment 6

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #5)
> I guess we want to uplift this to 57, right?

Yes. I wanted to confirm the crashes are gone in 58 first however, and it is only just making it into today's build.

Comment 7

[Sylvestre Ledru [:Sylvestre]]

ok, fyi, 57b3 gtb is Monday evening Paris time!

Comment 8

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

Comment on attachment 8910289 [details] [diff] [review]
Ensure SurfaceCache state coherency whenever we perform an operation that may discard surfaces., v1

Approval Request Comment
[Feature/Bug causing the regression]: 1380649, 1370412
[User impact if declined]: May experience occasional crash.
[Is this code covered by automated tests?]: In part.
[Has the fix been verified in Nightly?]: Yes, we stopped getting crash reports from the field.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: N/A
[Is the change risky?]: No.
[Why is the change risky/not risky?]: Contained to one module, well understood implications. The only time we will have a behaviour change is when we would have crashed a bit later anyways.
[String changes made/needed]: N/A

Comment 9

[Sylvestre Ledru [:Sylvestre]]

No crash on the two first signature after 21th builds
One on the 23th build bp-0015fd63-4f3b-46a8-a098-98fc30170924 which maybe didn't have the fix.
Anyway, it shows a positive improvement, so, taking it.

Comment 10

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

(In reply to Sylvestre Ledru [:sylvestre] from comment #9)
> No crash on the two first signature after 21th builds
> One on the 23th build bp-0015fd63-4f3b-46a8-a098-98fc30170924 which maybe
> didn't have the fix.
> Anyway, it shows a positive improvement, so, taking it.

That has the fix, but the crash suggests a different problem. I'm not sure how to explain that one! It claims to be crashing returning a value we have on the stack. Or possible checking an object variable on the heap (which should be valid, we check before using it). We see some very low volume uncommon/weird signatures in imagelib that just seem to be random -- unless it happens with some volume, I'm not terribly worried.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5addb4530c11

Comment 12

[Andrei Vaida [:avaida]]

(In reply to Andrew Osmond [:aosmond] from comment #8)
> [Is this code covered by automated tests?]: In part.
> [Has the fix been verified in Nightly?]: Yes, we stopped getting crash
> reports from the field.
> [Needs manual test from QE? If yes, steps to reproduce]: No.

Setting qe-verify- based on Andrew's assessment on manual testing needs and the fact that this fix has - at least in some measure - automated coverage.