Closed Bug 1318283 Opened 8 years ago Closed 7 years ago

Crash in mozilla::dom::CanvasRenderingContext2D::Fill

Categories

(Core :: Graphics, defect)

50 Branch
x86
Windows 8
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: eflores, Assigned: milan)

References

Details

(Keywords: crash, regression, Whiteboard: [gfx-noted])

Crash Data

Attachments

(2 files)

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

Looks new in 50. #15 topcrasher.
Whiteboard: [gfx-noted]
(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.)
(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).
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
(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.
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.
Attached patch 1318283-debug.patch — — Splinter Review
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.
Assignee: nobody → edwin
Attachment #8814490 - Flags: review?(milan)
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?
Attachment #8814490 - Flags: review?(milan) → review+
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?
Flags: needinfo?(mstange)
Will track here, rather than in bug 1313884.
Assignee: edwin.bugs → nobody
Keywords: regression
Crash Signature: [@ mozilla::dom::CanvasRenderingContext2D::Fill] → [@ mozilla::dom::CanvasRenderingContext2D::Fill] [@ mozilla::dom::CanvasRenderingContext2D::DrawImage]
Guessing: this started with bug 927892 (part 6), and the volume went up when bug 1289975 went to beta.
See Also: → 1289975, 927892
Nical, what do you think about (temporarily) disabling and uplifting the optimization from bug 1289975 to see if the crash volume drops down?
Flags: needinfo?(nical.bugzilla)
(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.
Flags: needinfo?(nical.bugzilla)
(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.
(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 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 :(
Attachment #8819025 - Flags: review?(mstange) → review+
Pushed by msreckovic@mozilla.com:
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 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.
Attachment #8819025 - Flags: approval-mozilla-beta?
Attachment #8819025 - Flags: approval-mozilla-aurora?
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
Attachment #8819025 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Marking this as fixed in 53, from comment 4
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.
Attachment #8819025 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
Flags: needinfo?(milan)
Crash Signature: [@ mozilla::dom::CanvasRenderingContext2D::Fill] [@ mozilla::dom::CanvasRenderingContext2D::DrawImage] → [@ mozilla::dom::CanvasRenderingContext2D::Fill] [@ mozilla::dom::CanvasRenderingContext2D::DrawImage] [@ mozilla::dom::CanvasRenderingContext2D::Stroke] [@ mozilla::dom::CanvasRenderingContext2D::StrokeRect]
Flags: needinfo?(milan)
Same cause as crashes in Stroke and StrokeRect.
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.
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 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.
Attachment #8819025 - Flags: review+ → review?(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!
Flags: needinfo?(mstange)
Attachment #8819025 - Flags: review?(mstange) → review+
Should we be backing out the change from comment 25 as well?
Assignee: nobody → milan
Flags: needinfo?(milan)
Updated the patch to also back out the comment 25 speculative change.
Flags: needinfo?(milan)
Pushed by msreckovic@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3e71fdb434e7
UpdateFilter can sometimes invalidate the draw target. Back out the previous speculative patch. r=mstange
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.
Flags: needinfo?(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.
Flags: needinfo?(ryanvm) → needinfo?(lhenry)
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.
Flags: needinfo?(lhenry)
Attachment #8819025 - Flags: approval-mozilla-beta?
Attachment #8819025 - Flags: approval-mozilla-beta+
Attachment #8819025 - Flags: approval-mozilla-aurora?
Attachment #8819025 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/3e71fdb434e7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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
Attachment #8819025 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8819025 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Seems this did stop the volume signature in this crash (::Fill), as I don't see any late build nightly/aurora/beta crashes.
See Also: → 1331274
(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.
See Also: → 1328932
"mozilla::gfx::DrawTargetSkia::Fill" was probably fixed by this too.
Crash Signature: [@ mozilla::dom::CanvasRenderingContext2D::Fill] [@ mozilla::dom::CanvasRenderingContext2D::DrawImage] [@ mozilla::dom::CanvasRenderingContext2D::Stroke] [@ mozilla::dom::CanvasRenderingContext2D::StrokeRect] → [@ mozilla::dom::CanvasRenderingContext2D::Fill] [@ mozilla::dom::CanvasRenderingContext2D::DrawImage] [@ mozilla::dom::CanvasRenderingContext2D::Stroke] [@ mozilla::dom::CanvasRenderingContext2D::StrokeRect] [@ mozilla::gfx::DrawTargetSkia::Fill]
(In reply to Marco Castelluccio [:marco] from comment #52)
> "mozilla::gfx::DrawTargetSkia::Fill" was probably fixed by this too.

More likely, bug 1331274.
Crash Signature: [@ mozilla::dom::CanvasRenderingContext2D::Fill] [@ mozilla::dom::CanvasRenderingContext2D::DrawImage] [@ mozilla::dom::CanvasRenderingContext2D::Stroke] [@ mozilla::dom::CanvasRenderingContext2D::StrokeRect] [@ mozilla::gfx::DrawTargetSkia::Fill] → [@ mozilla::dom::CanvasRenderingContext2D::Fill] [@ mozilla::dom::CanvasRenderingContext2D::DrawImage] [@ mozilla::dom::CanvasRenderingContext2D::Stroke] [@ mozilla::dom::CanvasRenderingContext2D::StrokeRect]
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 ?
Flags: needinfo?(milan)
Seeing the StrokeRect in 52b6 and earlier.
Flags: needinfo?(milan)
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
Flags: needinfo?(milan)
Bug 1352295
Flags: needinfo?(milan)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: