Closed
Bug 1355168
Opened 7 years ago
Closed 7 years ago
Crash in mozilla::dom::CanvasRenderingContext2D::DrawWindow
Categories
(Core :: Graphics: Canvas2D, defect, P1)
Core
Graphics: Canvas2D
Tracking
()
RESOLVED
FIXED
mozilla55
People
(Reporter: MatsPalmgren_bugz, Assigned: kechen)
Details
(Keywords: crash, Whiteboard: [gfx-noted])
Crash Data
Attachments
(2 files)
59 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
3.42 KB,
patch
|
ritu
:
approval-mozilla-esr52+
|
Details | Diff | Splinter Review |
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•7 years 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.
Comment 2•7 years ago
|
||
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•7 years ago
|
Flags: needinfo?(kechen)
Comment 4•7 years 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•7 years 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•7 years 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+
Comment 7•7 years ago
|
||
I should have just forwarded the review to Mats.
Updated•7 years ago
|
Assignee: nobody → kechen
Assignee | ||
Comment 8•7 years 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) |
Comment 10•7 years ago
|
||
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•7 years 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•7 years 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•7 years ago
|
||
Pushed by jacheng@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/47777ed2808d Ensure mTarget in CanvasRendering2D::DrawWindow; r=mats
Comment 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/47777ed2808d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Comment 15•7 years ago
|
||
Looks like a simple patch worth approving for Beta/ESR52?
status-firefox53:
--- → wontfix
status-firefox54:
--- → affected
status-firefox-esr52:
--- → affected
Flags: needinfo?(kechen)
Updated•7 years ago
|
Flags: needinfo?(vliu)
Assignee | ||
Comment 16•7 years ago
|
||
Sure! I will request for uplift after running the try on beta and esr52.
Flags: needinfo?(kechen)
Assignee | ||
Comment 17•7 years ago
|
||
Maybe I should find a solid way to test it before request the uplift.
Comment 18•7 years ago
|
||
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•7 years 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 20•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/52fdb2acc000
Comment 22•7 years ago
|
||
(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-
Updated•7 years ago
|
Flags: needinfo?(kechen)
Assignee | ||
Comment 24•7 years 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 25•7 years ago
|
||
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?
Comment 26•7 years ago
|
||
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•7 years ago
|
||
[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•7 years ago
|
Attachment #8858702 -
Flags: approval-mozilla-esr52?
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-
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+
Comment 30•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-esr52/rev/f45ba43512ad
Updated•2 years ago
|
Flags: needinfo?(MatsPalmgren_bugz)
You need to log in
before you can comment on or make changes to this bug.
Description
•