Closed
Bug 1254447
Opened 8 years ago
Closed 8 years ago
Release video texture that existed not matter acceleratedCanvas is disabled
Categories
(Core :: Graphics: Canvas2D, defect)
Tracking
()
RESOLVED
FIXED
mozilla48
People
(Reporter: pchang, Assigned: pchang)
References
Details
(Whiteboard: [gfx-noted])
Attachments
(2 files)
58 bytes,
text/x-review-board-request
|
jrmuizel
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
3.07 KB,
patch
|
pchang
:
review+
ritu
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
+++ This bug was initially created as a clone of Bug #1253238 comment 10 +++
Assignee | ||
Updated•8 years ago
|
Keywords: crash,
intermittent-failure
Assignee | ||
Updated•8 years ago
|
No longer blocks: e10s-tests, e10s-tests-osx
Are you saying we know this assert can trigger? Because we should then not crash :) and instead handle this case. Can we have a quick patch if the real one is going to take longer?
Assignee | ||
Comment 2•8 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #1) > Are you saying we know this assert can trigger? Because we should then not > crash :) and instead handle this case. Can we have a quick patch if the > real one is going to take longer? I think this assert should be triggered in debug build but some problem might happen early because current e10s stability is not good. From bug 1253238 comment 0, I did see the GetFeatureSGetGraphicsFeatureStatustatus failed in opt build. 00:48:17 INFO - 1812 INFO TEST-OK | dom/canvas/test/test_windingRuleUndefined.html | took 62ms 00:48:17 INFO - ###!!! [Child][MessageChannel::SendAndWait] Error: (msgtype=0x4800D8,name=PContent::Msg_GetGraphicsFeatureStatus) Channel closing: too late to send/recv, messages will be lost 00:48:17 INFO - ###!!! [Child][MessageChannel] Error: (msgtype=0x4E0001,name=PCrashReporter::Msg_AnnotateCrashReport) Channel closing: too late to send/recv, messages will be lost 00:48:17 INFO - Crash Annotation GraphicsCriticalError: |[C0][GFX1-]: Accelerated Skia canvas is disabled (t=26.8167)
Assignee | ||
Updated•8 years ago
|
Assignee: nobody → howareyou322
Assignee | ||
Comment 3•8 years ago
|
||
There is one thing to mention that it should be a timing issue to query feature status under e10s, especially during content shutdown because of channel closing. XUL!mozilla::dom::CanvasRenderingContext2D::~CanvasRenderingContext2D() [CanvasRenderingContext2D.cpp:8f45b49276ee : 985 + 0x5] 00:48:35 INFO - rbx = 0x0000000121e83000 rbp = 0x00007fff5fbfc0f0 00:48:35 INFO - rsp = 0x00007fff5fbfc0e0 r12 = 0x00007fff5fbfc178 00:48:35 INFO - r13 = 0x0000000112417000 r14 = 0x0000000000007f00 00:48:35 INFO - r15 = 0x00000000000000b2 rip = 0x0000000101d74c6e 00:48:35 INFO - Found by: call frame info 00:48:35 INFO - 2 XUL!SnowWhiteKiller::~SnowWhiteKiller() [nsCycleCollector.cpp:8f45b49276ee : 2674 + 0x6] 00:48:35 INFO - rbx = 0x000000012f7f70d8 rbp = 0x00007fff5fbfc140 00:48:35 INFO - rsp = 0x00007fff5fbfc100 r12 = 0x00007fff5fbfc178 00:48:35 INFO - r13 = 0x0000000112417000 r14 = 0x0000000000007f00 00:48:35 INFO - r15 = 0x00000000000000b2 rip = 0x000000010048e60b 00:48:35 INFO - Found by: call frame info 00:48:35 INFO - 3 XUL!nsCycleCollector::FreeSnowWhite(bool)
Assignee | ||
Updated•8 years ago
|
Summary: Release video texture if it existed but acceleratedCanvas just got disabled → Release video texture that existed not matter acceleratedCanvas is disabled
Assignee | ||
Comment 4•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=78ff17f00354
Assignee | ||
Comment 5•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/39153/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/39153/
Attachment #8728814 -
Flags: review?(jmuizelaar)
Updated•8 years ago
|
Attachment #8728814 -
Flags: review?(jmuizelaar)
Comment 6•8 years ago
|
||
Comment on attachment 8728814 [details] MozReview Request: Bug 1254447 - Don't always check UseAcceleratedCanvas when calling GetSkiaGLGlue, r?jrmuizel https://reviewboard.mozilla.org/r/39153/#review36813 Can you elaborate what is happening in this change in the commit message? i.e. explain what's broken and how it's being fixed. I also don't understand what "Release video texture that existed not matter acceleratedCanvas is disabled" means.
Assignee | ||
Comment 7•8 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #6) > Comment on attachment 8728814 [details] > MozReview Request: Bug 1254447 - Release video texture that existed not > matter acceleratedCanvas is disabled, r?jrmuizel > > https://reviewboard.mozilla.org/r/39153/#review36813 > > Can you elaborate what is happening in this change in the commit message? > i.e. explain what's broken and how it's being fixed. I also don't understand From bug 1253238 comment 1, there was an ipc error[1] because we tried to query acceleratedCanvas feature status during shutdown. I thought it was a timing issue under e10s testing. So my patch is not to query AcceleratedCanvas feature status in every gfxPlatform::GetSkiaGLGlue calls. Callers need to check skiaGL is enabled before using > what "Release video texture that existed not matter acceleratedCanvas is > disabled" means. [1] 00:48:17 INFO - ###!!! [Child][MessageChannel::SendAndWait] Error: (msgtype=0x4800D8,name=PContent::Msg_GetGraphicsFeatureStatus) Channel closing: too late to send/recv, messages will be lost
Assignee | ||
Updated•8 years ago
|
Attachment #8728814 -
Flags: review?(jmuizelaar)
Assignee | ||
Comment 8•8 years ago
|
||
Comment on attachment 8728814 [details] MozReview Request: Bug 1254447 - Don't always check UseAcceleratedCanvas when calling GetSkiaGLGlue, r?jrmuizel Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39153/diff/1-2/
Updated•8 years ago
|
Attachment #8728814 -
Flags: review?(jmuizelaar) → review+
Comment 9•8 years ago
|
||
Comment on attachment 8728814 [details] MozReview Request: Bug 1254447 - Don't always check UseAcceleratedCanvas when calling GetSkiaGLGlue, r?jrmuizel https://reviewboard.mozilla.org/r/39153/#review36851 ::: gfx/thebes/gfxPlatform.cpp:1244 (Diff revision 1) > gfxPlatform::GetSkiaGLGlue() > { > #ifdef USE_SKIA_GPU > - if (!UseAcceleratedCanvas()) { > + // Check the accelerated Canvas is enabled for the first time, > + // because the callers should check it before using. > + if (!mSkiaGlue && !UseAcceleratedCanvas()) { ::: gfx/thebes/gfxPlatform.cpp:1244 (Diff revision 2) > gfxPlatform::GetSkiaGLGlue() > { > #ifdef USE_SKIA_GPU > - if (!UseAcceleratedCanvas()) { > + // Check the accelerated Canvas is enabled for the first time, > + // because the callers should check it before using. > + if (!mSkiaGlue && !UseAcceleratedCanvas()) { This hunk can be moved inside the 'if (!mSkiaGlue)' below
Comment 10•8 years ago
|
||
Also please fix the commit message. "Release video texture that existed not matter acceleratedCanvas is disabled" isn't great english and doesn't describe most of what's going on in the patch.
Assignee | ||
Comment 11•8 years ago
|
||
Comment on attachment 8728814 [details] MozReview Request: Bug 1254447 - Don't always check UseAcceleratedCanvas when calling GetSkiaGLGlue, r?jrmuizel Review request updated; see interdiff: https://reviewboard.mozilla.org/r/39153/diff/2-3/
Attachment #8728814 -
Attachment description: MozReview Request: Bug 1254447 - Release video texture that existed not matter acceleratedCanvas is disabled, r?jrmuizel → MozReview Request: Bug 1254447 - Don't always check UseAcceleratedCanvas when calling GetSkiaGLGlue, r?jrmuizel
Assignee | ||
Comment 12•8 years ago
|
||
https://reviewboard.mozilla.org/r/39153/#review36851 > This hunk can be moved inside the 'if (!mSkiaGlue)' below done
Comment 15•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5d560d039557
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Comment 16•8 years ago
|
||
Please request Aurora and Beta uplift approval on this patch when you get a chance.
Assignee | ||
Comment 17•8 years ago
|
||
Comment on attachment 8728814 [details] MozReview Request: Bug 1254447 - Don't always check UseAcceleratedCanvas when calling GetSkiaGLGlue, r?jrmuizel Approval Request Comment [Feature/regressing bug #]:Canvas 2D [User impact if declined]: It might have texture leakage during shutdown [Describe test coverage new/current, TreeHerder]:https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5d560d039557 [Risks and why]: very low [String/UUID change made/needed]:none
Flags: needinfo?(howareyou322)
Attachment #8728814 -
Flags: approval-mozilla-aurora?
Assignee | ||
Comment 18•8 years ago
|
||
Approval Request Comment [Feature/regressing bug #]:Canvas 2D [User impact if declined]: It might have texture leakage during shutdown [Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=5d560d039557 [Risks and why]: very low [String/UUID change made/needed]: None
Attachment #8733223 -
Flags: review+
Attachment #8733223 -
Flags: approval-mozilla-beta?
Comment on attachment 8728814 [details] MozReview Request: Bug 1254447 - Don't always check UseAcceleratedCanvas when calling GetSkiaGLGlue, r?jrmuizel Fixes an intermittent failure, Aurora47+
Attachment #8728814 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8733223 [details] [diff] [review] bug-1254447-beta.patch Beta46+
Attachment #8733223 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/559e13fd5a47
Comment 22•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/963854e6252d
Should we avoid calling SkiaGLTex() if we don't have SkiaGL? Instead of this: GLuint skiaGLTex = SkiaGLTex(); if (mIsSkiaGL && skiaGLTex) { ... do this: GLuint skiaGLTex = 0; if (mIsSkiaGL) { skiaGLTex = SkiaGLTex(); } if (skiaGLTex) { ...
Flags: needinfo?(howareyou322)
Assignee | ||
Comment 24•8 years ago
|
||
(In reply to Milan Sreckovic [:milan] from comment #23) > Should we avoid calling SkiaGLTex() if we don't have SkiaGL? Instead of > this: > > GLuint skiaGLTex = SkiaGLTex(); > if (mIsSkiaGL && skiaGLTex) { > ... > > do this: > > GLuint skiaGLTex = 0; > if (mIsSkiaGL) { > skiaGLTex = SkiaGLTex(); > } > if (skiaGLTex) { > ... Create bug 1260960 to follow up.
Flags: needinfo?(howareyou322)
You need to log in
before you can comment on or make changes to this bug.
Description
•