Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Improve the error handling and cleanup our canvas2d code

RESOLVED FIXED in Firefox 54

Status

()

Core
Canvas: 2D
RESOLVED FIXED
3 months ago
3 months ago

People

(Reporter: Mats Palmgren (vacation - back in August), Assigned: Mats Palmgren (vacation - back in August))

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox53 wontfix, firefox54 fixed, firefox55 fixed, firefox-esr5254+ fixed)

Details

Attachments

(2 attachments)

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
Created attachment 8857524 [details] [diff] [review]
fix
Attachment #8857524 - Flags: review?(mstange)
Attachment #8857524 - Flags: review?(mstange) → review+
Keywords: checkin-needed

Comment 2

3 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/286d77c6835a
Add more consistent error handling and some code cleanup. r=mstange
Keywords: checkin-needed
Blocks: 779426

Comment 3

3 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/286d77c6835a
Status: NEW → RESOLVED
Last Resolved: 3 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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.
Attachment #8857524 - Flags: approval-mozilla-aurora?
status-firefox53: --- → wontfix
status-firefox54: --- → affected
There are some regressions (bug 1357092), so let's not uplift until we sort those out.
Depends on: 1357092
Created attachment 8858951 [details] [diff] [review]
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
Attachment #8858951 - Flags: review?(mstange)
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
(In reply to Mats Palmgren (:mats) from comment #4)
> [List of other uplifts needed for the feature/fix]:

Yes, bug 1357092.
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.
Attachment #8858951 - Flags: review?(mstange) → review+
Comment on attachment 8857524 [details] [diff] [review]
fix

Fix a potential crash. Aurora54+.
Attachment #8857524 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8857524 [details] [diff] [review]
fix

Per comment #6, change approval‑mozilla‑aurora back to ?
Attachment #8857524 - Flags: approval-mozilla-aurora+ → approval-mozilla-aurora?
Comment on attachment 8857524 [details] [diff] [review]
fix

54 was merged to Beta today.
Attachment #8857524 - Flags: approval-mozilla-aurora? → approval-mozilla-beta?
Comment on attachment 8857524 [details] [diff] [review]
fix

Fix a potential crash. Beta54+. Should be in 54 beta 1.
Attachment #8857524 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
needs an esr52 approval request as well i guess (at least from the attached patch)
Flags: needinfo?(mats)
Comment on attachment 8857524 [details] [diff] [review]
fix

[Approval Request Comment]
See comment 4/5.
Flags: needinfo?(mats)
Attachment #8857524 - Flags: approval-mozilla-esr52?

Comment 17

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/601f56f86809
status-firefox54: affected → fixed
(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.
Flags: qe-verify-
status-firefox-esr52: --- → affected
tracking-firefox-esr52: --- → 54+

Comment 19

3 months ago
Comment on attachment 8857524 [details] [diff] [review]
fix

Meets the ESR uplift bar, ESR52.2+
Attachment #8857524 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 20

3 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/9ee455ddcd68
status-firefox-esr52: affected → fixed
You need to log in before you can comment on or make changes to this bug.