Crash in mozilla::dom::CanvasRenderingContext2D::DrawWindow

RESOLVED FIXED in Firefox -esr52
(NeedInfo from)

Status

()

Core
Canvas: 2D
P1
critical
RESOLVED FIXED
7 months ago
3 months ago

People

(Reporter: mats, Assigned: kechen, NeedInfo)

Tracking

({crash})

unspecified
mozilla55
crash
Points:
---
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox-esr52 fixed, firefox53 wontfix, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [gfx-noted], crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

7 months ago
This bug was filed from the Socorro interface and is 
report bp-37a74ca7-df8b-412f-8f55-638602170405.
=============================================================

We have more crashes (132 in the past 7 days) than I expected for this signature,
given that it's an internal API.

Windows 7	65	49.2%
OS X 10.12	17	12.9%
Windows 10	17	12.9%
Windows XP	15	11.4%
Windows 8.1	10	7.6%
Windows Vista	4	3.0%
OS X 10.11	3	2.3%
Android	        1	0.8%


Looks like all are null-pointer crashes.
I'm guessing mTarget is null here:
https://hg.mozilla.org/mozilla-central/annotate/7a3f514cf849/dom/canvas/CanvasRenderingContext2D.cpp#l5278
due to the nsContentUtils::FlushLayoutForTree call on line 5234.
(Reporter)

Comment 1

6 months ago
I think our new Firefox Screenshots feature is using this method, so it would be nice
to get this fixed before we release that.
Hi Vincent and Kevin,

Could you please check the nullptr mTarget at:
https://hg.mozilla.org/mozilla-central/annotate/7a3f514cf849/dom/canvas/CanvasRenderingContext2D.cpp#l5278
Flags: needinfo?(vliu)
Flags: needinfo?(kechen)
Priority: -- → P1
Whiteboard: [gfx-noted]
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Flags: needinfo?(kechen)

Comment 4

6 months ago
mozreview-review
Comment on attachment 8858702 [details]
Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

https://reviewboard.mozilla.org/r/130716/#review133242
Attachment #8858702 - Flags: review?(mstange) → review+
(Reporter)

Comment 5

6 months ago
Comment on attachment 8858702 [details]
Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

Hmm, this doesn't seem like the right fix to me.

We already have an EnsureTarget() call on line 5587:
http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/dom/canvas/CanvasRenderingContext2D.cpp#5587,5591
I think the problem is that the nsContentUtils::FlushLayoutForTree call
on line 5591, which calls FlushPendingNotifications under the hood:
http://searchfox.org/mozilla-central/rev/a7334b2896ed720fba25800e11e24952e6037d77/dom/base/nsContentUtils.cpp#6613
may destroy stuff, including that target... so I think the IsTargetValid()
check should be somewhere below that flush.  On line 5593 perhaps?

BTW, shouldn't the EnsureTarget() call also be after the flush?
It seems like it should if we want it to reflect the current layout state.
Attachment #8858702 - Flags: feedback-

Comment 6

6 months ago
mozreview-review
Comment on attachment 8858702 [details]
Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

https://reviewboard.mozilla.org/r/130716/#review133248
Attachment #8858702 - Flags: review+
I should have just forwarded the review to Mats.

Updated

6 months ago
Assignee: nobody → kechen
(Assignee)

Comment 8

6 months ago
Hello Mats,

You are right, if flushing the layout after EnsureTarget and the <canvas> element is also in the window, we might set the mTarget to the null and run into the crash.
And since FlushLayoutForTree could change the layout of <canvas>, it's advisable to do the EnsureTarget after the flush.

However, I currently cannot reproduce the crash, do you have any advice in reproducing it?
Flags: needinfo?(mats)
Comment hidden (mozreview-request)
A lot of user comments (in crash reports) mentioned TabGroups addon [1].
Few comments:
 - "Crash while clicking on an empty group in the Tab Groups extension. From what I can tell, Tab Groups tried to switch to a group with zero tabs in it and exit the tab-groups view."
 - "Crashed while using Tab Groups extension GUI to reorganize open tabs into a new group."
 - "Chose "Manage Tab Groups" from the excellent tab groups add-on, and then Firefox crashed."
...

[1] https://crash-stats.mozilla.com/search/?signature=%3Dmozilla%3A%3Adom%3A%3ACanvasRenderingContext2D%3A%3ADrawWindow&product=Firefox&date=%3E%3D2016-10-18T15%3A36%3A29.000Z&date=%3C2017-04-18T15%3A36%3A29.000Z&_sort=-date&_facets=signature&_facets=user_comments&_facets=build_id&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-user_comments
(Assignee)

Comment 11

6 months ago
I've tried to reproduce it by using TabGroup addon with version 2.1.3 like the comments do on Firefox 52.0.2 on Windows 10 and Mac 10.12.2 but couldn't reproduce it currently.
(Reporter)

Comment 12

6 months ago
mozreview-review
Comment on attachment 8858702 [details]
Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

https://reviewboard.mozilla.org/r/130716/#review135404

