AX: .js dialogs shown in unload while AX is running crash WebKit.
authorcfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 May 2014 23:02:59 +0000 (23:02 +0000)
committercfleizach@apple.com <cfleizach@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 May 2014 23:02:59 +0000 (23:02 +0000)
https://bugs.webkit.org/show_bug.cgi?id=123828

Reviewed by Anders Carlsson.

Utilize platform API to inform AX clients when the WebProcess will suspend and resume.
This allows us to avoid having special behavior for accessibility when the WebProcess needs
to wait on a reply.

* Platform/IPC/Connection.cpp:
(IPC::Connection::waitForSyncReply):
* Platform/IPC/Connection.h:
* Platform/IPC/mac/ConnectionMac.cpp:
(IPC::Connection::willSendSyncMessage):
(IPC::Connection::didReceiveSyncReply):
* WebProcess/WebCoreSupport/WebChromeClient.cpp:
(WebKit::WebChromeClient::runBeforeUnloadConfirmPanel):
(WebKit::WebChromeClient::runJavaScriptAlert):
(WebKit::WebChromeClient::runJavaScriptConfirm):
(WebKit::WebChromeClient::runJavaScriptPrompt):
(WebKit::WebChromeClient::print):
(WebKit::WebChromeClient::exceededDatabaseQuota):
* WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
(WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@168653 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebKit2/ChangeLog
Source/WebKit2/Platform/IPC/Connection.cpp
Source/WebKit2/Platform/IPC/Connection.h
Source/WebKit2/Platform/IPC/mac/ConnectionMac.mm
Source/WebKit2/Platform/IPC/unix/ConnectionUnix.cpp
Source/WebKit2/WebProcess/WebCoreSupport/WebChromeClient.cpp
Source/WebKit2/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp
Source/WebKit2/WebProcess/WebPage/WebPage.cpp

index f9268c9..772fef9 100644 (file)
@@ -1,3 +1,30 @@
+2014-05-12  Chris Fleizach  <cfleizach@apple.com>
+
+        AX: .js dialogs shown in unload while AX is running crash WebKit.
+        https://bugs.webkit.org/show_bug.cgi?id=123828
+
+        Reviewed by Anders Carlsson.
+
+        Utilize platform API to inform AX clients when the WebProcess will suspend and resume.
+        This allows us to avoid having special behavior for accessibility when the WebProcess needs
+        to wait on a reply.
+
+        * Platform/IPC/Connection.cpp:
+        (IPC::Connection::waitForSyncReply):
+        * Platform/IPC/Connection.h:
+        * Platform/IPC/mac/ConnectionMac.cpp:
+        (IPC::Connection::willSendSyncMessage):
+        (IPC::Connection::didReceiveSyncReply):
+        * WebProcess/WebCoreSupport/WebChromeClient.cpp:
+        (WebKit::WebChromeClient::runBeforeUnloadConfirmPanel):
+        (WebKit::WebChromeClient::runJavaScriptAlert):
+        (WebKit::WebChromeClient::runJavaScriptConfirm):
+        (WebKit::WebChromeClient::runJavaScriptPrompt):
+        (WebKit::WebChromeClient::print):
+        (WebKit::WebChromeClient::exceededDatabaseQuota):
+        * WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:
+        (WebKit::WebFrameLoaderClient::dispatchDecidePolicyForResponse):
+
 2014-05-12  Alex Christensen  <achristensen@webkit.org>
 
         Progress on web timing.
index 321c120..5990a87 100644 (file)
@@ -510,6 +510,8 @@ std::unique_ptr<MessageDecoder> Connection::waitForSyncReply(uint64_t syncReques
 {
     double absoluteTime = currentTime() + (timeout.count() / 1000.0);
 
+    willSendSyncMessage(syncSendFlags);
+    
     bool timedOut = false;
     while (!timedOut) {
         // First, check if we have any messages that we need to process.
@@ -525,16 +527,20 @@ std::unique_ptr<MessageDecoder> Connection::waitForSyncReply(uint64_t syncReques
             ASSERT_UNUSED(syncRequestID, pendingSyncReply.syncRequestID == syncRequestID);
             
             // We found the sync reply, or the connection was closed.
-            if (pendingSyncReply.didReceiveReply || !m_shouldWaitForSyncReplies)
+            if (pendingSyncReply.didReceiveReply || !m_shouldWaitForSyncReplies) {
+                didReceiveSyncReply(syncSendFlags);
                 return std::move(pendingSyncReply.replyDecoder);
+            }
         }
 
         // Processing a sync message could cause the connection to be invalidated.
         // (If the handler ends up calling Connection::invalidate).
         // If that happens, we need to stop waiting, or we'll hang since we won't get
         // any more incoming messages.
-        if (!isValid())
+        if (!isValid()) {
+            didReceiveSyncReply(syncSendFlags);
             return nullptr;
+        }
 
         // We didn't find a sync reply yet, keep waiting.
         // This allows the WebProcess to still serve clients while waiting for the message to return.
@@ -550,6 +556,8 @@ std::unique_ptr<MessageDecoder> Connection::waitForSyncReply(uint64_t syncReques
             timedOut = !m_syncMessageState->wait(absoluteTime);
         
     }
+    
+    didReceiveSyncReply(syncSendFlags);
 
     return nullptr;
 }
index d85a96b..40a138a 100644 (file)
@@ -62,8 +62,11 @@ enum MessageSendFlags {
 };
 
 enum SyncMessageSendFlags {
-    // Will allow events to continue being handled while waiting for the sync reply.
-    SpinRunLoopWhileWaitingForReply = 1 << 0,
+    // Use this to inform that this sync call will suspend this process until the user responds with input.
+    InformPlatformProcessWillSuspend = 1 << 0,
+    // Some platform accessibility clients can't suspend gracefully and need to spin the run loop so WebProcess doesn't hang.
+    // FIXME (126021): Remove when no platforms need to support this.
+    SpinRunLoopWhileWaitingForReply = 1 << 1,
 };
     
 #define MESSAGE_CHECK_BASE(assertion, connection) do \
@@ -212,6 +215,9 @@ private:
     // Can be called on any thread.
     void enqueueIncomingMessage(std::unique_ptr<MessageDecoder>);
 
+    void willSendSyncMessage(unsigned syncSendFlags);
+    void didReceiveSyncReply(unsigned syncSendFlags);
+    
     Client* m_client;
     bool m_isServer;
     std::atomic<uint64_t> m_syncRequestID;
index 2bea1a4..b543b1e 100644 (file)
@@ -30,6 +30,7 @@
 #include "ImportanceAssertion.h"
 #include "MachPort.h"
 #include "MachUtilities.h"
+#include <WebCore/AXObjectCache.h>
 #include <mach/mach_error.h>
 #include <mach/vm_map.h>
 #include <wtf/RunLoop.h>
@@ -42,6 +43,18 @@ extern "C" void xpc_connection_get_audit_token(xpc_connection_t, audit_token_t*)
 extern "C" void xpc_connection_kill(xpc_connection_t, int);
 #endif
 
+#if __has_include(<HIServices/AccessibilityPriv.h>)
+#include <HIServices/AccessibilityPriv.h>
+#else
+typedef enum {
+    AXSuspendStatusRunning = 0,
+    AXSuspendStatusSuspended,
+} AXSuspendStatus;
+#endif
+
+#if !TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000
+extern "C" AXError _AXUIElementNotifyProcessSuspendStatus(AXSuspendStatus);
+#endif
 
 namespace IPC {
 
@@ -536,5 +549,21 @@ bool Connection::kill()
 
     return false;
 }
+    
+void Connection::willSendSyncMessage(unsigned flags)
+{
+#if !TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000
+    if ((flags & InformPlatformProcessWillSuspend) && WebCore::AXObjectCache::accessibilityEnabled())
+        _AXUIElementNotifyProcessSuspendStatus(AXSuspendStatusSuspended);
+#endif
+}
 
+void Connection::didReceiveSyncReply(unsigned flags)
+{
+#if !TARGET_OS_IPHONE && __MAC_OS_X_VERSION_MIN_REQUIRED >= 101000
+    if ((flags & InformPlatformProcessWillSuspend) && WebCore::AXObjectCache::accessibilityEnabled())
+        _AXUIElementNotifyProcessSuspendStatus(AXSuspendStatusRunning);
+#endif
+}    
+    
 } // namespace IPC
index 99ae498..f4ccd34 100644 (file)
@@ -557,5 +557,15 @@ Connection::SocketPair Connection::createPlatformConnection()
     SocketPair socketPair = { sockets[0], sockets[1] };
     return socketPair;
 }
+    
+void Connection::willSendSyncMessage(unsigned flags)
+{
+    UNUSED_PARAM(flags);
+}
+    
+void Connection::didReceiveSyncReply(unsigned flags)
+{
+    UNUSED_PARAM(flags);    
+}
 
 } // namespace IPC
