Bug 227133 – Trying to take empty snapshots of the view should not cause crashes

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Trying to take empty snapshots of the view should not cause crashes.

[Chris Dumez]

Created attachment 431685 [details]
Returns a NSError

[Geoffrey Garen]

Comment on attachment 431685 [details]
Returns a NSError

View in context: https://bugs.webkit.org/attachment.cgi?id=431685&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1178
> +    if (CGRectIsEmpty(rectInViewCoordinates) || !snapshotWidth) {
> +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)]() mutable {
> +            completionHandler(nil, createNSError(WKErrorUnknown).get());
> +        });
> +        return;
> +    }

Is returning WKErrorUnknown better than returning an empty image? NSImage appears to support an empty image constructor, with the caveat that using the image later might cause you problems:

"It is permissible to initialize the image object by passing a size of (0.0, 0.0); however, you must set the size to a non-zero value before using it or an exception will be raised." [ https://developer.apple.com/documentation/appkit/nsimage/1520033-initwithsize?language=objc ]

Perhaps those problems will be clearer than WKErrorUnknown.

[Chris Dumez]

(In reply to Geoffrey Garen from comment #2)
> Comment on attachment 431685 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431685&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1178
> > +    if (CGRectIsEmpty(rectInViewCoordinates) || !snapshotWidth) {
> > +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)]() mutable {
> > +            completionHandler(nil, createNSError(WKErrorUnknown).get());
> > +        });
> > +        return;
> > +    }
> 
> Is returning WKErrorUnknown better than returning an empty image? NSImage
> appears to support an empty image constructor, with the caveat that using
> the image later might cause you problems:
> 
> "It is permissible to initialize the image object by passing a size of (0.0,
> 0.0); however, you must set the size to a non-zero value before using it or
> an exception will be raised." [
> https://developer.apple.com/documentation/appkit/nsimage/1520033-
> initwithsize?language=objc ]
> 
> Perhaps those problems will be clearer than WKErrorUnknown.

If you prefer, I can certainly do that. I tried to maintain pre-existing behavior in release builds (we would hit the assertion in debug).

[Chris Dumez]

(In reply to Chris Dumez from comment #3)
> (In reply to Geoffrey Garen from comment #2)
> > Comment on attachment 431685 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=431685&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1178
> > > +    if (CGRectIsEmpty(rectInViewCoordinates) || !snapshotWidth) {
> > > +        RunLoop::main().dispatch([completionHandler = WTFMove(completionHandler)]() mutable {
> > > +            completionHandler(nil, createNSError(WKErrorUnknown).get());
> > > +        });
> > > +        return;
> > > +    }
> > 
> > Is returning WKErrorUnknown better than returning an empty image? NSImage
> > appears to support an empty image constructor, with the caveat that using
> > the image later might cause you problems:
> > 
> > "It is permissible to initialize the image object by passing a size of (0.0,
> > 0.0); however, you must set the size to a non-zero value before using it or
> > an exception will be raised." [
> > https://developer.apple.com/documentation/appkit/nsimage/1520033-
> > initwithsize?language=objc ]
> > 
> > Perhaps those problems will be clearer than WKErrorUnknown.
> 
> If you prefer, I can certainly do that. I tried to maintain pre-existing
> behavior in release builds (we would hit the assertion in debug).

I am also unsure how to create an empty UIImage. I don't see a similar API as for NSImage.

[Chris Dumez]

Found in the context of rdar://79424132.

[Chris Dumez]

Created attachment 431692 [details]
Returns an empty image

[Tim Horton]

Comment on attachment 431692 [details]
Returns an empty image

View in context: https://bugs.webkit.org/attachment.cgi?id=431692&action=review

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180
> +            completionHandler(image.get(), nil);

I'm a little confused. The image param is nullable, shouldn't we just do that? (as a consumer of many cocoa APIs, that seems less surprising to me than this)

[Chris Dumez]

(In reply to Tim Horton from comment #7)
> Comment on attachment 431692 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=431692&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180
> > +            completionHandler(image.get(), nil);
> 
> I'm a little confused. The image param is nullable, shouldn't we just do
> that? (as a consumer of many cocoa APIs, that seems less surprising to me
> than this)

So your preference would be a nil image AND a nil NSError?

[Chris Dumez]

(In reply to Chris Dumez from comment #8)
> (In reply to Tim Horton from comment #7)
> > Comment on attachment 431692 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=431692&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180
> > > +            completionHandler(image.get(), nil);
> > 
> > I'm a little confused. The image param is nullable, shouldn't we just do
> > that? (as a consumer of many cocoa APIs, that seems less surprising to me
> > than this)
> 
> So your preference would be a nil image AND a nil NSError?

Our documentation says "The completionHandler is passed the image of the viewport contents or an error."

My first patch was passing an error, my second one is passing an empty image. I think either would be fine but we need to agree on something and fix the crashes on the bots :)

[Tim Horton]

(In reply to Chris Dumez from comment #9)
> (In reply to Chris Dumez from comment #8)
> > (In reply to Tim Horton from comment #7)
> > > Comment on attachment 431692 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=431692&action=review
> > > 
> > > > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:1180
> > > > +            completionHandler(image.get(), nil);
> > > 
> > > I'm a little confused. The image param is nullable, shouldn't we just do
> > > that? (as a consumer of many cocoa APIs, that seems less surprising to me
> > > than this)
> > 
> > So your preference would be a nil image AND a nil NSError?
> 
> Our documentation says "The completionHandler is passed the image of the
> viewport contents or an error."
> 
> My first patch was passing an error, my second one is passing an empty
> image. I think either would be fine but we need to agree on something and
> fix the crashes on the bots :)

Ahaa, fair :) I think either is definitely fine.

[Tim Horton]

I guess.. you did ask for an empty image, and you got an empty image, so I will r+ that one.

[EWS]

Committed r279006 (238931@main): <https://commits.webkit.org/238931@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 431692 [details].

[Radar WebKit Bug Importer]

<rdar://problem/79468888>

[Geoffrey Garen]

> I guess.. you did ask for an empty image, and you got an empty image

FWIW, that was my thinking too.