Bug 134623 - [iOS][WK2] Black web view after un-suspending process
Summary: [iOS][WK2] Black web view after un-suspending process
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tim Horton
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-03 18:20 PDT by Tim Horton
Modified: 2015-06-19 22:18 PDT (History)
11 users (show)

See Also:


Attachments
patch (12.08 KB, patch)
2014-07-03 18:36 PDT, Tim Horton
simon.fraser: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tim Horton 2014-07-03 18:20:37 PDT
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>
Comment 1 Tim Horton 2014-07-03 18:36:25 PDT
Created attachment 234386 [details]
patch
Comment 2 Simon Fraser (smfr) 2014-07-03 22:10:34 PDT
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.
Comment 3 Tim Horton 2014-07-03 22:25:53 PDT
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!
Comment 4 Tim Horton 2014-07-03 22:25:57 PDT
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!
Comment 5 Tim Horton 2014-07-04 00:10:49 PDT
http://trac.webkit.org/changeset/170787