800319 - Investigate possibility of gradient cache backend mismatches

Status: RESOLVED FIXED

Description

[Bas Schouten (:bas.schouten)]

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?

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Ignore GradientStops of the wrong backend.

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.

Comment 2

[Jeff Muizelaar [:jrmuizel]]

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?

Comment 3

[Bas Schouten (:bas.schouten)]

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

Comment 4

[Bas Schouten (:bas.schouten)]

I mistook ajones for padenot! :)

Comment 5

[Paul Adenot (:padenot)]

Attachment: Add the backend type to the hash.

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

Comment 6

[Bas Schouten (:bas.schouten)]

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();

Comment 7

[Paul Adenot (:padenot)]

Ah, I got confused because I was testing on a Linux machine, and |ctx->GetDrawTarget()->GetType()| was null. Sorry about that.

Comment 8

[Paul Adenot (:padenot)]

Attachment: Make the gradient cache differentiate between backend types. r=

This should do the trick. Again, it's not tested too much, as I don't have a
Windows machine.

Comment 9

[Bas Schouten (:bas.schouten)]

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!

Comment 10

[Paul Adenot (:padenot)]

Attachment: Make the gradient cache differentiate between backend types.

Comment 11

[Paul Adenot (:padenot)]

Bas, would it be reasonably easy to write a crash test for that fix?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 670375 [details] [diff] [review]
Ignore GradientStops of the wrong backend.

Please mark obsolete patches as such.

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/70f9b80d2087

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/70f9b80d2087

Comment 15

[Paul Adenot (:padenot)]

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

Comment 16

[bhavana bajaj [:bajaj]]

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.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/299d63aee8ef