Regression(r248832): Unable to quicklook HTML files in Mail
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Sep 2019 23:55:36 +0000 (23:55 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 19 Sep 2019 23:55:36 +0000 (23:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=202012
<rdar://problem/55285295>

Reviewed by Geoff Garen and Brent Fulgham.

r248832 inadvertently reverted the fix for Mail that landed in r247400 by not using
the same logic to initialize the sandbox extension if the process had already
finished launching or not. In particular, the new code path that happens on process
launch unconditionally used '/' as resource directory for the sandbox extension if
the client did not provide one. The logic in maybeInitializeSandboxExtensionHandle()
would use the file URL's base URL as resource directory when creating a sandbox
extension for '/' would fail (which it often does).

To address the issue, have the logic that runs on process launch call
maybeInitializeSandboxExtensionHandle() so avoid duplicating code and make sure
both cases now have the Mail fix.

* UIProcess/AuxiliaryProcessProxy.cpp:
(WebKit::AuxiliaryProcessProxy::didFinishLaunching):
* UIProcess/AuxiliaryProcessProxy.h:
(WebKit::AuxiliaryProcessProxy::isLaunching const):
* UIProcess/WebPageProxy.cpp:
(WebKit::WebPageProxy::maybeInitializeSandboxExtensionHandle):
(WebKit::WebPageProxy::loadRequestWithNavigationShared):
(WebKit::WebPageProxy::loadFile):
* UIProcess/WebPageProxy.h:
* WebProcess/WebPage/WebPage.messages.in:

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

Source/WebKit/ChangeLog
Source/WebKit/UIProcess/AuxiliaryProcessProxy.cpp
Source/WebKit/UIProcess/AuxiliaryProcessProxy.h
Source/WebKit/UIProcess/WebPageProxy.cpp
Source/WebKit/UIProcess/WebPageProxy.h
Source/WebKit/WebProcess/WebPage/WebPage.cpp
Source/WebKit/WebProcess/WebPage/WebPage.h
Source/WebKit/WebProcess/WebPage/WebPage.messages.in

index a35362b..3f39d0f 100644 (file)
@@ -1,3 +1,34 @@
+2019-09-19  Chris Dumez  <cdumez@apple.com>
+
+        Regression(r248832): Unable to quicklook HTML files in Mail
+        https://bugs.webkit.org/show_bug.cgi?id=202012
+        <rdar://problem/55285295>
+
+        Reviewed by Geoff Garen and Brent Fulgham.
+
+        r248832 inadvertently reverted the fix for Mail that landed in r247400 by not using
+        the same logic to initialize the sandbox extension if the process had already
+        finished launching or not. In particular, the new code path that happens on process
+        launch unconditionally used '/' as resource directory for the sandbox extension if
+        the client did not provide one. The logic in maybeInitializeSandboxExtensionHandle()
+        would use the file URL's base URL as resource directory when creating a sandbox
+        extension for '/' would fail (which it often does).
+
+        To address the issue, have the logic that runs on process launch call
+        maybeInitializeSandboxExtensionHandle() so avoid duplicating code and make sure
+        both cases now have the Mail fix.
+
+        * UIProcess/AuxiliaryProcessProxy.cpp:
+        (WebKit::AuxiliaryProcessProxy::didFinishLaunching):
+        * UIProcess/AuxiliaryProcessProxy.h:
+        (WebKit::AuxiliaryProcessProxy::isLaunching const):
+        * UIProcess/WebPageProxy.cpp:
+        (WebKit::WebPageProxy::maybeInitializeSandboxExtensionHandle):
+        (WebKit::WebPageProxy::loadRequestWithNavigationShared):
+        (WebKit::WebPageProxy::loadFile):
+        * UIProcess/WebPageProxy.h:
+        * WebProcess/WebPage/WebPage.messages.in:
+
 2019-09-19  Tim Horton  <timothy_horton@apple.com>
 
         macCatalyst apps crash under TextCheckingControllerProxy::replaceRelativeToSelection when spell checking
index f599736..2e63254 100644 (file)
@@ -30,6 +30,8 @@
 #include "LoadParameters.h"
 #include "Logging.h"
 #include "WebPageMessages.h"
+#include "WebPageProxy.h"
+#include "WebProcessProxy.h"
 #include <wtf/RunLoop.h>
 
 namespace WebKit {
@@ -217,12 +219,16 @@ void AuxiliaryProcessProxy::didFinishLaunching(ProcessLauncher*, IPC::Connection
             auto bufferSize = encoder->bufferSize();
             std::unique_ptr<IPC::Decoder> decoder = makeUnique<IPC::Decoder>(buffer, bufferSize, nullptr, Vector<IPC::Attachment> { });
             LoadParameters loadParameters;
-            String sandboxExtensionPath;
-            if (decoder->decode(loadParameters) && decoder->decode(sandboxExtensionPath)) {
-                SandboxExtension::createHandleForReadByPid(sandboxExtensionPath, processIdentifier(), loadParameters.sandboxExtensionHandle);
-                send(Messages::WebPage::LoadRequest(loadParameters), decoder->destinationID());
-                continue;
-            }
+            URL resourceDirectoryURL;
+            WebPageProxyIdentifier pageID;
+            if (decoder->decode(loadParameters) && decoder->decode(resourceDirectoryURL) && decoder->decode(pageID)) {
+                if (auto* page = WebProcessProxy::webPage(pageID)) {
+                    page->maybeInitializeSandboxExtensionHandle(static_cast<WebProcessProxy&>(*this), loadParameters.request.url(), resourceDirectoryURL, loadParameters.sandboxExtensionHandle);
+                    send(Messages::WebPage::LoadRequest(loadParameters), decoder->destinationID());
+                }
+            } else
+                ASSERT_NOT_REACHED();
+            continue;
         }
 #endif
         if (pendingMessage.asyncReplyInfo)
index 158fd2d..f5a09b0 100644 (file)
@@ -98,6 +98,7 @@ public:
         Terminated,
     };
     State state() const;
+    bool isLaunching() const { return state() == State::Launching; }
 
     ProcessID processIdentifier() const { return m_processLauncher ? m_processLauncher->processIdentifier() : 0; }
 
index 9dca8f7..5a4645f 100644 (file)
@@ -1066,6 +1066,13 @@ void WebPageProxy::maybeInitializeSandboxExtensionHandle(WebProcessProxy& proces
     if (!url.isLocalFile())
         return;
 
+#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
+    // If the process is still launching then it does not have a PID yet. We will take care of creating the sandbox extension
+    // once the process has finished launching.
+    if (process.isLaunching())
+        return;
+#endif
+
     if (!resourceDirectoryURL.isEmpty()) {
         if (process.hasAssumedReadAccessToURL(resourceDirectoryURL))
             return;
@@ -1176,19 +1183,10 @@ void WebPageProxy::loadRequestWithNavigationShared(Ref<WebProcessProxy>&& proces
     addPlatformLoadParameters(loadParameters);
 
 #if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
-    if (processIdentifier() || !url.isLocalFile())
+    if (!process->isLaunching() || !url.isLocalFile())
         process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
-    else {
-        String sandboxExtensionPath;
-        if (!m_pageLoadState.resourceDirectoryURL().isEmpty()) {
-            sandboxExtensionPath = m_pageLoadState.resourceDirectoryURL().fileSystemPath();
-            process->assumeReadAccessToBaseURL(*this, m_pageLoadState.resourceDirectoryURL());
-        } else {
-            sandboxExtensionPath = "/";
-            willAcquireUniversalFileReadSandboxExtension(process);
-        }
-        process->send(Messages::WebPage::LoadRequestWaitingForPID(loadParameters, sandboxExtensionPath), webPageID);
-    }
+    else
+        process->send(Messages::WebPage::LoadRequestWaitingForPID(loadParameters, m_pageLoadState.resourceDirectoryURL(), m_identifier), webPageID);
 #else
     process->send(Messages::WebPage::LoadRequest(loadParameters), webPageID);
 #endif
@@ -1230,26 +1228,20 @@ RefPtr<API::Navigation> WebPageProxy::loadFile(const String& fileURLString, cons
 
     m_pageLoadState.setPendingAPIRequest(transaction, { navigation->navigationID(), fileURLString }, resourceDirectoryURL);
 
-    String resourceDirectoryPath = resourceDirectoryURL.fileSystemPath();
-
     LoadParameters loadParameters;
     loadParameters.navigationID = navigation->navigationID();
     loadParameters.request = fileURL;
     loadParameters.shouldOpenExternalURLsPolicy = ShouldOpenExternalURLsPolicy::ShouldNotAllow;
     loadParameters.userData = UserData(process().transformObjectsToHandles(userData).get());
-#if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
-    SandboxExtension::createHandleForReadByPid(resourceDirectoryPath, processIdentifier(), loadParameters.sandboxExtensionHandle);
-#else
-    SandboxExtension::createHandle(resourceDirectoryPath, SandboxExtension::Type::ReadOnly, loadParameters.sandboxExtensionHandle);
-#endif
+    maybeInitializeSandboxExtensionHandle(m_process, fileURL, resourceDirectoryURL, loadParameters.sandboxExtensionHandle);
     addPlatformLoadParameters(loadParameters);
 
     m_process->assumeReadAccessToBaseURL(*this, resourceDirectoryURL);
 #if HAVE(SANDBOX_ISSUE_READ_EXTENSION_TO_PROCESS_BY_PID)
-    if (processIdentifier())
-        m_process->send(Messages::WebPage::LoadRequest(loadParameters), m_webPageID);
+    if (m_process->isLaunching())
+        m_process->send(Messages::WebPage::LoadRequestWaitingForPID(loadParameters, resourceDirectoryURL, m_identifier), m_webPageID);
     else
-        m_process->send(Messages::WebPage::LoadRequestWaitingForPID(loadParameters, resourceDirectoryPath), m_webPageID);
+        m_process->send(Messages::WebPage::LoadRequest(loadParameters), m_webPageID);
 #else
     m_process->send(Messages::WebPage::LoadRequest(loadParameters), m_webPageID);
 #endif
index 71d0a1c..c1892ec 100644 (file)
@@ -1585,6 +1585,8 @@ public:
     void setMockCaptureDevicesEnabledOverride(Optional<bool>);
 #endif
 
+    void maybeInitializeSandboxExtensionHandle(WebProcessProxy&, const URL&, const URL& resourceDirectoryURL, SandboxExtension::Handle&);
+
 private:
     WebPageProxy(PageClient&, WebProcessProxy&, Ref<API::PageConfiguration>&&);
     void platformInitialize();
@@ -1925,8 +1927,6 @@ private:
     void setPluginComplexTextInputState(uint64_t pluginComplexTextInputIdentifier, uint64_t complexTextInputState);
 #endif
 
-    void maybeInitializeSandboxExtensionHandle(WebProcessProxy&, const URL& url, const URL& resourceDirectoryURL, SandboxExtension::Handle&);
-
 #if USE(AUTOMATIC_TEXT_REPLACEMENT)
     void toggleSmartInsertDelete();
     void toggleAutomaticQuoteSubstitution();
index 269c42f..3fbafa5 100644 (file)
@@ -1553,7 +1553,7 @@ void WebPage::loadRequest(LoadParameters&& loadParameters)
 }
 
 // LoadRequestWaitingForPID should never be sent to the WebProcess. It must always be converted to a LoadRequest message.
-NO_RETURN void WebPage::loadRequestWaitingForPID(LoadParameters&&, const String&)
+NO_RETURN void WebPage::loadRequestWaitingForPID(LoadParameters&&, URL&&, WebPageProxyIdentifier)
 {
     RELEASE_ASSERT_NOT_REACHED();
 }
index 3a7d6e9..a4e2fee 100644 (file)
@@ -1317,7 +1317,7 @@ private:
     void tryClose();
     void platformDidReceiveLoadParameters(const LoadParameters&);
     void loadRequest(LoadParameters&&);
-    NO_RETURN void loadRequestWaitingForPID(LoadParameters&&, const String&);
+    NO_RETURN void loadRequestWaitingForPID(LoadParameters&&, URL&&, WebPageProxyIdentifier);
     void loadData(LoadParameters&&);
     void loadAlternateHTML(LoadParameters&&);
     void navigateToPDFLinkWithSimulatedClick(const String& url, WebCore::IntPoint documentPoint, WebCore::IntPoint screenPoint);
index c6da6e2..49ef8ba 100644 (file)
@@ -165,7 +165,7 @@ GenerateSyntheticEditingCommand(enum:uint8_t WebKit::SyntheticEditingCommandType
     LoadURLInFrame(URL url, String referrer, WebCore::FrameIdentifier frameID)
     LoadDataInFrame(IPC::DataReference data, String MIMEType, String encodingName, URL baseURL, WebCore::FrameIdentifier frameID)
     LoadRequest(struct WebKit::LoadParameters loadParameters)
-    LoadRequestWaitingForPID(struct WebKit::LoadParameters loadParameters, String sandboxExtensionPath)
+    LoadRequestWaitingForPID(struct WebKit::LoadParameters loadParameters, URL resourceDirectoryURL, WebKit::WebPageProxyIdentifier pageID)
     LoadData(struct WebKit::LoadParameters loadParameters)
     LoadAlternateHTML(struct WebKit::LoadParameters loadParameters)