Bug 193716 – [PSON] Flash on back navigation on Mac

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Antti Koivisto]

1. Go to news.ycombinator.com
2. Navigate to some external site
3. Hit back

[Antti Koivisto]

<rdar://problem/47148458>

[Antti Koivisto]

Created attachment 359883 [details]
patch

[Chris Dumez]

Comment on attachment 359883 [details]
patch

r=me

[Antti Koivisto]

Created attachment 359894 [details]
patch

[WebKit Commit Bot]

Comment on attachment 359894 [details]
patch

Clearing flags on attachment: 359894

Committed r240343: <https://trac.webkit.org/changeset/240343>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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.

[Ryan Haddad]

Reverted r240343 for reason:

Caused 4 PSON API test failures.

Committed r240365: <https://trac.webkit.org/changeset/240365>

[Chris Dumez]

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.

[Antti Koivisto]

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

[Chris Dumez]

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

[Antti Koivisto]

Created attachment 360002 [details]
patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Chris Dumez]

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.

[Antti Koivisto]

Created attachment 360019 [details]
patch

[Chris Dumez]

*** Bug 193770 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

Comment on attachment 360019 [details]
patch

Clearing flags on attachment: 360019

Committed r240443: <https://trac.webkit.org/changeset/240443>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.