Bug 133664

Summary: [iOS] Clear UIProcess visual state after WebProcess crash
Product: WebKit Reporter: Martin Hock <mhock>
Component: WebKit2Assignee: Martin Hock <mhock>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, thorton
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
benjamin: review-
address comments
benjamin: review+, benjamin: commit-queue-
address comment + fix mac build none

Description Martin Hock 2014-06-09 16:38:26 PDT
[iOS] Clear UIProcess visual state after WebProcess crash
Comment 1 Martin Hock 2014-06-09 16:40:49 PDT
Created attachment 232747 [details]
patch
Comment 2 Martin Hock 2014-06-09 16:42:11 PDT
<rdar://problem/16952742>
Comment 3 Benjamin Poulain 2014-06-09 19:25:42 PDT
Comment on attachment 232747 [details]
patch

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

> Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:641
> +    _overridesMinimumLayoutSize = NO;
> +    _minimumLayoutSizeOverride = {0, 0};
> +    _overridesMinimumLayoutSizeForMinimalUI = NO;
> +    _minimumLayoutSizeOverrideForMinimalUI = {0, 0};
> +    _overridesMaximumUnobscuredSize = NO;
> +    _maximumUnobscuredSizeOverride = {0, 0};
> +    _usesMinimalUI = NO;
> +    _needsToNotifyDelegateAboutMinimalUI = NO;
> +    _inputViewBounds = {{0, 0}, {0, 0}};

Most of those are defined by external factors, they should not be reset on crash!

> Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:147
> +    for (auto& idLayer : m_layers) {
> +        m_animationDelegates.remove(idLayer.key);
> +        asLayer(idLayer.value.get()).contents = nullptr;
> +    }

Shouldn't we remove the sublayers before clearing the root?
Comment 4 Martin Hock 2014-06-11 13:56:20 PDT
Created attachment 232898 [details]
address comments
Comment 5 Martin Hock 2014-06-11 13:58:58 PDT
(In reply to comment #3)
> (From update of attachment 232747 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=232747&action=review
> 
> > Source/WebKit2/UIProcess/API/Cocoa/WKWebView.mm:641
> > +    _overridesMinimumLayoutSize = NO;
> > +    _minimumLayoutSizeOverride = {0, 0};
> > +    _overridesMinimumLayoutSizeForMinimalUI = NO;
> > +    _minimumLayoutSizeOverrideForMinimalUI = {0, 0};
> > +    _overridesMaximumUnobscuredSize = NO;
> > +    _maximumUnobscuredSizeOverride = {0, 0};
> > +    _usesMinimalUI = NO;
> > +    _needsToNotifyDelegateAboutMinimalUI = NO;
> > +    _inputViewBounds = {{0, 0}, {0, 0}};
> 
> Most of those are defined by external factors, they should not be reset on crash!

Yes, I should be pushing instead. Fixed.

> > Source/WebKit2/UIProcess/mac/RemoteLayerTreeHost.mm:147
> > +    for (auto& idLayer : m_layers) {
> > +        m_animationDelegates.remove(idLayer.key);
> > +        asLayer(idLayer.value.get()).contents = nullptr;
> > +    }
> 
> Shouldn't we remove the sublayers before clearing the root?

I followed the same order as updateLayerTree which implies that the root is updated before the layers, but changed this anyway as the custom is to destroy in opposite order of creation.
Comment 6 Benjamin Poulain 2014-06-11 14:25:36 PDT
Comment on attachment 232898 [details]
address comments

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

> Source/WebKit2/UIProcess/ios/WKContentView.mm:387
> +    [_webView _didRelaunchProcess];

You should do this from the iOS's PageClient instead. No need to push ContentView->WebView here.
Comment 7 Martin Hock 2014-06-11 14:53:12 PDT
Created attachment 232908 [details]
address comment + fix mac build
Comment 8 Martin Hock 2014-06-11 15:50:09 PDT
Committed r169851: <http://trac.webkit.org/changeset/169851>