Closed Bug 1251091 Opened 4 years ago Closed 4 years ago

fix surface key comparison in ImageSurfaceCache::LookupBestMatch

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox45 + wontfix
firefox46 + fixed
firefox47 + fixed

People

(Reporter: tnikkel, Assigned: tnikkel)

References

Details

Attachments

(1 file)

No description provided.
Attached patch patchSplinter Review
I hit the assert at http://mxr.mozilla.org/mozilla-central/source/image/SurfaceCache.cpp?rev=411f18fdffeb#646 while debugging something else and traced the problem back to this. The existing frame(s) in the ImageSurfaceCache had aFlags == NO_COLORSPACE_CONVERSION, but we were looking for aFlags == 0. I don't know how those aFlags got in the cache, and I can't reproduce now using the same steps. But we should be able to populate the surface cache with surfaces with that flag via canvas.drawImage. So I'll have a go at writing a test in a little bit.
Attachment #8723334 - Flags: review?(dholbert)
Comment on attachment 8723334 [details] [diff] [review]
patch

Good catch! r=me, 2 nits:

First: the extended commit message has:
>The callback was called with the surface key of it's entry,

s/it's/its/

Second:
>-      int64_t idealArea = idealKey.Size().width * idealKey.Size().height;
>-      int64_t surfaceArea = aSurfaceKey.Size().width * aSurfaceKey.Size().height;
>+      int64_t idealArea = aIdealKey.Size().width * aIdealKey.Size().height;
>+      int64_t surfaceArea = currentKey.Size().width * currentKey.Size().height;
>       int64_t bestMatchArea =
>         bestMatchKey.Size().width * bestMatchKey.Size().height;
>       // If the best match is smaller than the ideal size, prefer bigger sizes.
>       if (bestMatchArea < idealArea) {
>         if (surfaceArea > bestMatchArea) {
>           bestMatch = surface;

Nit: you should probably rename "surfaceArea" to "currentArea" in this chunk, for consistency.  (We have idealKey & idealArea, and bestMatchKey and bestMatchArea -- so currentKey's associated area should be currentArea).

(Alternately/also: consider s/current/currentSurface/ if it doesn't make stuff too long.)
Attachment #8723334 - Flags: review?(dholbert) → review+
(In reply to Daniel Holbert [:dholbert] from comment #2)
> Nit: you should probably rename "surfaceArea" to "currentArea" in this
> chunk, for consistency.  (We have idealKey & idealArea, and bestMatchKey and
> bestMatchArea -- so currentKey's associated area should be currentArea).
> 
> (Alternately/also: consider s/current/currentSurface/ if it doesn't make
> stuff too long.)

I changed:
surface -> current
surfaceArea -> currentArea.
I didn't call the current surface |currentSurface| as we have |bestMatch| for the best match surface and it seems reasonable to omit the "surface" suffix for surfaces in this loop where we are concerned with finding the best surface. They are a completely different type from other variables in this loop, so if anyone makes a mistake it won't compile.
Try jobs showing the crashtest working (ie failing before patch, passing after patch)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=adbaee4e3f8f
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3776c70b366d
https://hg.mozilla.org/mozilla-central/rev/921b59dcd782
https://hg.mozilla.org/mozilla-central/rev/3078ede85d93
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Comment on attachment 8723334 [details] [diff] [review]
patch

It's probably too late to get this on beta (and hence never ship this regression), but let's get this fixed on aurora.

The patch applies on top of bug 1228314 and bug 1251742, which are both simple fixes that probably don't have much effect in practice, but it's easier (and hence less chance to make mistakes) to just uplift them instead of rebasing this patch.

Approval Request Comment
[Feature/regressing bug #]: bug 1186796
[User impact if declined]: find the wrong image surface
[Describe test coverage new/current, TreeHerder]: added crashtest
[Risks and why]: safe
[String/UUID change made/needed]: none
Attachment #8723334 - Flags: approval-mozilla-aurora?
Regression in 45, tracking. (but i agree it is late for 45)
Comment on attachment 8723334 [details] [diff] [review]
patch

Please uplift to aurora, crash fix for recent regression.  Adds new tests.
Attachment #8723334 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.