Add RefCounted CompletionHandler wrapping abstraction for sending policy decisions back to WebProcess
Created attachment 345896 [details] Patch
Created attachment 345919 [details] Patch
Created attachment 345921 [details] Patch
Created attachment 345922 [details] Patch
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?
Created attachment 345928 [details] Patch
Comment on attachment 345928 [details] Patch r=me
http://trac.webkit.org/r234327
<rdar://problem/42673418>
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;
(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
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
Reverted r234327 for reason: Caused 2 crashes on macOS and iOS debug API tests Committed r234371: <https://trac.webkit.org/changeset/234371>
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();
(In reply to Said Abou-Hallawa from comment #10) I agree. > using SendFunction = CompletionHandler<void(WebCore::PolicyAction, I'll use this. Thanks!
Created attachment 346081 [details] Patch
http://trac.webkit.org/r234375