Investigate possibility of gradient cache backend mismatches

RESOLVED FIXED in Firefox 18

Status

()

defect
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: bas.schouten, Assigned: padenot)

Tracking

unspecified
mozilla19
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox18+ fixed, firefox19+ fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

I've seen some crashes locally where it seems we're passing in the wrong type of GradientStops object into Direct2D DrawTargets. I wonder if it's possible for the gradient cache to return a gradient cached for another backend?

Anthony, can you confirm if this is possible, and if so do you want to fix it or should I?
I've confirmed this actually happens. This patch at least prevents us from using the wrong backend type's gradient stops and doing a bogus cast with a corresponding nasty crash. We should still fix the cache of course.
Attachment #670375 - Flags: review?(jmuizelaar)
Assignee: nobody → ajones
Comment on attachment 670375 [details] [diff] [review]
Ignore GradientStops of the wrong backend.

Review of attachment 670375 [details] [diff] [review]:
-----------------------------------------------------------------

Can we make this use gfx::LogFailure instead?
Attachment #670375 - Flags: review?(jmuizelaar) → review-
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Comment on attachment 670375 [details] [diff] [review]
> Ignore GradientStops of the wrong backend.
> 
> Review of attachment 670375 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Can we make this use gfx::LogFailure instead?

I'd rather not, it doesn't exist in Stand-alone Azure and doesn't log through a nice logging system that actually works usably on Windows. :)
Assignee: ajones → nobody
I mistook ajones for padenot! :)
This patch adds the fact that we are using Cairo or not to the hash and the comparison, to avoid retrieving a gradient from the cache which was created by the wrong backend.

My understanding here is that |ctx->IsCairo()| will be |false| when we are using D2D, but |true| if this gradient is using Cairo (because we use it as a fallback).
Assignee: nobody → paul
Status: NEW → ASSIGNED
Attachment #674231 - Flags: review?(bas)
Comment on attachment 674231 [details] [diff] [review]
Add the backend type to the hash.

Review of attachment 674231 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCSSRendering.cpp
@@ +2282,5 @@
>        gradientPattern->SetExtend(gfxPattern::EXTEND_REPEAT);
>      }
>      // Register the gradient newly computed in the cache.
>      pattern = new GradientCacheData(gradientPattern, forceRepeatToCoverTiles,
> +      GradientCacheKey(aGradient, oneCellArea.Size(), flags, ctx->IsCairo()));

This is incorrect, ctx->IsCairo() just indicates if this is an Azure or a Cairo backed context. We're dealing with an Azure backed context here that's using a cairo Azure backend. So we need to look at ctx->GetDrawTarget()->GetType();
Attachment #674231 - Flags: review?(bas) → review-
Ah, I got confused because I was testing on a Linux machine, and |ctx->GetDrawTarget()->GetType()| was null. Sorry about that.
This should do the trick. Again, it's not tested too much, as I don't have a
Windows machine.
Attachment #675157 - Flags: review?(bas)
Attachment #674231 - Attachment is obsolete: true
Comment on attachment 675157 [details] [diff] [review]
Make the gradient cache differentiate between backend types. r=

Review of attachment 675157 [details] [diff] [review]:
-----------------------------------------------------------------

Looks great!
Attachment #675157 - Flags: review?(bas) → review+
Attachment #675157 - Attachment is obsolete: true
Bas, would it be reasonably easy to write a crash test for that fix?
Status: ASSIGNED → NEW
Keywords: checkin-needed
Comment on attachment 670375 [details] [diff] [review]
Ignore GradientStops of the wrong backend.

Please mark obsolete patches as such.
Attachment #670375 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/70f9b80d2087
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Attachment #675157 - Attachment is obsolete: false
Comment on attachment 675533 [details] [diff] [review]
Make the gradient cache differentiate between backend types.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 798224
User impact if declined: Visual glitch on Windows 7.
Testing completed (on m-c, etc.): Landed around two weeks ago on m-c.
Risk to taking this patch (and alternatives if risky): Not too risky, baked for some time, and fix a crash.
String or UUID changes made by this patch: None

Carry forward r+.
Attachment #675533 - Flags: review+
Attachment #675533 - Flags: approval-mozilla-aurora?
Comment on attachment 675533 [details] [diff] [review]
Make the gradient cache differentiate between backend types.

Approving on aurora as the uplift of this will fix Bug 798224.It is low risk ,has had good amount of bake time on m-c and verified on nightly.
Attachment #675533 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #675157 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.