Bug 192482 – PSON logic gets confused by concurrent decidePolicyForNavigationAction requests

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

PSON logic gets confused by concurrent decidePolicyForNavigationAction requests.

[Chris Dumez]

<rdar://problem/46470145>

[Chris Dumez]

Created attachment 356767 [details]
Patch

[Alex Christensen]

Comment on attachment 356767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356767&action=review

> Source/WebKit/ChangeLog:25
> +        To address these issues, the following design is now implemented:

This sounds good.

> Source/WebKit/UIProcess/SuspendedPageProxy.cpp:123
> +std::unique_ptr<SuspendedPageProxy> SuspendedPageProxy::unsuspendAndConsume()

This is a very strange ownership model.  It returns a unique_ptr of itself?

> Source/WebKit/UIProcess/SuspendedPageProxy.h:50
> +    void whenReadyToConsume(CompletionHandler<void(SuspendedPageProxy*)>&&);

I think this is poorly named.  I think it needs a verb at the beginning.
Also, what does "consume" mean in this case?  Does it mean we are discarding the process or we are promoting this suspended page to be the page backing the WKWebView?

> Source/WebKit/UIProcess/WebPageProxy.cpp:772
> +    m_isValid = true;

I don't understand why we are changing the validity of the page during swapping.  You mentioned it earlier today and it puzzled me then, too.

[Chris Dumez]

Comment on attachment 356767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356767&action=review

>> Source/WebKit/UIProcess/SuspendedPageProxy.cpp:123
>> +std::unique_ptr<SuspendedPageProxy> SuspendedPageProxy::unsuspendAndConsume()
> 
> This is a very strange ownership model.  It returns a unique_ptr of itself?

Yes, it returns a unique_ptr of itself, thus "consume" in the same. Once a SuspendedPageProxy is unsuspended, it can no longer be used so it gets removed from the WebProcessPool (which normally owns it). At this point somebody has to own the suspended page and it makes sense that it is the caller of unsuspend().

>> Source/WebKit/UIProcess/SuspendedPageProxy.h:50
>> +    void whenReadyToConsume(CompletionHandler<void(SuspendedPageProxy*)>&&);
> 
> I think this is poorly named.  I think it needs a verb at the beginning.
> Also, what does "consume" mean in this case?  Does it mean we are discarding the process or we are promoting this suspended page to be the page backing the WKWebView?

consume means unsuspend. After you unsuspend a suspended page, it can no longer be used, which is why I used the naming "consume".

[Chris Dumez]

Comment on attachment 356767 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356767&action=review

>> Source/WebKit/UIProcess/WebPageProxy.cpp:772
>> +    m_isValid = true;
> 
> I don't understand why we are changing the validity of the page during swapping.  You mentioned it earlier today and it puzzled me then, too.

This is pre-existing behavior, has been like this from the start when Brady implemented it. We currently call processDidTerminate() which sets m_isValid to false so we need to set it back to true when we reconnect to the new process. This is all synchronous so it does not matter much.

[Chris Dumez]

Created attachment 356814 [details]
Patch

[Chris Dumez]

Ok, I tried to improve the naming / ownership model based on your feedback. Hopefully you'll like this iteration better.

[Chris Dumez]

ping review?

[Antti Koivisto]

Comment on attachment 356814 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=356814&action=review

The additional asynchronous steps make the code even harder to follow, which is sad. But this does solve a real problem.

> Source/WebKit/ChangeLog:11
> +        It is possible to get 2 parallel decidePolicyForNavigationAction() requests from the
> +        WebProcess when a new load is started before responding to the existing policy
> +        decision. This would lead to several issues with regards to PSON:

Could we alternatively avoid this issue by not calling policy delegates for about:blank loads for suspension?

[Chris Dumez]

(In reply to Antti Koivisto from comment #9)
> Comment on attachment 356814 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=356814&action=review
> 
> The additional asynchronous steps make the code even harder to follow, which
> is sad. But this does solve a real problem.
> 
> > Source/WebKit/ChangeLog:11
> > +        It is possible to get 2 parallel decidePolicyForNavigationAction() requests from the
> > +        WebProcess when a new load is started before responding to the existing policy
> > +        decision. This would lead to several issues with regards to PSON:
> 
> Could we alternatively avoid this issue by not calling policy delegates for
> about:blank loads for suspension?

We already do not do policy checks for about:blank suspension loads. The issue is really that the browser (or the page) can asks for several loads before we've received the policy decision for the first load.

[Chris Dumez]

Comment on attachment 356814 [details]
Patch

Clearing flags on attachment: 356814

Committed r239080: <https://trac.webkit.org/changeset/239080>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

Looks like the new api test from https://trac.webkit.org/changeset/239080/webkit

is failing on Debug builds

Log
https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK1%20%28Tests%29/builds/10666/steps/run-api-tests/logs/stdio

Failed

    TestWebKitAPI.ProcessSwap.ConcurrentHistoryNavigations
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        LEAK: 1 WebProcessPool
        
        /Volumes/Data/slave/sierra-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:2537
        Expected equality of these values:
          webkitPID
            Which is: 57142
          [webView _webProcessIdentifier]
            Which is: 57144

[Chris Dumez]

(In reply to Truitt Savell from comment #13)
> Looks like the new api test from
> https://trac.webkit.org/changeset/239080/webkit
> 
> is failing on Debug builds
> 
> Log
> https://build.webkit.org/builders/Apple%20Sierra%20Debug%20WK1%20%28Tests%29/
> builds/10666/steps/run-api-tests/logs/stdio
> 
> Failed
> 
>     TestWebKitAPI.ProcessSwap.ConcurrentHistoryNavigations
>         Received data during response processing, queuing it.
>         Received data during response processing, queuing it.
>         Received data during response processing, queuing it.
>         LEAK: 1 WebProcessPool
>         
>        
> /Volumes/Data/slave/sierra-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/
> ProcessSwapOnNavigation.mm:2537
>         Expected equality of these values:
>           webkitPID
>             Which is: 57142
>           [webView _webProcessIdentifier]
>             Which is: 57144

Fixed API test in <https://trac.webkit.org/changeset/239095>.

[Ross Kirsling]

This patch broke the WinCairo Debug build:
> Source\WebKit\UIProcess\WebPageProxy.cpp(2762): error C2381: 'WebKit::WebPageProxy::didFailToSuspendAfterProcessSwap': redefinition; '__declspec(noreturn)' or '[[noreturn]]' differs
> Source\WebKit\UIProcess\WebPageProxy.h(1569): note: see declaration of 'WebKit::WebPageProxy::didFailToSuspendAfterProcessSwap'

So I made a simple build fix in r239129:
https://trac.webkit.org/changeset/239129/webkit

But this evidently causes tests to crash in Cocoa Debug, so I tried a follow-up in r239131 which should have been *even more* innocuous:
https://trac.webkit.org/changeset/239131/webkit

Yet this did not help:
https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r239131%20(8973)/results.html

So it was rolled out in bug 192646.

[Ross Kirsling]

Committed MSVC build fix in r239144: <https://trac.webkit.org/changeset/239144>