Bug 134623 – [iOS][WK2] Black web view after un-suspending process

• Component: WebKit2
• Status: RESOLVED FIXED

[Tim Horton]

It is possible to get empty or volatile surfaces on-screen by suspending the process, having the surfaces get marked volatile, having memory pressure and getting the surfaces purged, then un-suspending the process. Since the view never technically left the window, we don't get a commit that reparents the view - indeed, we may not get a commit at all, so the empty/volatile surfaces will linger.

We should fix this by forcing a commit when we un-suspend, and also waiting for said commit to come in. We could in the future short-circuit this by having the UI process attempt to make the surfaces non-volatile and only requiring a commit immediately if the visible area is partially covered by empty surfaces, but for now this will work (and we don't really allow the UI process to adjust surface volatility yet).

<rdar://problem/17513223>

[Tim Horton]

Created attachment 234386 [details]
patch

[Simon Fraser (smfr)]

Comment on attachment 234386 [details]
patch

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

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1107
> +    m_viewStateChangeWantsReply = (m_viewStateChangeWantsReply == WantsReplyOrNot::DoesWantReply || wantsReplyOrNot == WantsReplyOrNot::DoesWantReply) ? WantsReplyOrNot::DoesWantReply : WantsReplyOrNot::DoesNotWantReply;

I feel like the enum is hurting readability in this code.

> Source/WebKit2/UIProcess/WebPageProxy.cpp:1176
>  #if ENABLE(INPUT_TYPE_COLOR_POPOVER)
> -        // When leaving the current page, close the popover color well.
> -        if (m_colorPicker)
> -            endColorPicker();
> +            // When leaving the current page, close the popover color well.
> +            if (m_colorPicker)
> +                endColorPicker();
>  #endif
>  #if PLATFORM(IOS)
> -        // When leaving the current page, close the video fullscreen.
> -        if (m_videoFullscreenManager)
> -            m_videoFullscreenManager->requestHideAndExitFullscreen();
> +            // When leaving the current page, close the video fullscreen.
> +            if (m_videoFullscreenManager)
> +                m_videoFullscreenManager->requestHideAndExitFullscreen();
>  #endif
> +        }

Seems like these should be factored into a onLeavingWindow() or something.

[Tim Horton]

Comment on attachment 234386 [details]
patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1107
>> +    m_viewStateChangeWantsReply = (m_viewStateChangeWantsReply == WantsReplyOrNot::DoesWantReply || wantsReplyOrNot == WantsReplyOrNot::DoesWantReply) ? WantsReplyOrNot::DoesWantReply : WantsReplyOrNot::DoesNotWantReply;
> 
> I feel like the enum is hurting readability in this code.

Agreed. I would *love* to get rid of it, so I'll do that.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1176
>> +        }
> 
> Seems like these should be factored into a onLeavingWindow() or something.

Why not!

[Tim Horton]

Comment on attachment 234386 [details]
patch

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

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1107
>> +    m_viewStateChangeWantsReply = (m_viewStateChangeWantsReply == WantsReplyOrNot::DoesWantReply || wantsReplyOrNot == WantsReplyOrNot::DoesWantReply) ? WantsReplyOrNot::DoesWantReply : WantsReplyOrNot::DoesNotWantReply;
> 
> I feel like the enum is hurting readability in this code.

Agreed. I would *love* to get rid of it, so I'll do that.

>> Source/WebKit2/UIProcess/WebPageProxy.cpp:1176
>> +        }
> 
> Seems like these should be factored into a onLeavingWindow() or something.

Why not!

[Tim Horton]

http://trac.webkit.org/changeset/170787