Regression(r238817) PSON Page Cache API tests are failing

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

PSON Page Cache API tests are failing since r238817.

[Chris Dumez]

I know what's going on. I'll upload a fix shortly.

[Chris Dumez]

Created attachment 356455 [details]
Patch

[Ryosuke Niwa]

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??

[Chris Dumez]

(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.

[Ryosuke Niwa]

But if the user had 100 tabs, wouldn't this keep 100 extra processes alive under memory pressure?

[Ryosuke Niwa]

(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*

[Chris Dumez]

(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.

[Alex Christensen]

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?

[Radar WebKit Bug Importer]

<rdar://problem/46456681>

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 356516 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[WebKit Commit Bot]

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: [e-mail])
inspector/dom-debugger/event-timer-breakpoints.html bug 192368 (author: [e-mail])
inspector/console/messageAdded-from-named-evaluations.html bug 192369 (authors: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

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: [e-mail] and [e-mail])
The commit-queue is continuing to process your patch.

[Chris Dumez]

Comment on attachment 356516 [details]
Patch

Clearing flags on attachment: 356516

Committed r238867: <https://trac.webkit.org/changeset/238867>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.