Closed Bug 1161815 Opened 6 years ago Closed 6 years ago

Sometimes CreateBrushForPattern can return a nullptr brush

Categories

(Core :: Graphics, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox39 --- fixed
firefox40 --- fixed
thunderbird_esr38 40+ fixed

People

(Reporter: bas.schouten, Assigned: bas.schouten)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attachment #8601795 - Flags: review?(jmuizelaar)
Refactored a little to be more elegant and robust.
Attachment #8601795 - Attachment is obsolete: true
Attachment #8601795 - Flags: review?(jmuizelaar)
Attachment #8601798 - Flags: review?(jmuizelaar)
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.
Attachment #8601798 - Flags: review?(jmuizelaar) → review+
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.
https://hg.mozilla.org/mozilla-central/rev/51798e7d8abc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
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
Attachment #8601798 - Flags: approval-mozilla-aurora?
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!
Attachment #8601798 - Flags: approval-mozilla-aurora? → approval-mozilla-beta+
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
Attachment #8601798 - Flags: approval-mozilla-esr38?
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.
Flags: needinfo?(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.
Flags: needinfo?(ryanvm) → needinfo?(bas)
(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.
Flags: needinfo?(bas)
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.
Attachment #8601798 - Flags: approval-mozilla-esr38? → approval-mozilla-esr38-
You need to log in before you can comment on or make changes to this bug.