Bug 194924 – REGRESSION(PSON) Scroll position is sometimes not restored on history navigation

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

Scroll position is sometimes not restored on history navigation when PSON is enabled.

[Chris Dumez]

<rdar://problem/48216125>

[Chris Dumez]

Created attachment 362671 [details]
Patch

[Alex Christensen]

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?

[Alex Christensen]

Comment on attachment 362671 [details]
Patch

A release build also shows the test doesn't exercise the fix.

[Chris Dumez]

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

[Chris Dumez]

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?

[Chris Dumez]

Created attachment 362681 [details]
Patch

[EWS Watchlist]

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.

[Ryosuke Niwa]

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?

[Chris Dumez]

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

[Chris Dumez]

Created attachment 362687 [details]
Patch

[EWS Watchlist]

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.

[Ryosuke Niwa]

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

[Chris Dumez]

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

[Chris Dumez]

Created attachment 362689 [details]
Patch

[Chris Dumez]

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.

[EWS Watchlist]

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.

[Chris Dumez]

Woohoo found something that builds on WPE.

[Geoffrey Garen]

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.

[Alex Christensen]

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

[Chris Dumez]

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.

[Chris Dumez]

Committed r241950: <https://trac.webkit.org/changeset/241950>