see bug 160005
Created attachment 284218 [details] Patch
Comment on attachment 284218 [details] Patch r-. I think you need to look at all the other callers of IOSurface::create()
Created attachment 284301 [details] Patch
Created attachment 284303 [details] Patch
Comment on attachment 284303 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284303&action=review > Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp:157 > + if (m_data.surface) { Someday we really need to figure out how to refactor this function, it's pretty crazy. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:68 > + auto iosurface = std::unique_ptr<IOSurface>(new IOSurface(size, contextSize, colorSpace, pixelFormat, success)); it's either IOSurface or ioSurface or maybe just surface (my favorite, I think), but surely not 'iosurface'. > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:189 > + ASSERT(!success); ??? why do you care what the initial value of the out arg is? Shouldn't you just store false to it right away? This makes it weird for callers. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:1402 > + viewSnapshot->clearImage(); I don't think we want to clear the original snapshot just because we failed to compress; we should leave it alone. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4245 > + return; It's definitely not OK to not call the completion handler.
Created attachment 284330 [details] Patch
Created attachment 284334 [details] Patch
<rdar://problem/27495102>
Created attachment 284338 [details] Patch
Created attachment 284339 [details] Patch
Created attachment 284345 [details] Patch
Comment on attachment 284339 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284339&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:214 > NSLog(@"Surface creation failed for options %@", options); Let's use LOG_ALWAYS_ERROR here. Would be nice if we could also dump some stats about memory pressure (in a future patch). > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:399 > + callback(nullptr); Did you check that the callers' callbacks are able to handle a null IOSurface? > Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:410 > + if (acceleratesDrawing() && m_frontBuffer.surface) { > layer.contents = m_frontBuffer.surface->asLayerContents(); > return; > } Won't we fall through and ASSERT(!acceleratesDrawing()); on device here? I think we should set layer contents to nil and return. > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4242 > + completionHandler(nullptr); Same comment about the handlers being able to take null.
Comment on attachment 284345 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=284345&action=review > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:91 > size_t height = CGImageGetHeight(image); Did you check the createFromImage callers? > Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:407 > + if (acceleratesDrawing() && m_frontBuffer.surface) { Shouldn't the surface check be inside here? (and still return even if we don't have one) Otherwise we'll assert below (or maybe crash trying to retrieve the bitmap) if the allocation fails, right? We should probably clear the backing store in that case, instead. Probably should read like: if (acceleratesDrawing()) { layer.contents = m_frontBuffer.surface ? m_frontBuffer.surface->asLayerContents() : nil; return; }
(In reply to comment #12) > Comment on attachment 284339 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284339&action=review > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4242 > > + completionHandler(nullptr); > > Same comment about the handlers being able to take null. There are already lots of ways to call this completion handler with null.
(In reply to comment #12) > Comment on attachment 284339 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284339&action=review > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:214 > > NSLog(@"Surface creation failed for options %@", options); > > Let's use LOG_ALWAYS_ERROR here. Would be nice if we could also dump some > stats about memory pressure (in a future patch). > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:399 > > + callback(nullptr); > > Did you check that the callers' callbacks are able to handle a null > IOSurface? Yes, hence the if (convertedSurface) viewSnapshot->setSurface(WTFMove(convertedSurface)); change. > > > Source/WebKit2/Shared/mac/RemoteLayerBackingStore.mm:410 > > + if (acceleratesDrawing() && m_frontBuffer.surface) { > > layer.contents = m_frontBuffer.surface->asLayerContents(); > > return; > > } > > Won't we fall through and ASSERT(!acceleratesDrawing()); on device here? I > think we should set layer contents to nil and return. > > > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:4242 > > + completionHandler(nullptr); > > Same comment about the handlers being able to take null.
(In reply to comment #13) > Comment on attachment 284345 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=284345&action=review > > > Source/WebCore/platform/graphics/cocoa/IOSurface.mm:91 > > size_t height = CGImageGetHeight(image); > > Did you check the createFromImage callers? Yes I did. WebViewImpl::takeViewSnapshot() handles nullptr. (and IOSurface::createFromImage() already returns nullptr as an error case.)
Created attachment 284377 [details] Patch
Comment on attachment 284377 [details] Patch Clearing flags on attachment: 284377 Committed r203631: <http://trac.webkit.org/changeset/203631>
All reviewed patches have been landed. Closing bug.