index e72bfb2..1bef85a 100644 (file)
@@ -327,7 +327,12 @@ bool WebChromeClient::runBeforeUnloadConfirmPanel(const String& message, Frame*
     WebFrame* webFrame = WebFrame::fromCoreFrame(*frame);
 
     bool shouldClose = false;
-    if (!WebProcess::shared().parentProcessConnection()->sendSync(Messages::WebPageProxy::RunBeforeUnloadConfirmPanel(message, webFrame->frameID()), Messages::WebPageProxy::RunBeforeUnloadConfirmPanel::Reply(shouldClose), m_page->pageID()))
+
+    unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend;
+    if (WebPage::synchronousMessagesShouldSpinRunLoop())
+        syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply;
+    
+    if (!WebProcess::shared().parentProcessConnection()->sendSync(Messages::WebPageProxy::RunBeforeUnloadConfirmPanel(message, webFrame->frameID()), Messages::WebPageProxy::RunBeforeUnloadConfirmPanel::Reply(shouldClose), m_page->pageID(), std::chrono::milliseconds::max(), syncSendFlags))
         return false;
 
     return shouldClose;
@@ -360,8 +365,9 @@ void WebChromeClient::runJavaScriptAlert(Frame* frame, const String& alertText)
     // Notify the bundle client.
     m_page->injectedBundleUIClient().willRunJavaScriptAlert(m_page, alertText, webFrame);
 
-    // FIXME (126021): It is not good to change IPC behavior conditionally, and SpinRunLoopWhileWaitingForReply was known to cause trouble in other similar cases.
-    unsigned syncSendFlags = WebPage::synchronousMessagesShouldSpinRunLoop() ? IPC::SpinRunLoopWhileWaitingForReply : 0;
+    unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend;
+    if (WebPage::synchronousMessagesShouldSpinRunLoop())
+        syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply;
     WebProcess::shared().parentProcessConnection()->sendSync(Messages::WebPageProxy::RunJavaScriptAlert(webFrame->frameID(), alertText), Messages::WebPageProxy::RunJavaScriptAlert::Reply(), m_page->pageID(), std::chrono::milliseconds::max(), syncSendFlags);
 }
 
