Bug 188089

Summary: Add RefCounted CompletionHandler wrapping abstraction for sending policy decisions back to WebProcess
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Status: RESOLVED FIXED    
Severity: Normal CC: aestes, cdumez, ggaren, realdawei, sabouhallawa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Alex Christensen 2018-07-26 19:24:03 PDT 
Add RefCounted CompletionHandler wrapping abstraction for sending policy decisions back to WebProcess
Comment 1 Alex Christensen 2018-07-26 19:30:41 PDT 
Created attachment 345896 [details]
Patch
Comment 2 Alex Christensen 2018-07-27 09:31:53 PDT 
Created attachment 345919 [details]
Patch
Comment 3 Alex Christensen 2018-07-27 09:53:52 PDT 
Created attachment 345921 [details]
Patch
Comment 4 Alex Christensen 2018-07-27 10:00:11 PDT 
Created attachment 345922 [details]
Patch
Comment 5 Said Abou-Hallawa 2018-07-27 10:27:20 PDT 
Comment on attachment 345922 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:2415
> +    template<typename... Args> static Ref<PolicyDecisionSender> create(Args... args)
> +    {
> +        return adoptRef(*new PolicyDecisionSender(std::forward<Args>(args)...));
> +    }

Why is this function taking a template parameter pack even though all the callers are sending one parameters which is the CompletionHandler?

> Source/WebKit/UIProcess/WebPageProxy.cpp:2420
> +    template<typename... Args> void operator()(Args... args)
> +    {
> +        if (m_completionHandler)
> +            m_completionHandler(std::forward<Args>(args)...);
> +    }

I am not sure why this operator is not just a function, for example

template<typename... Args> void send(Args... args)
or template<typename... Args> void complete(Args... args)
or template<typename... Args> void apply(Args... args)

> Source/WebKit/UIProcess/WebPageProxy.cpp:2455
> +    sender.get()(action, navigation ? navigation->navigationID() : 0, downloadID, WTFMove(websitePolicies));

