Scroll position is sometimes not restored on history navigation when PSON is enabled.
<rdar://problem/48216125>
Created attachment 362671 [details] Patch
Comment on attachment 362671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362671&action=review > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2710 > + SetForScope<void (*)(WebCore::HistoryItem&)> bypassHistoryItemUpdateNotifications(WebCore::notifyHistoryItemChanged, ignoreHistoryItemUpdates); ignoreHistoryItemUpdates could be replaced by my favorite lambda [](auto){} > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4837 > TEST(ProcessSwap, ScrollPositionRestoration) This test passes for me in debug before the change. Is that expected?
Comment on attachment 362671 [details] Patch A release build also shows the test doesn't exercise the fix.
(In reply to Alex Christensen from comment #4) > Comment on attachment 362671 [details] > Patch > > A release build also shows the test doesn't exercise the fix. Did you try on iOS? It definitely reproduced the bug for me with the iOS simulator.
Comment on attachment 362671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362671&action=review Not sure why the r- >> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2710 >> + SetForScope<void (*)(WebCore::HistoryItem&)> bypassHistoryItemUpdateNotifications(WebCore::notifyHistoryItemChanged, ignoreHistoryItemUpdates); > > ignoreHistoryItemUpdates could be replaced by my favorite lambda [](auto){} Will this work even though it expects a function pointer?
Created attachment 362681 [details] Patch
Attachment 362681 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:2708: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:2708: Missing space inside { }. [whitespace/braces] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 362681 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362681&action=review This seems sensible but I'd like someone familiar with the relevant code review it.W > Source/WebKit/ChangeLog:14 > + wuch as the scroll position. Nit: wuch. > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4863 > + TestWebKitAPI::Util::sleep(0.5); What's up with all these sleeps?
(In reply to Ryosuke Niwa from comment #9) > Comment on attachment 362681 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=362681&action=review > > This seems sensible but I'd like someone familiar with the relevant code > review it.W > > > Source/WebKit/ChangeLog:14 > > + wuch as the scroll position. > > Nit: wuch. > Whoops. > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4863 > > + TestWebKitAPI::Util::sleep(0.5); > > What's up with all these sleeps? Need to give some time for the ipc to get received by the uiprocess.
Created attachment 362687 [details] Patch
Attachment 362687 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:2708: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:2708: Missing space inside { }. [whitespace/braces] [5] Total errors found: 2 in 4 files If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to Chris Dumez from comment #10) > > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4863 > > > + TestWebKitAPI::Util::sleep(0.5); > > > > What's up with all these sleeps? > > Need to give some time for the ipc to get received by the uiprocess. There's no way to round-trip IPC to web content process to ensure such a state? Relying on a sleep makes me worry that this test might become flaky in the future...
(In reply to Ryosuke Niwa from comment #13) > (In reply to Chris Dumez from comment #10) > > > > > > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4863 > > > > + TestWebKitAPI::Util::sleep(0.5); > > > > > > What's up with all these sleeps? > > > > Need to give some time for the ipc to get received by the uiprocess. > > There's no way to round-trip IPC to web content process to ensure such a > state? Relying on a sleep makes me worry that this test might become flaky > in the future... I agree it is not ideal but I was already happy to be able to write a test for this. I will try to find a more robust way tomorrow.
Created attachment 362689 [details] Patch
Comment on attachment 362671 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362671&action=review >>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:2710 >>> + SetForScope<void (*)(WebCore::HistoryItem&)> bypassHistoryItemUpdateNotifications(WebCore::notifyHistoryItemChanged, ignoreHistoryItemUpdates); >> >> ignoreHistoryItemUpdates could be replaced by my favorite lambda [](auto){} > > Will this work even though it expects a function pointer? Your proposal did not build with WPE.
Attachment 362689 [details] did not pass style-queue: ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:2708: Missing space before { [whitespace/braces] [5] ERROR: Source/WebKit/WebProcess/WebPage/WebPage.cpp:2708: Missing space inside { }. [whitespace/braces] [5] Total errors found: 2 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.
Woohoo found something that builds on WPE.
Comment on attachment 362689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362689&action=review r=me > Source/WebKit/ChangeLog:10 > + call restoreSessionInternal() to restore the HistoryItems based on the UIProcess' UIProcess's > Source/WebKit/ChangeLog:12 > + updates back to the UIProcess. Without PSON, this would be unnecessary but harmless. When did this ever happen without PSON? (I think only PSON creates a new WebPage for backforward navigation.) > Source/WebKit/WebProcess/WebPage/WebPage.cpp:2705 > + // Since we're merely restoring HistoryItems from the UIProcess, there is no need to send HistoryItem update notifications back to the UIProcess. I would comment more strongly: Avoid sending HistoryItem updates to the UIProcess because we may overwrite important in-flight state from the UI process (like scroll position). > Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:4816 > +// Pages with dedicated workers do not go into page cache. > +var myWorker = new Worker('worker.js'); Seems like it would be nice to add window.internals.disablePageCache() for cases like this.
(In reply to Chris Dumez from comment #6) > Will this work even though it expects a function pointer? Lambdas with no captures can be used as function pointers. Lambdas with auto parameters are kind of like templates, which was probably what was causing problems with WPE.
Comment on attachment 362689 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=362689&action=review >> Source/WebKit/ChangeLog:12 >> + updates back to the UIProcess. Without PSON, this would be unnecessary but harmless. > > When did this ever happen without PSON? (I think only PSON creates a new WebPage for backforward navigation.) After a crash.
Committed r241950: <https://trac.webkit.org/changeset/241950>