@@ -373,8 +379,9 @@ bool WebChromeClient::runJavaScriptConfirm(Frame* frame, const String& message)
     // Notify the bundle client.
     m_page->injectedBundleUIClient().willRunJavaScriptConfirm(m_page, message, webFrame);
 
-    // FIXME (126021): It is not good to change IPC behavior conditionally, and SpinRunLoopWhileWaitingForReply was known to cause trouble in other similar cases.
-    unsigned syncSendFlags = WebPage::synchronousMessagesShouldSpinRunLoop() ? IPC::SpinRunLoopWhileWaitingForReply : 0;
+    unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend;
+    if (WebPage::synchronousMessagesShouldSpinRunLoop())
+        syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply;
     bool result = false;
     if (!WebProcess::shared().parentProcessConnection()->sendSync(Messages::WebPageProxy::RunJavaScriptConfirm(webFrame->frameID(), message), Messages::WebPageProxy::RunJavaScriptConfirm::Reply(result), m_page->pageID(), std::chrono::milliseconds::max(), syncSendFlags))
         return false;
@@ -390,8 +397,10 @@ bool WebChromeClient::runJavaScriptPrompt(Frame* frame, const String& message, c
     // Notify the bundle client.
     m_page->injectedBundleUIClient().willRunJavaScriptPrompt(m_page, message, defaultValue, webFrame);
 
-    // FIXME (126021): It is not good to change IPC behavior conditionally, and SpinRunLoopWhileWaitingForReply was known to cause trouble in other similar cases.
-    unsigned syncSendFlags = WebPage::synchronousMessagesShouldSpinRunLoop() ? IPC::SpinRunLoopWhileWaitingForReply : 0;
+    unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend;
+    if (WebPage::synchronousMessagesShouldSpinRunLoop())
+        syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply;
+    
     if (!WebProcess::shared().parentProcessConnection()->sendSync(Messages::WebPageProxy::RunJavaScriptPrompt(webFrame->frameID(), message, defaultValue), Messages::WebPageProxy::RunJavaScriptPrompt::Reply(result), m_page->pageID(), std::chrono::milliseconds::max(), syncSendFlags))
         return false;
 
@@ -608,7 +617,11 @@ void WebChromeClient::print(Frame* frame)
     RefPtr<PrinterListGtk> printerList = PrinterListGtk::shared();
 #endif
 
-    m_page->sendSync(Messages::WebPageProxy::PrintFrame(webFrame->frameID()), Messages::WebPageProxy::PrintFrame::Reply());
+    unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend;
+    if (WebPage::synchronousMessagesShouldSpinRunLoop())
+        syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply;
+    
+    m_page->sendSync(Messages::WebPageProxy::PrintFrame(webFrame->frameID()), Messages::WebPageProxy::PrintFrame::Reply(), std::chrono::milliseconds::max(), syncSendFlags);
 }
 
 #if ENABLE(SQL_DATABASE)
