Bug 193716

Summary: [PSON] Flash on back navigation on Mac
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: bfulgham, cdumez, commit-queue, ews-watchlist, ryanhaddad, simon.fraser, tsavell, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 196774    
Bug Blocks:    
Attachments:
Description Flags
patch
cdumez: review+
patch
none
patch
cdumez: review+, ews-watchlist: commit-queue-
Archive of layout-test-results from ews202 for win-future
none
patch none

Description Antti Koivisto 2019-01-23 06:51:50 PST
1. Go to news.ycombinator.com
2. Navigate to some external site
3. Hit back
Comment 1 Antti Koivisto 2019-01-23 06:52:20 PST
<rdar://problem/47148458>
Comment 2 Antti Koivisto 2019-01-23 07:48:47 PST
Created attachment 359883 [details]
patch
Comment 3 Chris Dumez 2019-01-23 08:25:19 PST
Comment on attachment 359883 [details]
patch

r=me
Comment 4 Antti Koivisto 2019-01-23 09:21:22 PST
Created attachment 359894 [details]
patch
Comment 5 WebKit Commit Bot 2019-01-23 09:43:08 PST
Comment on attachment 359894 [details]
patch

Clearing flags on attachment: 359894

Committed r240343: <https://trac.webkit.org/changeset/240343>
Comment 6 WebKit Commit Bot 2019-01-23 09:43:09 PST
All reviewed patches have been landed.  Closing bug.
Comment 7 Truitt Savell 2019-01-23 15:03:57 PST
It looks like the changes in https://trac.webkit.org/changeset/240343/webkit

has caused 4 API failures on Mac and 2 on iOS. 

Build:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/10631

API failures:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK1%20%28Tests%29/builds/10631/steps/run-api-tests/logs/stdio

I was able to reproduce these using command:
run-api-tests --root debug-240343

I reproduced all 4 API failures on Mac. running this on r240342 yields no API failures.
Comment 8 Ryan Haddad 2019-01-23 16:24:01 PST
Reverted r240343 for reason:

Caused 4 PSON API test failures.

Committed r240365: <https://trac.webkit.org/changeset/240365>
Comment 9 Chris Dumez 2019-01-23 16:30:53 PST
Comment on attachment 359894 [details]
patch

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

> Source/WebKit/ChangeLog:10
> +

I think the reason that the API tests are failing is because the logic in this patch is slightly wrong. Note that before your patch, we would keep the SuspendedPageProxy alive, even if the page did not enter page cache. If it did not enter page cache, we would merely close the WebPage on WebContent process side but the SuspendedPageProxy would stay on UIProcess side to keep the process alive so that we can reuse the process.
Comment 10 Antti Koivisto 2019-01-23 21:50:20 PST
> I think the reason that the API tests are failing is because the logic in
> this patch is slightly wrong. Note that before your patch, we would keep the
> SuspendedPageProxy alive, even if the page did not enter page cache. If it
> did not enter page cache, we would merely close the WebPage on WebContent
> process side but the SuspendedPageProxy would stay on UIProcess side to keep
> the process alive so that we can reuse the process.

So we need to hang to proxies of closed pages just to keep the process alive? It is weird that we are relying on such side effects.
Comment 11 Chris Dumez 2019-01-23 21:54:22 PST
(In reply to Antti Koivisto from comment #10)
> > I think the reason that the API tests are failing is because the logic in
> > this patch is slightly wrong. Note that before your patch, we would keep the
> > SuspendedPageProxy alive, even if the page did not enter page cache. If it
> > did not enter page cache, we would merely close the WebPage on WebContent
> > process side but the SuspendedPageProxy would stay on UIProcess side to keep
> > the process alive so that we can reuse the process.
> 
> So we need to hang to proxies of closed pages just to keep the process
> alive? It is weird that we are relying on such side effects.

It was convenient but I agree it can be confusing. Maybe we should refactor at some point.
Comment 12 Antti Koivisto 2019-01-24 02:11:31 PST
Created attachment 360002 [details]
patch
Comment 13 EWS Watchlist 2019-01-24 03:58:20 PST
Comment on attachment 360002 [details]
patch

Attachment 360002 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/10871629

New failing tests:
js/dom/custom-constructors.html
Comment 14 EWS Watchlist 2019-01-24 03:58:31 PST
Created attachment 360003 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment 15 Chris Dumez 2019-01-24 08:45:51 PST
Comment on attachment 360002 [details]
patch

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

r=me with suggestions.

> Source/WebKit/UIProcess/SuspendedPageProxy.cpp:173
> +    shouldKeepOnFailure = m_page.drawingArea() && m_page.drawingArea()->type() == DrawingAreaTypeTiledCoreAnimation;

How about shouldDelayClosingOnFailure ?

> Tools/ChangeLog:12
> +        not sufficient to gurantee we have received all the messages. Wait for them.

typo: gurantee.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:2344
> +        TestWebKitAPI::Util::run(&receivedMessage);

I think we could simply use TestWebKitAPI::Util::sleep() in the loop here.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:2439
> +        TestWebKitAPI::Util::run(&receivedMessage);

ditto.
Comment 16 Antti Koivisto 2019-01-24 09:55:43 PST
Created attachment 360019 [details]
patch
Comment 17 Chris Dumez 2019-01-24 09:56:38 PST
*** Bug 193770 has been marked as a duplicate of this bug. ***
Comment 18 WebKit Commit Bot 2019-01-24 10:35:11 PST
Comment on attachment 360019 [details]
patch

Clearing flags on attachment: 360019

Committed r240443: <https://trac.webkit.org/changeset/240443>
Comment 19 WebKit Commit Bot 2019-01-24 10:35:13 PST
All reviewed patches have been landed.  Closing bug.