Summary: | [PSON] Flash on back navigation on Mac | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antti Koivisto <koivisto> | ||||||||||||
Component: | Layout and Rendering | Assignee: | 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
Antti Koivisto
2019-01-23 06:51:50 PST
Created attachment 359883 [details]
patch
Comment on attachment 359883 [details]
patch
r=me
Created attachment 359894 [details]
patch
Comment on attachment 359894 [details] patch Clearing flags on attachment: 359894 Committed r240343: <https://trac.webkit.org/changeset/240343> All reviewed patches have been landed. Closing bug. 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. Reverted r240343 for reason: Caused 4 PSON API test failures. Committed r240365: <https://trac.webkit.org/changeset/240365> 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. > 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.
(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. Created attachment 360002 [details]
patch
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 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 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. Created attachment 360019 [details]
patch
*** Bug 193770 has been marked as a duplicate of this bug. *** Comment on attachment 360019 [details] patch Clearing flags on attachment: 360019 Committed r240443: <https://trac.webkit.org/changeset/240443> All reviewed patches have been landed. Closing bug. |