1355873 - Improve the error handling and cleanup our canvas2d code

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect)

Description

[Mats Palmgren (inactive)]

crash-stats shows that we still have some crashes in our canvas2d code,
so I wrote a patch to improve the error handling and cleanup that code
a bit in general, in an attempt to make it more robust:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=57059a6d5612aa15261595707a05d36d547ad5c2

Comment 1

[Mats Palmgren (inactive)]

Attachment: fix

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/286d77c6835a
Add more consistent error handling and some code cleanup. r=mstange

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/286d77c6835a

Comment 4

[Mats Palmgren (inactive)]

Comment on attachment 8857524 [details] [diff] [review]
fix

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:crash
[Is this code covered by automated tests?]:yes
[Has the fix been verified in Nightly?]:no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:no
[Why is the change risky/not risky?]:just adds additional error handling, no functional changes
[String changes made/needed]:none

I think we should take this in v54 to make this code more robust.

Comment 6

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

There are some regressions (bug 1357092), so let's not uplift until we sort those out.

Comment 7

[Mats Palmgren (inactive)]

Attachment: patch for ESR52

There was one code conflict that I had to tweak slightly and I figured
I should ask for review of that instance (I think you can ignore the rest).

At the end of CanvasRenderingContext2D::DrawImage on trunk we have this:
http://searchfox.org/mozilla-central/rev/4bd7a206dea5382c97a8a0c30beef668cc449f5b/dom/canvas/CanvasRenderingContext2D.cpp#5390-5404
where we added the error check on lines 5396-5398.

On ESR52 the same code looks like this:
https://hg.mozilla.org/releases/mozilla-esr52/annotate/tip/dom/canvas/CanvasRenderingContext2D.cpp#l4975
so I had to add a |tempTarget| there to insert that check.

Here's the interdiff of the patches in case that helps:
1702,1707c1690,1695
<  
<      AdjustedTarget tempTarget(this, bounds.IsEmpty() ? nullptr : &bounds);
<      if (!tempTarget) {
<        gfxDevCrash(LogReason::InvalidDrawTarget) << "Invalid adjusted target in Canvas2D " << gfx::hexa((DrawTarget*)mTarget) << ", " << NeedToDrawShadow() << NeedToApplyFilter();
<        return;
<      }
---
> -    AdjustedTarget(this, bounds.IsEmpty() ? nullptr : &bounds)->
> +    AdjustedTarget tempTarget(this, bounds.IsEmpty() ? nullptr : &bounds);
> +    if (!tempTarget) {
> +      aError.Throw(NS_ERROR_FAILURE);
> +      return;
> +    }
1713c1701,1702
<      tempTarget->DrawSurface(srcSurf,
---
> +    tempTarget->
>        DrawSurface(srcSurf,

https://treeherder.mozilla.org/#/jobs?repo=try&revision=85c3a94a7326915a7a1be21d0ef457c608735dc4

Comment 8

[Mats Palmgren (inactive)]

This is the chunk if I'm not mistaken:
https://bugzilla.mozilla.org/attachment.cgi?id=8858951&action=diff#a/dom/canvas/CanvasRenderingContext2D.cpp_sec48

Comment 9

[Mats Palmgren (inactive)]

(In reply to Mats Palmgren (:mats) from comment #4)
> [List of other uplifts needed for the feature/fix]:

Yes, bug 1357092.

Comment 10

[Markus Stange [:mstange]]

Comment on attachment 8858951 [details] [diff] [review]
patch for ESR52

Review of attachment 8858951 [details] [diff] [review]:
-----------------------------------------------------------------

That one adjustment seems like a reasonable change to me. Though I wonder whether the second !tempTarget check is actually necessary (both in this and in the original patch), but I guess it doesn't do any harm.

Comment 11

[Gerry Chang [:gchang]]

Comment on attachment 8857524 [details] [diff] [review]
fix

Fix a potential crash. Aurora54+.

Comment 12

[Gerry Chang [:gchang]]

Comment on attachment 8857524 [details] [diff] [review]
fix

Per comment #6, change approval‑mozilla‑aurora back to ?

Comment 13

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8857524 [details] [diff] [review]
fix

54 was merged to Beta today.

Comment 14

[Gerry Chang [:gchang]]

Comment on attachment 8857524 [details] [diff] [review]
fix

Fix a potential crash. Beta54+. Should be in 54 beta 1.

Comment 15

[Carsten Book [:Tomcat]]

needs an esr52 approval request as well i guess (at least from the attached patch)

Comment 16

[Mats Palmgren (inactive)]

Comment on attachment 8857524 [details] [diff] [review]
fix

[Approval Request Comment]
See comment 4/5.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/601f56f86809

Comment 18

[Andrei Vaida [:avaida]]

(In reply to Mats Palmgren (:mats) from comment #4)
> [User impact if declined]:crash
> [Is this code covered by automated tests?]:yes
> [Has the fix been verified in Nightly?]:no
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Mats' assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8857524 [details] [diff] [review]
fix

Meets the ESR uplift bar, ESR52.2+

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/9ee455ddcd68