This is hard to read. I am not sure what sender.get()(...) really means. I think a verbose name would be better.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4028
> +    auto listener = makeRef(frame->setUpPolicyListenerProxy([this, protectedThis = makeRef(*this), frame = makeRef(*frame), sender = sender.copyRef(), navigation] (WebCore::PolicyAction policyAction, API::WebsitePolicies* policies, ShouldProcessSwapIfPossible swap) mutable {

listener is initialized far from it is used. In fact there is a return statement comes before is it even used.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4055
>      if (frame->didHandleContentFilterUnblockNavigation(request))
> -        return receivedPolicyDecision(PolicyAction::Ignore, *frame, listenerID, &m_navigationState->navigation(newNavigationID), std::nullopt);
> +        return receivedPolicyDecision(PolicyAction::Ignore, &m_navigationState->navigation(newNavigationID), std::nullopt, WTFMove(sender));

Does this return statement need to come after the initialization of listener?
Comment 6 Alex Christensen 2018-07-27 10:46:43 PDT 
Created attachment 345928 [details]
Patch
Comment 7 Geoffrey Garen 2018-07-27 13:46:19 PDT 
Comment on attachment 345928 [details]
Patch

r=me
Comment 8 Alex Christensen 2018-07-27 13:50:18 PDT 
http://trac.webkit.org/r234327
Comment 9 Radar WebKit Bug Importer 2018-07-27 13:51:37 PDT 
<rdar://problem/42673418>
Comment 10 Said Abou-Hallawa 2018-07-27 19:04:26 PDT 
Comment on attachment 345928 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:2415
> +    template<typename Lambda> static Ref<PolicyDecisionSender> create(Lambda lambda)
> +    {
> +        return adoptRef(*new PolicyDecisionSender(WTFMove(lambda)));
> +    }

I do not think it is correct to have create() as a template function given that lambda is going to be assigned to a specific type which is CompletionHandler<void(WebCore::PolicyAction, uint64_t newNavigationID, DownloadID, std::optional<WebsitePoliciesData>)>. So why do not we specific about the type of the lambda?

    using SendFunction = CompletionHandler<void(WebCore::PolicyAction, uint64_t newNavigationID, DownloadID, std::optional<WebsitePoliciesData>)>;

    static Ref<PolicyDecisionSender> create(SendFunction&& sendFunction)
    {
        return adoptRef(*new PolicyDecisionSender(WTFMove(sendFunction)));
    }

> Source/WebKit/UIProcess/WebPageProxy.cpp:2423
> +    template<typename Lambda> PolicyDecisionSender(Lambda lambda)
> +        : m_completionHandler(WTFMove(lambda)) { }

I do not think this constructor should be templatized. Also shouldn't lambda be an rvalue reference?

    PolicyDecisionSender(SendFunction&& sendFunction)
        : m_sendFunction(WTFMove(sendFunction))
    {
    }

> Source/WebKit/UIProcess/WebPageProxy.cpp:2425
> +    CompletionHandler<void(WebCore::PolicyAction, uint64_t newNavigationID, DownloadID, std::optional<WebsitePoliciesData>)> m_completionHandler;

This can be written simpler:

    SendFunction m_sendFunction;
Comment 11 Dawei Fenton (:realdawei) 2018-07-30 09:28:53 PDT 
(In reply to Alex Christensen from comment #8)
> http://trac.webkit.org/r234327

Seeing API crashes after this revision:

steps to reproduce:
run-api-tests --debug --verbose TestWebKitAPI.WebKit.JavaScriptDuringNavigation

Sample output:
 TestWebKitAPI.WebKit.JavaScriptDuringNavigation
        ASSERTION FAILED: Completion handler should always be called
        !m_function
        /Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/CompletionHandler.h(51) : WTF::CompletionHandler<void (WebCore::PolicyAction, unsigned long long, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData>)>::~CompletionHandler()
        1   0x102fa1749 WTFCrash
        2   0x108770577 WTF::CompletionHandler<void (WebCore::PolicyAction, unsigned long long, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData>)>::~CompletionHandler()
        3   0x1087704f5 WTF::CompletionHandler<void (WebCore::PolicyAction, unsigned long long, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData>)>::~CompletionHandler()
        4   0x1087704c3 WebKit::WebPageProxy::PolicyDecisionSender::~PolicyDecisionSender()
        5   0x108770475 WebKit::WebPageProxy::PolicyDecisionSender::~PolicyDecisionSender()
        6   0x108770447 WTF::RefCounted<WebKit::WebPageProxy::PolicyDecisionSender>::deref() const
        7   0x1087703ef WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >::~Ref()
        8   0x108716dd5 WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >::~Ref()
        9   0x108773fa7 WebKit::WebPageProxy::decidePolicyForResponse(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, bool, unsigned long long, WebKit::UserData const&)::$_10::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        10  0x108773d89 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::CallableWrapper<WebKit::WebPageProxy::decidePolicyForResponse(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebCore::ResourceResponse const&, WebCore::ResourceRequest const&, bool, unsigned long long, WebKit::UserData const&)::$_10>::call(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        11  0x1084f6073 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible) const
        12  0x1084f5c2c WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        13  0x1084f911a WebKit::WebFrameProxy::setUpPolicyListenerProxy(WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>&&)::$_0::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        14  0x1084f8e39 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::CallableWrapper<WebKit::WebFrameProxy::setUpPolicyListenerProxy(WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>&&)::$_0>::call(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        15  0x1084f6073 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible) const
        16  0x1084f5c2c WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        17  0x1084f5c94 WebKit::WebFramePolicyListenerProxy::ignore()
        18  0x1084f6664 WebKit::WebFrameProxy::webProcessWillShutDown()
        19  0x108967b9d WebKit::WebProcessProxy::disconnectFramesFromPage(WebKit::WebPageProxy*)
        20  0x1086d7080 WebKit::WebPageProxy::close()
        21  0x108c35aea -[WKWebView dealloc]
        22  0x7fff6bf6b042 (anonymous namespace)::AutoreleasePoolPage::pop(void*)
        23  0x7fff447a88e6 _CFAutoreleasePoolPop
        24  0x7fff468e68ad -[NSAutoreleasePool drain]
        25  0x10251a497 main
        26  0x7fff6cb8c015 start
        27  0x2
Comment 12 Dawei Fenton (:realdawei) 2018-07-30 09:36:49 PDT 
another API test is also crashing after this revision:

Steps to reproduce: 
run-api-tests--debug --verbose TestWebKitAPI.WebKit.UpdateWebsitePoliciesInvalid

   TestWebKitAPI.WebKit.UpdateWebsitePoliciesInvalid
        ASSERTION FAILED: Completion handler should always be called
        !m_function
        /Volumes/Data/slave/highsierra-debug/build/WebKitBuild/Debug/usr/local/include/wtf/CompletionHandler.h(51) : WTF::CompletionHandler<void (WebCore::PolicyAction, unsigned long long, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData>)>::~CompletionHandler()
        1   0x10fb80749 WTFCrash
        2   0x117b31577 WTF::CompletionHandler<void (WebCore::PolicyAction, unsigned long long, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData>)>::~CompletionHandler()
        3   0x117b314f5 WTF::CompletionHandler<void (WebCore::PolicyAction, unsigned long long, WebKit::DownloadID, std::optional<WebKit::WebsitePoliciesData>)>::~CompletionHandler()
        4   0x117b314c3 WebKit::WebPageProxy::PolicyDecisionSender::~PolicyDecisionSender()
        5   0x117b31475 WebKit::WebPageProxy::PolicyDecisionSender::~PolicyDecisionSender()
        6   0x117b31447 WTF::RefCounted<WebKit::WebPageProxy::PolicyDecisionSender>::deref() const
        7   0x117b313ef WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >::~Ref()
        8   0x117ad7dd5 WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >::~Ref()
        9   0x117b137e3 WebKit::WebPageProxy::decidePolicyForNavigationAction(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >&&)::$_8::~$_8()
        10  0x117ad8115 WebKit::WebPageProxy::decidePolicyForNavigationAction(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >&&)::$_8::~$_8()
        11  0x117b31ea1 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::CallableWrapper<WebKit::WebPageProxy::decidePolicyForNavigationAction(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >&&)::$_8>::~CallableWrapper()
        12  0x117b31ba5 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::CallableWrapper<WebKit::WebPageProxy::decidePolicyForNavigationAction(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >&&)::$_8>::~CallableWrapper()
        13  0x117b31bc9 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::CallableWrapper<WebKit::WebPageProxy::decidePolicyForNavigationAction(unsigned long long, WebCore::SecurityOriginData const&, unsigned long long, WebKit::NavigationActionData&&, WebKit::FrameInfoData const&, unsigned long long, WebCore::ResourceRequest const&, WebCore::ResourceRequest&&, WebCore::ResourceResponse&&, WebKit::UserData const&, WTF::Ref<WebKit::WebPageProxy::PolicyDecisionSender, WTF::DumbPtrTraits<WebKit::WebPageProxy::PolicyDecisionSender> >&&)::$_8>::~CallableWrapper()
        14  0x1178b722f WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::~Function()
        15  0x1178b7095 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::~Function()
        16  0x1178b6c35 WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        17  0x1178ba11a WebKit::WebFrameProxy::setUpPolicyListenerProxy(WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>&&)::$_0::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        18  0x1178b9e39 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::CallableWrapper<WebKit::WebFrameProxy::setUpPolicyListenerProxy(WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>&&)::$_0>::call(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        19  0x1178b7073 WTF::Function<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible) const
        20  0x1178b6c2c WTF::CompletionHandler<void (WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)>::operator()(WebCore::PolicyAction, API::WebsitePolicies*, WebKit::ShouldProcessSwapIfPossible)
        21  0x1178b6c94 WebKit::WebFramePolicyListenerProxy::ignore()
        22  0x1178b7664 WebKit::WebFrameProxy::webProcessWillShutDown()
        23  0x117d28b9d WebKit::WebProcessProxy::disconnectFramesFromPage(WebKit::WebPageProxy*)
        24  0x117a98080 WebKit::WebPageProxy::close()
        25  0x117ff6aea -[WKWebView dealloc]
        26  0x7fff6bf6b042 (anonymous namespace)::AutoreleasePoolPage::pop(void*)
        27  0x7fff447a88e6 _CFAutoreleasePoolPop
        28  0x7fff468e68ad -[NSAutoreleasePool drain]
        29  0x10f0fb497 main
        30  0x7fff6cb8c015 start
        31  0x2
Comment 13 Dawei Fenton (:realdawei) 2018-07-30 10:14:18 PDT 
Reverted r234327 for reason:

Caused 2 crashes on macOS and iOS debug API tests

Committed r234371: <https://trac.webkit.org/changeset/234371>
Comment 14 Alex Christensen 2018-07-30 11:59:30 PDT 
I also needed this:

Index: Source/WebKit/UIProcess/WebPageProxy.cpp
===================================================================
--- Source/WebKit/UIProcess/WebPageProxy.cpp	(revision 234333)
+++ Source/WebKit/UIProcess/WebPageProxy.cpp	(working copy)
@@ -2428,8 +2428,10 @@
     
 void WebPageProxy::receivedPolicyDecision(PolicyAction action, API::Navigation* navigation, std::optional<WebsitePoliciesData>&& websitePolicies, Ref<PolicyDecisionSender>&& sender)
 {
-    if (!isValid())
+    if (!isValid()) {
+        sender->send(PolicyAction::Ignore, 0, DownloadID(), std::nullopt);
         return;
+    }
 
     auto transaction = m_pageLoadState.transaction();
Comment 15 Alex Christensen 2018-07-30 12:02:08 PDT 
(In reply to Said Abou-Hallawa from comment #10)
I agree.
>     using SendFunction = CompletionHandler<void(WebCore::PolicyAction,
I'll use this.  Thanks!
Comment 16 Alex Christensen 2018-07-30 12:04:46 PDT 
Created attachment 346081 [details]
Patch
Comment 17 Alex Christensen 2018-07-30 12:12:16 PDT 
http://trac.webkit.org/r234375