Currently CreateBrushForPattern can return a nullptr brush if for some reason it fails to create a transparent black brush. This issue can be solved by reusing a single SolidColorBrush. This will also benefit performance somewhat.

Refactored a little to be more elegant and robust.

Comment on attachment 8601798 [details] [diff] [review] Use a single ID2D1SolidColorBrush per DrawTarget v2 Review of attachment 8601798 [details] [diff] [review]: ----------------------------------------------------------------- Just for kicks I checked if IE does something similar and it seems to.

Comment on attachment 8601798 [details] [diff] [review] Use a single ID2D1SolidColorBrush per DrawTarget v2 Review of attachment 8601798 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/DrawTargetD2D1.cpp @@ +1409,5 @@ > + > + if (!imageBrush) { > + CreateTransparentBlackBrush(); > + } > + What does this do? Did you mean to return CreateTransparentBlackBrush() or assign it to imageBrush, or did you really mean to just create the brush, but still return imageBrush.forget()/nullptr? If the code is correct, it could probably use an extra comment.

(In reply to Milan Sreckovic [:milan] from comment #4) > Comment on attachment 8601798 [details] [diff] [review] > Use a single ID2D1SolidColorBrush per DrawTarget v2 > > Review of attachment 8601798 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: gfx/2d/DrawTargetD2D1.cpp > @@ +1409,5 @@ > > + > > + if (!imageBrush) { > > + CreateTransparentBlackBrush(); > > + } > > + > > What does this do? Did you mean to return CreateTransparentBlackBrush() or > assign it to imageBrush, or did you really mean to just create the brush, > but still return imageBrush.forget()/nullptr? > > If the code is correct, it could probably use an extra comment. Good catch, it was wrong, it's corrected locally.

Comment on attachment 8601798 [details] [diff] [review] Use a single ID2D1SolidColorBrush per DrawTarget v2 Approval Request Comment [Feature/regressing bug #]: None, dealing with TDRs [User impact if declined]: Crashier builds with TDRs [Describe test coverage new/current, TreeHerder]: Nightly [Risks and why]: Very low [String/UUID change made/needed]: None

Bas, this is already on the aurora channel, which has 40 now, did you mean to ask for uplift to 39, which is in the beta repo now?

(In reply to Robert Kaiser (:kairo@mozilla.com) from comment #9) > Bas, this is already on the aurora channel, which has 40 now, did you mean > to ask for uplift to 39, which is in the beta repo now? I did.. sorry.

Comment on attachment 8601798 [details] [diff] [review] Use a single ID2D1SolidColorBrush per DrawTarget v2 Approved for uplift to beta (39). It will be great if this helps with TDR issues!

https://hg.mozilla.org/releases/mozilla-esr38/rev/db22ef98c2e1 (THUNDERBIRD_38_VERBRANCH)

Comment on attachment 8601798 [details] [diff] [review] Use a single ID2D1SolidColorBrush per DrawTarget v2 [Approval Request Comment] If this is not a sec:{high,crit} bug, please state case for ESR consideration: Frequent ESR38 crashes per the recent comments in bug 1162520. User impact if declined: Crashier builds with TDRs Fix Landed on Version: Fx39+ Risk to taking this patch (and alternatives if risky): Was very low when uplifted to Beta39. AFAIK, there haven't been any known regressions since. String or UUID changes made by this patch: None

Ryan, While it would be great to fix the top crash #2 in ESR, when I read comment #21 of Bas in bug 1162520 ("okayish"), I am not sure I want to take these fixes in esr.

That's your call, I don't have a horse in this race. And TBH, I'm not sure what "okayish" means here. I did my best to surmise the current state of things in comment 14, but it'd probably be better for Bas to elaborate rather than having me guess.

(In reply to Ryan VanderMeulen [:RyanVM] from comment #16) > That's your call, I don't have a horse in this race. > > And TBH, I'm not sure what "okayish" means here. I did my best to surmise > the current state of things in comment 14, but it'd probably be better for > Bas to elaborate rather than having me guess. With okay-ish I mean I'd be lying if I said this was completely risk free, but it's had a fair amount of coverage and it seems to be doing fine. I can't point to a concrete reason why it'd cause any trouble, but it's not a trivial change :-). I -think- it will be fine, but I rather overestimate than underestimate risk.

Comment on attachment 8601798 [details] [diff] [review] Use a single ID2D1SolidColorBrush per DrawTarget v2 The risk sounds too high here to justify going into ESR. The fix will end up in the next major ESR release (45.1.0) coming in early March.