LGTM, thanks.
Attachment #8858702 - Flags: review?(mats) → review+

Comment 13

6 months ago
Pushed by jacheng@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/47777ed2808d
Ensure mTarget in CanvasRendering2D::DrawWindow; r=mats

Comment 14

6 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/47777ed2808d
Status: NEW → RESOLVED
Last Resolved: 6 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Looks like a simple patch worth approving for Beta/ESR52?
status-firefox53: --- → wontfix
status-firefox54: --- → affected
status-firefox-esr52: --- → affected
Flags: needinfo?(kechen)

Updated

6 months ago
Flags: needinfo?(vliu)
(Assignee)

Comment 16

6 months ago
Sure! I will request for uplift after running the try on beta and esr52.
Flags: needinfo?(kechen)
(Assignee)

Comment 17

6 months ago
Maybe I should find a solid way to test it before request the uplift.
FWIW, the last crash report from trunk came from the 24-April nightly, which was the last one to not contain this fix. That at least bodes well IMO, though it might be hard to say for sure until it goes out to a wider audience. The patch seems small enough that maybe it's worth taking on Beta now while we're still early in the cycle to see whether it stops the crashes too?
Flags: needinfo?(kechen)
(Assignee)

Comment 19

6 months ago
Comment on attachment 8858702 [details]
Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

Approval Request Comment
[Feature/Bug causing the regression]:
  A problem in DrawWindow function; not a regression, this problem has existed for a while.
[User impact if declined]:
  The browser will crash.
[Is this code covered by automated tests?]:
  No.
[Has the fix been verified in Nightly?]:
  No.
  It is landed to Nightly for few days, and no related crash is found so far, but we could not ensure the crash scenario is tested since the user number is fewer in Nightly channel.
[Needs manual test from QE? If yes, steps to reproduce]: 
  No.
[List of other uplifts needed for the feature/fix]:
  No.
[Is the change risky?]:
  No.
[Why is the change risky/not risky?]:
  It's a small patch, and just makes an early return in the function.
[String changes made/needed]:
  No.
Flags: needinfo?(kechen)
Attachment #8858702 - Flags: approval-mozilla-beta?
Comment on attachment 8858702 [details]
Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

It's worth taking this in beta and see if it's really fixed. Beta54+. Should be in 54 beta 5.
Attachment #8858702 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 21

6 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/52fdb2acc000
status-firefox54: affected → fixed
(In reply to Kevin Chen[:kechen] (UTC + 8) from comment #19)
> [Is this code covered by automated tests?]:
>   No.
> [Has the fix been verified in Nightly?]:
>   No.
>   It is landed to Nightly for few days, and no related crash is found so
> far, but we could not ensure the crash scenario is tested since the user
> number is fewer in Nightly channel.
> [Needs manual test from QE? If yes, steps to reproduce]: 
>   No.

Setting qe-verify- based on Kevin's assessment on manual testing needs and the nature/volume of this crash.
Flags: qe-verify-
Flags: needinfo?(kechen)
(Assignee)

Comment 24

5 months ago
I could not reproduce this crash and write a test case currently.

If this fix is critical and there is no related crashes since we landed it to beta 54; is it advisable to uplift this esr52 first?
I will open a follow-up bug for the test case.
Flags: needinfo?(kechen) → needinfo?(ryanvm)
Comment on attachment 8858702 [details]
Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

Yeah, let's take any follow-ups to a new bug and get this uplifted to ESR52.
Flags: needinfo?(ryanvm)
Attachment #8858702 - Flags: approval-mozilla-esr52?
Doesn't apply to esr52:
merging dom/canvas/CanvasRenderingContext2D.cpp
 warning: conflicts while merging dom/canvas/CanvasRenderingContext2D.cpp! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(Assignee)

Comment 27

5 months ago
Created attachment 8869916 [details] [diff] [review]
[Uplift] Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined:
  Run into the crash.
Fix Landed on Version:
  Nightly 55, Beta 54
Risk to taking this patch (and alternatives if risky): 
  Not risky, just make an early return.
String or UUID changes made by this patch: 
  None.
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8869916 - Flags: approval-mozilla-esr52?
(Assignee)

Updated

5 months ago
Attachment #8858702 - Flags: approval-mozilla-esr52?

Comment 28

5 months ago
Comment on attachment 8869916 [details] [diff] [review]
[Uplift] Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

This has been a long standing issue on ESR releases though not at a high enough volume. The fix doesn't seem to have eliminated the all instances of this crash. I don't feel the urgency to ship this in ESR52 (given the low volume of 12 crash hits in a week)
Attachment #8869916 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52-

Updated

5 months ago
status-firefox-esr52: affected → wontfix

Comment 29

3 months ago
Comment on attachment 8869916 [details] [diff] [review]
[Uplift] Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

I was told that this fix may be fixing a more severe issue, let's uplift to ESR52.
Attachment #8869916 - Flags: approval-mozilla-esr52- → approval-mozilla-esr52+

Updated

3 months ago
status-firefox-esr52: wontfix → affected

Comment 30

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