Make WKFullScreenWindowController more robust against modification by the embedding app

• Component: WebKit2
• Status: RESOLVED FIXED

[Jeremy Jones]

Make WKFullScreenWidnowController more robust against modification by the embedding app.

[Jeremy Jones]

rdar://problem/35408061

[Jeremy Jones]

Created attachment 326310 [details]
Patch

[Darin Adler]

Comment on attachment 326310 [details]
Patch

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

r=me if you fix the storage leak, but also please consider my other comments and make sure you do substantial testing

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6224
> +@implementation WKWebView (FullscreenMode)

Why put this in a separate category rather than in the main WKWebView implementation? Seems like it would be easy to accidentally write two different overrides of this method, but if in the main implementation we would get a compile time error if we made that mistake. I suppose there might be a benefit to using a category if we wanted it in a separate source file, but here it’s in the main source file with an #if.

> Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6226
> +- (void)removeFromSuperview {

Brace goes on a separate line in WebKit coding style.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:186
> -    [self setView:adoptNS([[UIView alloc] initWithFrame:[[UIScreen mainScreen] bounds]]).get()];
> +    [self setView:adoptNS([[UIView alloc] init]).get()];

Code like this has been dangerous in the past; objects with zero frames ended up in strange state later after resizing. How much have you tested with this change?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:417
> +    [[_window rootViewController] setView:[[UIView alloc] initWithFrame:[_window bounds]]];

Missing adoptNS here. I think that means we will have a storage leak.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:420
> +    [_window setWindowLevel:UIWindowLevelNormal-1];

Missing spaces around the "-" here.

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:460
> +            [protectedSelf _manager]->willEnterFullScreen();

Is there a guarantee that _manager will be non-null later when this is called back? By then the page could be null.

Cleaner way to write protectedSelf without stating the long type:

    protectedSelf = retainPtr(self)

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:569
>          [_webView _page]->setSuppressVisibilityUpdates(false);

Is there a guarantee that _page will be non-null later when this is called back?

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:596
> +- (void)webViewDidRemoveFromSuperviewWhileInFullscreen {

Brace goes on next line.

[Darin Adler]

Comment on attachment 326310 [details]
Patch

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

> Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:577
> +    if ([self isFullScreen]) {

I think this would read better with early exit.

[Jeremy Jones]

(In reply to Darin Adler from comment #3)
> Comment on attachment 326310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326310&action=review
> 
> r=me if you fix the storage leak, but also please consider my other comments
> and make sure you do substantial testing
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6224
> > +@implementation WKWebView (FullscreenMode)
> 
> Why put this in a separate category rather than in the main WKWebView
> implementation? Seems like it would be easy to accidentally write two
> different overrides of this method, but if in the main implementation we
> would get a compile time error if we made that mistake. I suppose there
> might be a benefit to using a category if we wanted it in a separate source
> file, but here it’s in the main source file with an #if.

Done. I put it with the other iOS specific methods.

> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebView.mm:6226
> > +- (void)removeFromSuperview {
> 
> Brace goes on a separate line in WebKit coding style.

Done.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:186
> > -    [self setView:adoptNS([[UIView alloc] initWithFrame:[[UIScreen mainScreen] bounds]]).get()];
> > +    [self setView:adoptNS([[UIView alloc] init]).get()];
> 
> Code like this has been dangerous in the past; objects with zero frames
> ended up in strange state later after resizing. How much have you tested
> with this change?

This isn't a problem for this particular case because of the auto layout constraints of the children of this view.

I'll change this to use an arbitrary initial size to anticipate any future, more complicated layout.

initWithFrame:CGRectMake(0, 0, 100, 100)

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:417
> > +    [[_window rootViewController] setView:[[UIView alloc] initWithFrame:[_window bounds]]];
> 
> Missing adoptNS here. I think that means we will have a storage leak.

Fixed this and audited for any other uses of +alloc without adoptNS. None found.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:420
> > +    [_window setWindowLevel:UIWindowLevelNormal-1];
> 
> Missing spaces around the "-" here.

Done.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:460
> > +            [protectedSelf _manager]->willEnterFullScreen();
> 
> Is there a guarantee that _manager will be non-null later when this is
> called back? By then the page could be null.

No. I've added a check and an early return to guard against teardown during async tasks. I audited and fixed five instances of this.

> 
> Cleaner way to write protectedSelf without stating the long type:
> 
>     protectedSelf = retainPtr(self)

Fixed in two places.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:569
> >          [_webView _page]->setSuppressVisibilityUpdates(false);
> 
> Is there a guarantee that _page will be non-null later when this is called
> back?

No. Fixed. See previous comment.

> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:596
> > +- (void)webViewDidRemoveFromSuperviewWhileInFullscreen {
> 
> Brace goes on next line.

Done.

[Jeremy Jones]

(In reply to Darin Adler from comment #4)
> Comment on attachment 326310 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=326310&action=review
> 
> > Source/WebKit/UIProcess/ios/WKFullScreenWindowControllerIOS.mm:577
> > +    if ([self isFullScreen]) {
> 
> I think this would read better with early exit.

Done.

[Jeremy Jones]

Created attachment 326365 [details]
Patch for landing.

[Jeremy Jones]

Created attachment 326386 [details]
Patch for landing.

[WebKit Commit Bot]

Comment on attachment 326386 [details]
Patch for landing.

Clearing flags on attachment: 326386

Committed r224608: <https://trac.webkit.org/changeset/224608>

[Frédéric Wang (:fredw)]

Closing bug since a patch landed.