We should not need to navigate to 'about:blank' to suspend pages to support PageCache when PSON is enabled.
<rdar://problem/46701466>
Created attachment 357238 [details] WIP Patch
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.
Created attachment 357244 [details] Patch
Created attachment 357251 [details] Patch
Comment on attachment 357251 [details] Patch Clearing flags on attachment: 357251 Committed r239182: <https://trac.webkit.org/changeset/239182>
All reviewed patches have been landed. Closing bug.
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)?
(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.
> 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.