Bug 179413 - Make WKFullScreenWindowController more robust against modification by the embedding app
Summary: Make WKFullScreenWindowController more robust against modification by the emb...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: iPhone / iPad Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-11-07 23:35 PST by Jeremy Jones
Modified: 2018-05-23 06:47 PDT (History)
7 users (show)

See Also:


Attachments
Patch (13.68 KB, patch)
2017-11-07 23:52 PST, Jeremy Jones
darin: review+
Details | Formatted Diff | Diff
Patch for landing. (14.48 KB, patch)
2017-11-08 13:54 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch for landing. (14.50 KB, patch)
2017-11-08 14:56 PST, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2017-11-07 23:35:16 PST
Make WKFullScreenWidnowController more robust against modification by the embedding app.
Comment 1 Jeremy Jones 2017-11-07 23:48:02 PST
rdar://problem/35408061
Comment 2 Jeremy Jones 2017-11-07 23:52:13 PST
Created attachment 326310 [details]
Patch
Comment 3 Darin Adler 2017-11-08 09:37:56 PST
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.
Comment 4 Darin Adler 2017-11-08 09:38:12 PST
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.
Comment 5 Jeremy Jones 2017-11-08 13:45:23 PST
(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.
Comment 6 Jeremy Jones 2017-11-08 13:45:58 PST
(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.
Comment 7 Jeremy Jones 2017-11-08 13:54:56 PST
Created attachment 326365 [details]
Patch for landing.
Comment 8 Jeremy Jones 2017-11-08 14:56:19 PST
Created attachment 326386 [details]
Patch for landing.
Comment 9 WebKit Commit Bot 2017-11-08 16:45:50 PST
Comment on attachment 326386 [details]
Patch for landing.

Clearing flags on attachment: 326386

Committed r224608: <https://trac.webkit.org/changeset/224608>
Comment 10 Frédéric Wang (:fredw) 2018-05-23 06:47:18 PDT
Closing bug since a patch landed.