Closed
Bug 800319
Opened 8 years ago
Closed 8 years ago
Investigate possibility of gradient cache backend mismatches
Categories
(Core :: Graphics, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: bas.schouten, Assigned: padenot)
Details
Attachments
(1 file, 3 obsolete files)
5.89 KB,
patch
|
padenot
:
review+
bajaj
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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?
Reporter | ||
Comment 1•8 years ago
|
||
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)
Updated•8 years ago
|
Assignee: nobody → ajones
Comment 2•8 years ago
|
||
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-
Reporter | ||
Comment 3•8 years ago
|
||
(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. :)
Updated•8 years ago
|
Assignee: ajones → nobody
Reporter | ||
Comment 4•8 years ago
|
||
I mistook ajones for padenot! :)
Assignee | ||
Comment 5•8 years ago
|
||
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).
Reporter | ||
Comment 6•8 years ago
|
||
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-
Assignee | ||
Comment 7•8 years ago
|
||
Ah, I got confused because I was testing on a Linux machine, and |ctx->GetDrawTarget()->GetType()| was null. Sorry about that.
Assignee | ||
Comment 8•8 years ago
|
||
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)
Assignee | ||
Updated•8 years ago
|
Attachment #674231 -
Attachment is obsolete: true
Reporter | ||
Comment 9•8 years ago
|
||
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+
Assignee | ||
Comment 10•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Attachment #675157 -
Attachment is obsolete: true
Assignee | ||
Comment 11•8 years ago
|
||
Bas, would it be reasonably easy to write a crash test for that fix?
Status: ASSIGNED → NEW
Keywords: checkin-needed
Comment 12•8 years ago
|
||
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
Comment 13•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/70f9b80d2087
Flags: in-testsuite?
Keywords: checkin-needed
Comment 14•8 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/70f9b80d2087
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
Updated•8 years ago
|
status-firefox18:
--- → affected
status-firefox19:
--- → fixed
tracking-firefox18:
--- → +
tracking-firefox19:
--- → +
Assignee | ||
Updated•8 years ago
|
Attachment #675157 -
Attachment is obsolete: false
Assignee | ||
Comment 15•8 years ago
|
||
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 16•8 years ago
|
||
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+
Updated•8 years ago
|
Attachment #675157 -
Attachment is obsolete: true
You need to log in
before you can comment on or make changes to this bug.
Description
•