1355168 - Crash in mozilla::dom::CanvasRenderingContext2D::DrawWindow

Status: RESOLVED FIXED

(Core :: Graphics: Canvas2D, defect, P1)

Description

[Mats Palmgren (inactive)]

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.

Comment 1

[Mats Palmgren (inactive)]

I think our new Firefox Screenshots feature is using this method, so it would be nice
to get this fixed before we release that.

Comment 2

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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

Comment 3

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: Bug 1355168 - Ensure mTarget in CanvasRendering2D::DrawWindow;

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

Comment 4

[Markus Stange [:mstange]]

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

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

Comment 5

[Mats Palmgren (inactive)]

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.

Comment 6

[Markus Stange [:mstange]]

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

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

Comment 7

[Markus Stange [:mstange]]

I should have just forwarded the review to Mats.

Comment 8

[Kevin Chen[:kechen] (UTC + 8)]

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?

Comment 9

[Kevin Chen[:kechen] (UTC + 8)]

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

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

Comment 10

[Calixte Denizet (:calixte)]

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

Comment 11

[Kevin Chen[:kechen] (UTC + 8)]

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.

Comment 12

[Mats Palmgren (inactive)]

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

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

LGTM, thanks.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47777ed2808d
Ensure mTarget in CanvasRendering2D::DrawWindow; r=mats

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/47777ed2808d

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Looks like a simple patch worth approving for Beta/ESR52?

Comment 16

[Kevin Chen[:kechen] (UTC + 8)]

Sure! I will request for uplift after running the try on beta and esr52.

Comment 17

[Kevin Chen[:kechen] (UTC + 8)]

Maybe I should find a solid way to test it before request the uplift.

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 19

[Kevin Chen[:kechen] (UTC + 8)]

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.

Comment 20

[Gerry Chang [:gchang]]

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.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/52fdb2acc000

Comment 22

[Andrei Vaida [:avaida]]

(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.

Comment 24

[Kevin Chen[:kechen] (UTC + 8)]

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.

Comment 25

[Ryan VanderMeulen [: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.

Comment 26

[Sylvestre Ledru [:Sylvestre]]

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

Comment 27

[Kevin Chen[:kechen] (UTC + 8)]

Attachment: [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.

Comment 28

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

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)

Comment 29

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

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.

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/f45ba43512ad