1318283 - Crash in mozilla::dom::CanvasRenderingContext2D::Fill

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

This bug was filed from the Socorro interface and is 
report bp-5b7b68f9-4120-42c0-8f99-d08922161117.
=============================================================

Looks new in 50. #15 topcrasher.

Comment 1

[Marco Castelluccio [:marco]]

This is for some reason more common for Russian users.
From https://crash-stats.mozilla.com/signature/?signature=mozilla%3A%3Adom%3A%3ACanvasRenderingContext2D%3A%3AFill&_columns=date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_columns=reason&_columns=address&_sort=-date&page=1#correlations:
(68.87% in signature vs 08.55% overall) useragent_locale = ru

Comment 2

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

(In reply to Marco Castelluccio [:marco] from comment #1)
> This is for some reason more common for Russian users.
> From
> https://crash-stats.mozilla.com/signature/
> ?signature=mozilla%3A%3Adom%3A%3ACanvasRenderingContext2D%3A%3AFill&_columns=
> date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_c
> olumns=reason&_columns=address&_sort=-date&page=1#correlations:
> (68.87% in signature vs 08.55% overall) useragent_locale = ru

What's the correlation with E10S?  We do disable E10S disproportionately  for locale = ru (I can't recall the reason or the bug tracking the fix.)

Comment 3

