Bug 193716 - [PSON] Flash on back navigation on Mac
Summary: [PSON] Flash on back navigation on Mac
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 193770 (view as bug list)
Depends on: 196774
Blocks:
  Show dependency treegraph
 
Reported: 2019-01-23 06:51 PST by Antti Koivisto
Modified: 2019-04-10 10:19 PDT (History)
9 users (show)

See Also:


Attachments
patch (5.53 KB, patch)
2019-01-23 07:48 PST, Antti Koivisto
cdumez: review+
Details | Formatted Diff | Diff
patch (5.55 KB, patch)
2019-01-23 09:21 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (9.39 KB, patch)
2019-01-24 02:11 PST, Antti Koivisto
cdumez: review+
ews: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews202 for win-future (12.86 MB, application/zip)
2019-01-24 03:58 PST, Build Bot
no flags Details
patch (9.31 KB, patch)
2019-01-24 09:55 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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 Build Bot 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 Build Bot 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.