rdar://problem/18507496
Created attachment 246534 [details] patch
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?
Created attachment 246923 [details] patch
(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.
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.
(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.
Created attachment 246934 [details] patch
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.
Created attachment 247256 [details] patch
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.
Created attachment 247258 [details] patch
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.
Created attachment 247259 [details] patch
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.
Created attachment 247262 [details] patch
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.
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
http://trac.webkit.org/changeset/180609
(In reply to comment #18) > http://trac.webkit.org/changeset/180609 With Dean's fixed applied.
Landed a Mavericks build fix attempt: https://trac.webkit.org/changeset/180614
(In reply to comment #20) > Landed a Mavericks build fix attempt: > https://trac.webkit.org/changeset/180614 Thanks rniwa!