Closed Bug 1401524 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: ImageLib, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- unaffected
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: aosmond, Assigned: aosmond)

References

Details

(Keywords: crash, regression, Whiteboard: [gfx-noted])

Crash Data

Attachments

(1 file)

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
Assignee: nobody → aosmond
Blocks: 1370412
Status: NEW → ASSIGNED
Keywords: crash, regression
Priority: -- → P3
Whiteboard: [gfx-noted]
Crash Signature: [@ mozilla::WrapNotNull<T> | mozilla::image::ImageSurfaceCache::SuggestedSize ] [@ mozilla::image::ImageSurfaceCache::SuggestedSize const ]
Priority: P3 → P1
Priority: P1 → P2
Attachment #8910289 - Flags: review?(tnikkel)
This is showing up on Android as [@ mozilla::image::ImageSurfaceCache::SuggestedSize ]
Crash Signature: [@ mozilla::WrapNotNull<T> | mozilla::image::ImageSurfaceCache::SuggestedSize ] [@ mozilla::image::ImageSurfaceCache::SuggestedSize const ] → [@ mozilla::WrapNotNull<T> | mozilla::image::ImageSurfaceCache::SuggestedSize ] [@ mozilla::image::ImageSurfaceCache::SuggestedSize const ] [@ mozilla::image::ImageSurfaceCache::SuggestedSize ]
Attachment #8910289 - Flags: review?(tnikkel) → review+
Pushed by aosmond@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/220a9b1bed87
Ensure SurfaceCache state coherency whenever we perform an operation that may discard surfaces. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/220a9b1bed87
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
I guess we want to uplift this to 57, right?
Flags: needinfo?(aosmond)
(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.
ok, fyi, 57b3 gtb is Monday evening Paris time!
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
Flags: needinfo?(aosmond)
Attachment #8910289 - Flags: approval-mozilla-beta?
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.
Attachment #8910289 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.