Bug 160006 – Add success parameter to IOSurface::create() to indicate valid m_surface.

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[alan]

see bug 160005

[alan]

Created attachment 284218 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 284218 [details]
Patch

r-. I think you need to look at all the other callers of IOSurface::create()

[alan]

Created attachment 284301 [details]
Patch

[alan]

Created attachment 284303 [details]
Patch

[Tim Horton]

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.

[alan]

Created attachment 284330 [details]
Patch

[alan]

Created attachment 284334 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/27495102>

[alan]

Created attachment 284338 [details]
Patch

[alan]

Created attachment 284339 [details]
Patch

[alan]

Created attachment 284345 [details]
Patch

[Simon Fraser (smfr)]

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.

[Tim Horton]

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;
}

[Tim Horton]

(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.

[alan]

(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.

[alan]

(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.)

[alan]

Created attachment 284377 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284377 [details]
Patch

Clearing flags on attachment: 284377

Committed r203631: <http://trac.webkit.org/changeset/203631>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.