1161815 - Sometimes CreateBrushForPattern can return a nullptr brush

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Bas Schouten (:bas.schouten)]

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.

Comment 1

[Bas Schouten (:bas.schouten)]

Attachment: Use a single ID2D1SolidColorBrush per DrawTarget

Comment 2

[Bas Schouten (:bas.schouten)]

Attachment: Use a single ID2D1SolidColorBrush per DrawTarget v2

Refactored a little to be more elegant and robust.

Comment 3

[Jeff Muizelaar [:jrmuizel]]

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 4

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 5

[Bas Schouten (:bas.schouten)]

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

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51798e7d8abc

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/51798e7d8abc

Comment 8

[Bas Schouten (:bas.schouten)]

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

Comment 9

[Robert Kaiser]

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?

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Robert Kaiser (:[e-mail]) 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 11

[Liz Henry (:lizzard) (relman/hg->git project)]

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!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/13e298c9841e

Comment 13

[Kent James (:rkent)]

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

Comment 14

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 17

[Bas Schouten (:bas.schouten)]

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

[Liz Henry (:lizzard) (relman/hg->git project)]

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.