Timothy Nikkel (:tnikkel) Assignee
	Description • 9 years ago

      No description provided.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 1 • 9 years ago

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.

	Daniel Holbert [:dholbert]
	Comment 2 • 9 years ago

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.)

	Timothy Nikkel (:tnikkel) Assignee
	Comment 3 • 9 years ago

(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.

	Timothy Nikkel (:tnikkel) Assignee
	Comment 4 • 9 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=78a079aa6708

	Timothy Nikkel (:tnikkel) Assignee
	Comment 5 • 9 years ago

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

	Pulsebot
	Comment 6 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/921b59dcd782
https://hg.mozilla.org/integration/mozilla-inbound/rev/3078ede85d93

	Carsten Book [:Tomcat]
	Comment 7 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/921b59dcd782
https://hg.mozilla.org/mozilla-central/rev/3078ede85d93

	Timothy Nikkel (:tnikkel) Assignee
	Comment 8 • 9 years ago

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

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 9 • 9 years ago

Regression in 45, tracking. (but i agree it is late for 45)

	Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)
	Comment 10 • 9 years ago

Comment on attachment 8723334 [details] [diff] [review]
patch

Please uplift to aurora, crash fix for recent regression.  Adds new tests.

	Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)
	Comment 11 • 9 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-aurora/rev/821f5cfb92c5
https://hg.mozilla.org/releases/mozilla-aurora/rev/1de2a92b1004