[Marco Castelluccio [:marco]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> (In reply to Marco Castelluccio [:marco] from comment #1)
> > This is for some reason more common for Russian users.
> > From
> > https://crash-stats.mozilla.com/signature/
> > ?signature=mozilla%3A%3Adom%3A%3ACanvasRenderingContext2D%3A%3AFill&_columns=
> > date&_columns=product&_columns=version&_columns=build_id&_columns=platform&_c
> > olumns=reason&_columns=address&_sort=-date&page=1#correlations:
> > (68.87% in signature vs 08.55% overall) useragent_locale = ru
> 
> What's the correlation with E10S?  We do disable E10S disproportionately 
> for locale = ru (I can't recall the reason or the bug tracking the fix.)

There's no strong correlation with e10s (it's enabled 16.56% in this signature vs 36.71% overall).

Comment 4

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Things we know:
 - mTarget should only be modified on the main thread.
 - mTarget is null in both bug 1313884 (from gfxCriticalError) and in this bug (from windbg).
 - All early returns from EnsureTarget check mTarget, or set mTarget = sErrorTarget.
 - sErrorTarget is never null for these crashes (otherwise the NS_ADDREF in EnsureErrorTarget would have segfaulted already).
 - If mTarget is null at [1], then we would have set it to sErrorTarget.
 - This bug persists from 50 to 51, in between which EnsureErrorTarget changed considerably.

That leads me to suspect these lines here:

https://dxr.mozilla.org/mozilla-release/rev/db564e184b40cfde89e36194eb48f2fed85dbc51/dom/canvas/CanvasRenderingContext2D.cpp#1677-1682

Bug 1313884 seemed to appear in 50.0b2. This bug saw serious volume in 50.0b11, but had a couple blips in 50.0b4 and 50.0b8.

Sifting through the pushlog now for b2[2], as the timeline for this bug is a bit noisy.


[1] https://dxr.mozilla.org/mozilla-release/rev/db564e184b40cfde89e36194eb48f2fed85dbc51/dom/canvas/CanvasRenderingContext2D.cpp#1652
[2] https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_50_0b1_RELEASE&tochange=FIREFOX_50_0b2_RELEASE

Comment 5

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

(In reply to Edwin Flores [:eflores] [:edwin] from comment #4)
>  - mTarget is null in both bug 1313884 (from gfxCriticalError) and in this
> bug (from windbg).

CanvasRenderingContext2D::mTarget, to be clear.

Comment 6

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

I just noticed the first patch from bug 1313884 (oops). |IsTargetValid()| is returning true, since that gfxCriticalError hasn't been hit.

That suggests that even though mTarget is null, mBufferProvider is not.

Comment 7

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Attachment: 1318283-debug.patch

I'm not proud of this patch, I don't like it, and I only intend for it to be in Nightly for as long as we need it there. But it'll get us exactly the data we need.

Comment 8

[Edwin Flores [inactive from 2016-12-01] [:eflores] [:edwin]]

Try run looks alright, modulo randoms: https://treeherder.mozilla.org/#/jobs?repo=try&revision=354be7f1ab231d7f218ec92a09f12f6f2c259199

Comment 9

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

Comment on attachment 8814490 [details] [diff] [review]
1318283-debug.patch

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

::: dom/canvas/CanvasRenderingContext2D.h
@@ +63,5 @@
> +  }
> +
> +  void MaybeCheckWrapped() {
> +    if (mEnsure && !mWrapped) {
> +      MOZ_RELEASE_ASSERT(false);

I'd make this a MOZ_CRASH with the message starting with GFX: as we look for those regularly.
Also, if this triggers, would you want to know if it was !mEnsure or mWrapped that triggered?

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f5624a5ba41e
Debug patch - r=milan

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f5624a5ba41e

Comment 12

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

https://crash-stats.mozilla.com/report/index/669a0917-bd3c-40ec-883b-534342161207

We resize canvas during the Fill() call.  Not sure that's the best.

Comment 13

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

Markus, this seems to happen as CanvasRenderingContext2D::UpdateFilter calls nsPresShell which ends up in JS and calls SetDimensions() which resets the canvas and the target goes away.  Thoughts?

Comment 14

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

Will track here, rather than in bug 1313884.

Comment 16

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

Guessing: this started with bug 927892 (part 6), and the volume went up when bug 1289975 went to beta.

Comment 17

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

Nical, what do you think about (temporarily) disabling and uplifting the optimization from bug 1289975 to see if the crash volume drops down?

Comment 18

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #17)
> Nical, what do you think about (temporarily) disabling and uplifting the
> optimization from bug 1289975 to see if the crash volume drops down?

I have no objection to doing this.

Comment 19

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

Attachment: Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Review commit: https://reviewboard.mozilla.org/r/98896/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/98896/

Comment 20

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

(In reply to Milan Sreckovic [:milan] from comment #19)
> Created attachment 8819025 [details]
> Bug 1318283: Temporarily disable an optimization from bug 1289975 and do not
> keep the buffer when SetDimension is called with the same value.
> 
> Review commit: https://reviewboard.mozilla.org/r/98896/diff/#index_header
> See other reviews: https://reviewboard.mozilla.org/r/98896/

If this lands, we should reopen bug 1289975.

Comment 21

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

(In reply to Milan Sreckovic [:milan] from comment #20)
>...
> 
> If this lands, we should reopen bug 1289975.

Also, most crashes are on nightly + beta, so the uplift would probably be necessary.

Comment 22

[Markus Stange [:mstange]]

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

https://reviewboard.mozilla.org/r/98896/#review99166

Sorry we have to do this :(

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8a55085bc211
Temporarily disable an optimization from bug 1289975 and do not keep the buffer when SetDimension is called with the same value. r=mstange

Comment 24

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

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Approval Request Comment
This is a speculative backout of bug 1289975 to understand if it was the cause of the drastic increase in frequency of this crash.  Not enough crashes on nightly to evaluate the effect.

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/8a55085bc211

Comment 26

[Julien Cristau [:jcristau]]

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

speculative backout to try and understand a crash, take in aurora52

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5d4c6e70b2c5

Comment 28

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

Marking this as fixed in 53, from comment 4

Comment 29

[Gerry Chang [:gchang]]

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Speculative backout of bug 1289975 and see if this is the cause. Beta51+. Should be in 51 beta 10.

Comment 30

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/740621a15dc4

Comment 31

[[:philipp]]

crashes with this signature are continuing in 51.0b10 and there is no apparent drop in their volume either. so it looks like the backout didn't have an effect.

Comment 32

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

Same cause as crashes in Stroke and StrokeRect.

Comment 33

[[:philipp]]

taking a closer look again, these signatures regressed in beta between 50.0b1 and 50.0b2 - changelog: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_50_0b1_RELEASE&tochange=FIREFOX_50_0b2_RELEASE

the first recorded crash on aurora (where the crashes also occur rather frequently) is from 51.0a2 build 20160929004005. so this could point to bug 1298552 as a possible source of the regression.

Comment 34

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

Indeed - I've left a comment/question in bug 1298552 for :mstange, so that we can freely talk about what that security bug was attempting to fix :)

Comment 35

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

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98896/diff/1-2/

Comment 36

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

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

MozReview made it look like this was an update of the previous patch.  Really confusing at this point.

Comment 37

[Markus Stange [:mstange]]

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

I like this patch. Thank you for debugging this!

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Should we be backing out the change from comment 25 as well?

Comment 39

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

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/98896/diff/2-3/

Comment 40

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

Updated the patch to also back out the comment 25 speculative change.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e71fdb434e7
UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch. r=mstange

Comment 42

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

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Ryan, can you make aurora/beta approval ? again so that we can consider uplifting the latest patch?  I don't seem to have the "power" to do it.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

I don't either. Liz, we're trying to reset the approval flags on attachment 8819025 [details] so we can request approval on the updated patch.

Comment 44

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

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Resetting flags from the backout.

Comment 45

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/3e71fdb434e7

Comment 46

[Julien Cristau [:jcristau]]

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

fix a crash in aurora52

Comment 47

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

Comment on attachment 8819025 [details]
Bug 1318283: UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch.

Crash fix, the first two signatures are pretty high volume, let's take it in Beta51.

Comment 48

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

https://hg.mozilla.org/releases/mozilla-beta/rev/6a017a5fdf01

Comment 49

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/16c3bb37545f

Comment 50

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

Seems this did stop the volume signature in this crash (::Fill), as I don't see any late build nightly/aurora/beta crashes.

Comment 51

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

(In reply to Carsten Book [:Tomcat] from comment #11)
> https://hg.mozilla.org/mozilla-central/rev/f5624a5ba41e

Backed out this diagnostic patch (it did the job!) in bug 1328932.

Comment 52

[Marco Castelluccio [:marco]]

"mozilla::gfx::DrawTargetSkia::Fill" was probably fixed by this too.

Comment 53

[Marco Castelluccio [:marco]]

(In reply to Marco Castelluccio [:marco] from comment #52)
> "mozilla::gfx::DrawTargetSkia::Fill" was probably fixed by this too.

More likely, bug 1331274.

Comment 54

[Calixte Denizet (:calixte)]

The crash with signature mozilla::dom::CanvasRenderingContext2D::StrokeRect has not been fixed in beta (neither in 51 nor in 52).

Version  # of crashes
52.0b1 	   13
51.0b99    29
51.0b14    24
51.0b13    7
51.0b12    14
51.0b11    15
51.0b10    20

:milan, should I open a new bug ?

Comment 55

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

Seeing the StrokeRect in 52b6 and earlier.

Comment 56

[Randell Jesup [:jesup] (needinfo me)]

https://crash-stats.mozilla.com/report/index/976c0408-8275-4798-a2e0-a9dc02170403
179 failures in the last week across all revs.

StrokeRect is failing in 53b8/etc, and the StrokeRect failures seem to go back to at least 50.x

milan - we should reopen, or file a new bug

Comment 57

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

Bug 1352295