WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
141567
WebGL: Destroy the GLContext when a GPU restart has been detected.
https://bugs.webkit.org/show_bug.cgi?id=141567
Summary
WebGL: Destroy the GLContext when a GPU restart has been detected.
Roger Fong
Reported
2015-02-13 10:44:02 PST
rdar://problem/18507496
Attachments
patch
(2.46 KB, patch)
2015-02-13 10:53 PST
,
Roger Fong
no flags
Details
Formatted Diff
Diff
patch
(6.49 KB, patch)
2015-02-19 15:10 PST
,
Roger Fong
dino
: review+
Details
Formatted Diff
Diff
patch
(9.34 KB, patch)
2015-02-19 18:56 PST
,
Roger Fong
no flags
Details
Formatted Diff
Diff
patch
(11.95 KB, patch)
2015-02-23 23:50 PST
,
Roger Fong
no flags
Details
Formatted Diff
Diff
patch
(16.65 KB, patch)
2015-02-24 12:53 PST
,
Roger Fong
no flags
Details
Formatted Diff
Diff
patch
(17.39 KB, patch)
2015-02-24 13:11 PST
,
Roger Fong
no flags
Details
Formatted Diff
Diff
patch
(17.43 KB, patch)
2015-02-24 13:17 PST
,
Roger Fong
no flags
Details
Formatted Diff
Diff
patch
(17.63 KB, patch)
2015-02-24 13:32 PST
,
Roger Fong
dino
: review+
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Roger Fong
Comment 1
2015-02-13 10:53:04 PST
Created
attachment 246534
[details]
patch
Dean Jackson
Comment 2
2015-02-13 11:12:21 PST
Comment on
attachment 246534
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=246534&action=review
> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:93 > + GLint restartStatus = 0; > + CGLGetParameter(glContext, kCGLCPGPURestartStatus, &restartStatus); > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > + CGLDestroyContext(glContext); > + return; > + } > +
As discussed in IRC, I think we need a little more intelligence here. 1. Check that this triggers a WebGL context lost (or make sure that this does that) 2. Ensure a page doesn't keep recreating the context in this case and submitting the same bad data. Maybe allow 3 lost contexts before crashing the process?
Roger Fong
Comment 3
2015-02-19 15:10:31 PST
Created
attachment 246923
[details]
patch
Roger Fong
Comment 4
2015-02-19 15:26:59 PST
(In reply to
comment #3
)
> Created
attachment 246923
[details]
> patch
I've now triggered a context lost event and made sure that the context does not get restored. It also adopts the same reload policy as all other webcontent crashes. 3 tries and an error.
Dean Jackson
Comment 5
2015-02-19 15:38:41 PST
Comment on
attachment 246923
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=246923&action=review
> Source/WebCore/platform/graphics/GraphicsContext3D.h:1445 > + > + WebGLRenderingContextBase* m_webglContext;
Do we ever think there might be a time when the WebGL context goes away before the GC3D?
> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:202 > + GLint abortOnBlacklist = 0; > + CGLSetParameter(m_contextObj, kCGLCPAbortOnGPURestartStatusBlacklisted, &abortOnBlacklist);
You'll need to #if around these since they are only available on OS X 10.10+
> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:180 > + GLint restartStatus = 0; > + CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus); > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > + CGLDestroyContext(_context->platformGraphicsContext3D()); > + _context->forceContextLost(); > + }
Similarly here. One concern I have is that we're only doing this test on display. Which means you could hose the system if you executed a lot of draw calls before allowing the system to draw. We might want to have this code in the drawing entry points, drawElements and drawArrays, but only test every X iterations or something. Anyway, if this makes an improvement on our hanging examples, let's stick with it.
Roger Fong
Comment 6
2015-02-19 17:58:16 PST
(In reply to
comment #5
)
> Comment on
attachment 246923
[details]
> patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=246923&action=review
> > > Source/WebCore/platform/graphics/GraphicsContext3D.h:1445 > > + > > + WebGLRenderingContextBase* m_webglContext; > > Do we ever think there might be a time when the WebGL context goes away > before the GC3D?
Hmm, if the destructor for WebGLRenderingContextBase gets called we always also destroy the associated graphicsContext3D, so this shouldn't be an issue. I believe I added a nullcheck to the forceContextLost call just in case. Maybe an assert would be more appropriate though.
> > > Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:202 > > + GLint abortOnBlacklist = 0; > > + CGLSetParameter(m_contextObj, kCGLCPAbortOnGPURestartStatusBlacklisted, &abortOnBlacklist); > > You'll need to #if around these since they are only available on OS X 10.10+ > > > Source/WebCore/platform/graphics/mac/WebGLLayer.mm:180 > > + GLint restartStatus = 0; > > + CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus); > > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > > + CGLDestroyContext(_context->platformGraphicsContext3D()); > > + _context->forceContextLost(); > > + } > > Similarly here. > > One concern I have is that we're only doing this test on display. Which > means you could hose the system if you executed a lot of draw calls before > allowing the system to draw. We might want to have this code in the drawing > entry points, drawElements and drawArrays, but only test every X iterations > or something.
How does 5 iterations sound?
> > Anyway, if this makes an improvement on our hanging examples, let's stick > with it.
Hmm if I do stick this stuff in drawArrays I feel like I won't need to have it in the display method, since the draw methods will always come first. Does that sound right? Although, I suppose, it could help though if there is only 1 draw call, we'll probably want to catch the error before having to attempt to draw more frames. (Thus in the display step), so maybe ew want both.
Roger Fong
Comment 7
2015-02-19 18:56:04 PST
Created
attachment 246934
[details]
patch
Roger Fong
Comment 8
2015-02-23 23:50:05 PST
Created
attachment 247207
[details]
patch Checks GPU status in draw method calls as well. Also does the appropriate gpu checking for iOS, which uses a slightly different API.
Roger Fong
Comment 9
2015-02-24 12:53:31 PST
Created
attachment 247256
[details]
patch
WebKit Commit Bot
Comment 10
2015-02-24 12:56:00 PST
Attachment 247256
[details]
did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:36: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:37: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Roger Fong
Comment 11
2015-02-24 13:11:29 PST
Created
attachment 247258
[details]
patch
WebKit Commit Bot
Comment 12
2015-02-24 13:14:09 PST
Attachment 247258
[details]
did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:36: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:37: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Roger Fong
Comment 13
2015-02-24 13:17:33 PST
Created
attachment 247259
[details]
patch
WebKit Commit Bot
Comment 14
2015-02-24 13:19:27 PST
Attachment 247259
[details]
did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:40: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Roger Fong
Comment 15
2015-02-24 13:32:03 PST
Created
attachment 247262
[details]
patch
WebKit Commit Bot
Comment 16
2015-02-24 13:34:13 PST
Attachment 247262
[details]
did not pass style-queue: ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:38: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:39: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] ERROR: Source/WebCore/platform/spi/ios/OpenGLESSPI.h:40: enum members should use InterCaps with an initial capital letter. [readability/enum_casing] [4] Total errors found: 3 in 11 files If any of these errors are false positives, please file a bug against check-webkit-style.
Dean Jackson
Comment 17
2015-02-24 15:31:04 PST
Comment on
attachment 247262
[details]
patch View in context:
https://bugs.webkit.org/attachment.cgi?id=247262&action=review
> Source/WebCore/platform/graphics/GraphicsContext3D.h:1278 > + // Call to make during draw calls to check on the GPU's status
Nit: missing . Naming comments coming.
> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:68 > +const int checkGPUStatusDrawIterations = 5;
Maybe GPUStatusCheckThreshold?
> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:69 > +int GraphicsContext3D::drawCounter = 0;
And checkCounter? I don't do names well, so whatever you think is best.
> Source/WebCore/platform/graphics/mac/GraphicsContext3DMac.mm:370 > +void GraphicsContext3D::checkGPUStatus()
Suggest naming this checkGPUStatusIfNecessary or something.
> Source/WebCore/platform/graphics/mac/WebGLLayer.mm:193 > + GLint restartStatus = 0; > + EAGLContext* currentContext = _context->platformGraphicsContext3D(); > + [currentContext getParameter:kEAGLCPGPURestartStatus to:&restartStatus]; > + if (restartStatus == kEAGLCPGPURestartStatusCaused || restartStatus == kEAGLCPGPURestartStatusBlacklisted) { > + [EAGLContext setCurrentContext:0]; > + [static_cast<EAGLContext*>(currentContext) release]; > + _context->forceContextLost(); > + } > #else > [super display]; > +#if __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000 > + GLint restartStatus = 0; > + CGLGetParameter(_context->platformGraphicsContext3D(), kCGLCPGPURestartStatus, &restartStatus); > + if (restartStatus == kCGLCPGPURestartStatusCaused || restartStatus == kCGLCPGPURestartStatusBlacklisted) { > + CGLSetCurrentContext(0); > + CGLDestroyContext(_context->platformGraphicsContext3D()); > + _context->forceContextLost(); > + } > +#endif
Do you think we need to test here? These calls should be fine. It's the actual draw calls that you tested above that would cause a restart. Unless you disagree, I say remove these ones.
> Source/WebCore/platform/spi/ios/OpenGLESSPI.h:2 > + * Copyright (C) 2014 Apple Inc. All rights reserved.
2015
Roger Fong
Comment 18
2015-02-24 20:20:34 PST
http://trac.webkit.org/changeset/180609
Roger Fong
Comment 19
2015-02-24 20:21:06 PST
(In reply to
comment #18
)
>
http://trac.webkit.org/changeset/180609
With Dean's fixed applied.
Ryosuke Niwa
Comment 20
2015-02-24 21:12:39 PST
Landed a Mavericks build fix attempt:
https://trac.webkit.org/changeset/180614
Roger Fong
Comment 21
2015-02-24 21:22:49 PST
(In reply to
comment #20
)
> Landed a Mavericks build fix attempt: >
https://trac.webkit.org/changeset/180614
Thanks rniwa!
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug