Bug 192668 – [PSON] We should not need to navigate to 'about:blank' to suspend pages

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

We should not need to navigate to 'about:blank' to suspend pages to support PageCache when PSON is enabled.

[Radar WebKit Bug Importer]

<rdar://problem/46701466>

[Chris Dumez]

Created attachment 357238 [details]
WIP Patch

[EWS Watchlist]

Attachment 357238 [details] did not pass style-queue:


ERROR: Source/WebKit/WebProcess/WebPage/WebDocumentLoader.cpp:53:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:1333:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 2 in 16 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 357244 [details]
Patch

[Chris Dumez]

Created attachment 357251 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 357251 [details]
Patch

Clearing flags on attachment: 357251

Committed r239182: <https://trac.webkit.org/changeset/239182>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antti Koivisto]

Comment on attachment 357251 [details]
Patch

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

Nice!

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-739
> -    if (request.url() == WTF::blankURL() && webPage->isSuspended()) {

Does isSuspended bit on WebPage have any clients after this? Maybe it is not even needed anymore?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:1331
> +    if (!PageCache::singleton().addIfCacheable(*currentHistoryItem, corePage())) {
> +        failedToSuspend();
> +        return;
> +    }

So this leaves us in a new type of state where we the current page is moved to page cache but we haven't navigated anywhere. I suppose that is ok.

Does failedToSuspend() throws away the process? Could we just close the page when caching fails but keep the process around (since it has useful caches etc for back navigation)?

[Chris Dumez]

(In reply to Antti Koivisto from comment #8)
> Comment on attachment 357251 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=357251&action=review
> 
> Nice!
> 
> > Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:-739
> > -    if (request.url() == WTF::blankURL() && webPage->isSuspended()) {
> 
> Does isSuspended bit on WebPage have any clients after this? Maybe it is not
> even needed anymore?

There is still one call site:
bool WebProcess::hasPageRequiringPageCacheWhileSuspended() const
{
    for (auto& page : m_pageMap.values()) {
        if (page->isSuspended())
            return true;
    }
    return false;
}

We need to know a page is suspended for process-swap when the process actually gets suspended on iOS to avoid clearing the PageCache.

> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:1331
> > +    if (!PageCache::singleton().addIfCacheable(*currentHistoryItem, corePage())) {
> > +        failedToSuspend();
> > +        return;
> > +    }
> 
> So this leaves us in a new type of state where we the current page is moved
> to page cache but we haven't navigated anywhere. I suppose that is ok.

Yes, it is kind of a new state but it seems to work OK, and navigating to the current history item works fine too and restores it from PageCache.

> 
> Does failedToSuspend() throws away the process? Could we just close the page
> when caching fails but keep the process around (since it has useful caches
> etc for back navigation)?

Yes, I am not keeping the process around anymore if page cache fails. I have discussed this with Brady and Geoff as well.
If it turns out to be an issue, then we can do better and cache processes. It is not quite as easy as it sounds though because
if you re-attach to a process that used to have a WebPage with your ID, you may get leftover IPC from the previous page when
you reconnect and it would trigger MESSAGE_CHECK failures in the UIProcess. So we'd still need some kind of handshake like
Close -> DoneClosing before we would be able to reuse the process.

[Antti Koivisto]

> Yes, I am not keeping the process around anymore if page cache fails. I have
> discussed this with Brady and Geoff as well.
> If it turns out to be an issue, then we can do better and cache processes.
> It is not quite as easy as it sounds though because
> if you re-attach to a process that used to have a WebPage with your ID, you
> may get leftover IPC from the previous page when
> you reconnect and it would trigger MESSAGE_CHECK failures in the UIProcess.
> So we'd still need some kind of handshake like
> Close -> DoneClosing before we would be able to reuse the process.

Makes sense. A suspended page is always in this specific page cache-like state. A hypothetical per-domain process cache would handle other cases.