@@ -627,9 +640,13 @@ void WebChromeClient::exceededDatabaseQuota(Frame* frame, const String& database
     newQuota = m_page->injectedBundleUIClient().didExceedDatabaseQuota(m_page, webSecurityOrigin.get(), databaseName, details.displayName(), currentQuota, currentOriginUsage, details.currentUsage(), details.expectedUsage());
 
     if (!newQuota) {
+        unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend;
+        if (WebPage::synchronousMessagesShouldSpinRunLoop())
+            syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply;
+        
         WebProcess::shared().parentProcessConnection()->sendSync(
             Messages::WebPageProxy::ExceededDatabaseQuota(webFrame->frameID(), origin->databaseIdentifier(), databaseName, details.displayName(), currentQuota, currentOriginUsage, details.currentUsage(), details.expectedUsage()),
-            Messages::WebPageProxy::ExceededDatabaseQuota::Reply(newQuota), m_page->pageID());
+            Messages::WebPageProxy::ExceededDatabaseQuota::Reply(newQuota), m_page->pageID(), std::chrono::milliseconds::max(), syncSendFlags);
     }
 
     dbManager.setQuota(origin, newQuota);
index 719b44a..1bf133a 100644 (file)
@@ -53,7 +53,6 @@
 #include "WebProcessProxyMessages.h"
 #include <JavaScriptCore/APICast.h>
 #include <JavaScriptCore/JSObject.h>
-#include <WebCore/AXObjectCache.h>
 #include <WebCore/CertificateInfo.h>
 #include <WebCore/Chrome.h>
 #include <WebCore/DOMWrapperWorld.h>
@@ -661,9 +660,9 @@ void WebFrameLoaderClient::dispatchDecidePolicyForResponse(const ResourceRespons
     uint64_t policyAction;
     uint64_t downloadID;
 
-    // Notify the UIProcess.
-    // FIXME (126021): It is not good to change IPC behavior conditionally, and SpinRunLoopWhileWaitingForReply was known to cause trouble in other similar cases.
-    unsigned syncSendFlags = WebPage::synchronousMessagesShouldSpinRunLoop() ? IPC::SpinRunLoopWhileWaitingForReply : 0;
+    unsigned syncSendFlags = IPC::InformPlatformProcessWillSuspend;
+    if (WebPage::synchronousMessagesShouldSpinRunLoop())
+        syncSendFlags |= IPC::SpinRunLoopWhileWaitingForReply;
     if (!webPage->sendSync(Messages::WebPageProxy::DecidePolicyForResponseSync(m_frame->frameID(), response, request, canShowMIMEType, listenerID, InjectedBundleUserMessageEncoder(userData.get())), Messages::WebPageProxy::DecidePolicyForResponseSync::Reply(receivedPolicyAction, policyAction, downloadID), std::chrono::milliseconds::max(), syncSendFlags))
         return;
 
index 8464218..ea995ee 100644 (file)
@@ -4594,7 +4594,7 @@ PassRefPtr<WebCore::Range> WebPage::rangeFromEditingRange(WebCore::Frame& frame,
     
 bool WebPage::synchronousMessagesShouldSpinRunLoop()
 {
-#if PLATFORM(MAC)
+#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED < 101000
     return WebCore::AXObjectCache::accessibilityEnabled();
 #endif
     return false;