Bug 192348 - Regression(r238817) PSON Page Cache API tests are failing
Summary: Regression(r238817) PSON Page Cache API tests are failing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 192318
  Show dependency treegraph
 
Reported: 2018-12-03 20:15 PST by Chris Dumez
Modified: 2018-12-04 11:44 PST (History)
11 users (show)

See Also:


Attachments
Patch (10.18 KB, patch)
2018-12-03 21:04 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (10.20 KB, patch)
2018-12-04 10:46 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews102 for mac-sierra (2.58 MB, application/zip)
2018-12-04 11:27 PST, Build Bot
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-12-03 20:15:51 PST
PSON Page Cache API tests are failing since r238817.
Comment 1 Chris Dumez 2018-12-03 20:18:21 PST
I know what's going on. I'll upload a fix shortly.
Comment 2 Chris Dumez 2018-12-03 21:04:18 PST
Created attachment 356455 [details]
Patch
Comment 3 Ryosuke Niwa 2018-12-03 21:51:20 PST
Comment on attachment 356455 [details]
Patch

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

> Source/WebKit/ChangeLog:16
> +        To address the issue, when a WebProcess is about to get suspended, we check if
> +        the process has any suspended WebPage (WebPage used for PSON PageCache support)
> +        and we bypass the PageCache clearing if it does.

But don't we need to clear the page cache when a process which has a suspended WebPage goes to background??
Comment 4 Chris Dumez 2018-12-03 22:15:47 PST
(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 356455 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356455&action=review
> 
> > Source/WebKit/ChangeLog:16
> > +        To address the issue, when a WebProcess is about to get suspended, we check if
> > +        the process has any suspended WebPage (WebPage used for PSON PageCache support)
> > +        and we bypass the PageCache clearing if it does.
> 
> But don't we need to clear the page cache when a process which has a
> suspended WebPage goes to background??

Suspended page cache is currently handled at ui process level. We have at most 2 such processes and they respond to memory pressure. I am not convinced we need any extra handling at this point.
Comment 5 Ryosuke Niwa 2018-12-03 22:19:58 PST
But if the user had 100 tabs, wouldn't this keep 100 extra processes alive under memory pressure?
Comment 6 Ryosuke Niwa 2018-12-03 22:21:23 PST
(In reply to Ryosuke Niwa from comment #5)
> But if the user had 100 tabs, wouldn't this keep 100 extra processes alive
> under memory pressure?

I meant to say don't we forget to clear the page cache in 100 extra processes*
Comment 7 Chris Dumez 2018-12-04 06:48:46 PST
(In reply to Ryosuke Niwa from comment #6)
> (In reply to Ryosuke Niwa from comment #5)
> > But if the user had 100 tabs, wouldn't this keep 100 extra processes alive
> > under memory pressure?
> 
> I meant to say don't we forget to clear the page cache in 100 extra
> processes*

My patch only bypasses page cache clearing in the 2 suspended page processes.
Comment 8 Alex Christensen 2018-12-04 09:53:56 PST
Comment on attachment 356455 [details]
Patch

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

> Source/WebCore/page/MemoryRelease.h:32
> +enum class MaintainPageCache { No, Yes };

: bool

> Source/WebKit/WebProcess/WebProcess.cpp:284
> +            auto maintainPageCache = m_isSuspending && hasPageRequiringPageCacheWhileSuspended() ? WebCore::MaintainPageCache::Yes : WebCore::MaintainPageCache::No;

Will m_isSuspending really be true during web process initialization?
Comment 9 Radar WebKit Bug Importer 2018-12-04 10:16:20 PST
<rdar://problem/46456681>
Comment 10 Chris Dumez 2018-12-04 10:44:04 PST
Comment on attachment 356455 [details]
Patch

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

>> Source/WebKit/WebProcess/WebProcess.cpp:284
>> +            auto maintainPageCache = m_isSuspending && hasPageRequiringPageCacheWhileSuspended() ? WebCore::MaintainPageCache::Yes : WebCore::MaintainPageCache::No;
> 
> Will m_isSuspending really be true during web process initialization?

No.
Comment 11 Chris Dumez 2018-12-04 10:46:28 PST
Created attachment 356516 [details]
Patch
Comment 12 Build Bot 2018-12-04 11:26:59 PST
Comment on attachment 356516 [details]
Patch

Attachment 356516 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10266737

New failing tests:
inspector/console/console-log-proxy.html
imported/w3c/web-platform-tests/encoding/legacy-mb-korean/euc-kr/euckr-encode-form-ks_c_5601-1987.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/15.1.3/15.1.3.3_encodeURI/S15.1.3.3_A1.1_T2.html
sputnik/Conformance/15_Native_Objects/15.1_The_Global_Object/15.1.3/15.1.3.3_encodeURI/S15.1.3.3_A2.3_T1.html
Comment 13 Build Bot 2018-12-04 11:27:01 PST
Created attachment 356518 [details]
Archive of layout-test-results from ews102 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 14 WebKit Commit Bot 2018-12-04 11:39:04 PST
The commit-queue encountered the following flaky tests while processing attachment 356516 [details]:

webgl/2.0.0/conformance/more/conformance/quickCheckAPI-B3.html bug 192367 (author: justin_fan@apple.com)
inspector/dom-debugger/event-timer-breakpoints.html bug 192368 (author: drousso@apple.com)
inspector/console/messageAdded-from-named-evaluations.html bug 192369 (authors: drousso@apple.com and joepeck@webkit.org)
The commit-queue is continuing to process your patch.
Comment 15 WebKit Commit Bot 2018-12-04 11:39:16 PST
The commit-queue encountered the following flaky tests while processing attachment 356516 [details]:

imported/w3c/web-platform-tests/dom/ranges/Range-intersectsNode.html bug 192370 (authors: cdumez@apple.com and youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 16 Chris Dumez 2018-12-04 11:44:15 PST
Comment on attachment 356516 [details]
Patch

Clearing flags on attachment: 356516

Committed r238867: <https://trac.webkit.org/changeset/238867>
Comment 17 Chris Dumez 2018-12-04 11:44:17 PST
All reviewed patches have been landed.  Closing bug.