Changeset 275486 in webkit

Timestamp:

Apr 5, 2021 9:02:40 PM (3 years ago)

Author:

commit-queue@webkit.org

Message:

Reduce crash inside getAuditToken
​https://bugs.webkit.org/show_bug.cgi?id=224196
<rdar://74536285>

Patch by Alex Christensen <​achristensen@webkit.org> on 2021-04-05
Reviewed by David Kilzer.

Something is crashing inside the call to getAuditToken, and I believe it is happening during teardown of the network process.
After many days of head scratching and many previous attempts at fixing this problem, it persists.
Since direct strategies at fixing this problem have failed, I now try something different.
Instead of calling getAuditToken at the beginning of every resource load to ask if the parent process has an entitlement,
I now call it only once per process. That should make things faster and less crashy. Otherwise no change in behavior.

• NetworkProcess/cocoa/NetworkSessionCocoa.mm:

(WebKit::activateSessionCleanup):
(WebKit::NetworkSessionCocoa::sessionWrapperForTask):

• Shared/Cocoa/DefaultWebBrowserChecks.h:
• Shared/Cocoa/DefaultWebBrowserChecks.mm:

(WebKit::doesParentProcessHaveITPEnabled):
(WebKit::isParentProcessAFullWebBrowser):

• WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:

(WebKit::WebCore::isWebBrowser):

• WebProcess/WebPage/WebPage.cpp:

(WebKit::WebPage::isParentProcessAWebBrowser const):

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• NetworkProcess/cocoa/NetworkSessionCocoa.mm (modified) (2 diffs)
• Shared/Cocoa/DefaultWebBrowserChecks.h (modified) (2 diffs)
• Shared/Cocoa/DefaultWebBrowserChecks.mm (modified) (3 diffs)
• WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp (modified) (1 diff)
• WebProcess/WebPage/WebPage.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-04-05  Alex Christensen  <achristensen@webkit.org>
+
+        Reduce crash inside getAuditToken
+        https://bugs.webkit.org/show_bug.cgi?id=224196
+        <rdar://74536285>
+
+        Reviewed by David Kilzer.
+
+        Something is crashing inside the call to getAuditToken, and I believe it is happening during teardown of the network process.
+        After many days of head scratching and many previous attempts at fixing this problem, it persists.
+        Since direct strategies at fixing this problem have failed, I now try something different.
+        Instead of calling getAuditToken at the beginning of every resource load to ask if the parent process has an entitlement,
+        I now call it only once per process.  That should make things faster and less crashy.  Otherwise no change in behavior.
+
+        * NetworkProcess/cocoa/NetworkSessionCocoa.mm:
+        (WebKit::activateSessionCleanup):
+        (WebKit::NetworkSessionCocoa::sessionWrapperForTask):
+        * Shared/Cocoa/DefaultWebBrowserChecks.h:
+        * Shared/Cocoa/DefaultWebBrowserChecks.mm:
+        (WebKit::doesParentProcessHaveITPEnabled):
+        (WebKit::isParentProcessAFullWebBrowser):
+        * WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp:
+        (WebKit::WebCore::isWebBrowser):
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::isParentProcessAWebBrowser const):
+
 2021-04-05  Chris Dumez  <cdumez@apple.com>
 

trunk/Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm

@@ -1146 +1146 @@
 
 #if !PLATFORM(IOS_FAMILY_SIMULATOR)
-    auto parentAuditToken = session.networkProcess().parentProcessConnection()->getAuditToken();
-    RELEASE_ASSERT(parentAuditToken); // This should be impossible.
-
-    bool itpEnabled = doesParentProcessHaveITPEnabled(parentAuditToken, parameters.appHasRequestedCrossWebsiteTrackingPermission);
+    bool itpEnabled = doesParentProcessHaveITPEnabled(session.networkProcess(), parameters.appHasRequestedCrossWebsiteTrackingPermission);
     bool passedEnabledState = session.isResourceLoadStatisticsEnabled();
 
@@ -1351 +1348 @@
 {
     auto shouldBeConsideredAppBound = isNavigatingToAppBoundDomain ? *isNavigatingToAppBoundDomain : NavigatingToAppBoundDomain::Yes;
-    if (RefPtr<IPC::Connection> connection = networkProcess().parentProcessConnection()) {
-        if (isParentProcessAFullWebBrowser(connection->getAuditToken()))
-            shouldBeConsideredAppBound = NavigatingToAppBoundDomain::No;
-    }
+    if (isParentProcessAFullWebBrowser(networkProcess()))
+        shouldBeConsideredAppBound = NavigatingToAppBoundDomain::No;
 #if ENABLE(RESOURCE_LOAD_STATISTICS)
     if (auto* storageSession = networkStorageSession()) {

trunk/Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.h

@@ -35 +35 @@
 namespace WebKit {
 
+class AuxiliaryProcess;
+
 bool hasRequestedCrossWebsiteTrackingPermission();
 bool hasProhibitedUsageStrings();
@@ -41 +43 @@
 void determineITPState();
 bool doesAppHaveITPEnabled();
-bool doesParentProcessHaveITPEnabled(Optional<audit_token_t>, bool);
+bool doesParentProcessHaveITPEnabled(AuxiliaryProcess&, bool hasRequestedCrossWebsiteTrackingPermission);
 bool isFullWebBrowser();
-bool isParentProcessAFullWebBrowser(Optional<audit_token_t>);
+bool isParentProcessAFullWebBrowser(AuxiliaryProcess&);
+
+#define WEBKIT_PARENT_PROCESS_FULL_WEB_BROWSER_PARAMETER_AUXILIARY_PROCESS 1
 
 } // namespace WebKit

trunk/Source/WebKit/Shared/Cocoa/DefaultWebBrowserChecks.mm

@@ -27 +27 @@
 #import "DefaultWebBrowserChecks.h"
 
+#import "AuxiliaryProcess.h"
+#import "Connection.h"
+#import "Logging.h"
 #import "TCCSPI.h"
 #import <WebCore/RegistrableDomain.h>
@@ -156 +159 @@
 }
 
-bool doesParentProcessHaveITPEnabled(Optional<audit_token_t> auditToken, bool hasRequestedCrossWebsiteTrackingPermissionValue)
+bool doesParentProcessHaveITPEnabled(AuxiliaryProcess& auxiliaryProcess, bool hasRequestedCrossWebsiteTrackingPermission)
 {
     ASSERT(isInWebKitChildProcess());
     ASSERT(RunLoop::isMain());
 
-    if (!isParentProcessAFullWebBrowser(auditToken) && !hasRequestedCrossWebsiteTrackingPermissionValue)
-        return true;
-
-    TCCAccessPreflightResult result = kTCCAccessPreflightDenied;
+    static bool itpEnabled { true };
+    static dispatch_once_t once;
+    dispatch_once(&once, ^{
+        if (!isParentProcessAFullWebBrowser(auxiliaryProcess) && !hasRequestedCrossWebsiteTrackingPermission)
+            return;
+
+        TCCAccessPreflightResult result = kTCCAccessPreflightDenied;
 #if (PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 140000) || (PLATFORM(MAC) && __MAC_OS_X_VERSION_MIN_REQUIRED >= 110000)
-    if (auditToken)
+        RefPtr<IPC::Connection> connection = auxiliaryProcess.parentProcessConnection();
+        if (!connection) {
+            ASSERT_NOT_REACHED();
+            RELEASE_LOG_ERROR(IPC, "Unable to get parent process connection");
+            return;
+        }
+
+        auto auditToken = connection->getAuditToken();
+        if (!auditToken) {
+            ASSERT_NOT_REACHED();
+            RELEASE_LOG_ERROR(IPC, "Unable to get parent process audit token");
+            return;
+        }
         result = TCCAccessPreflightWithAuditToken(getkTCCServiceWebKitIntelligentTrackingPrevention(), auditToken.value(), nullptr);
 #endif
-    return result != kTCCAccessPreflightDenied;
+        itpEnabled = result != kTCCAccessPreflightDenied;
+    });
+    return itpEnabled;
 }
 
@@ -208 +228 @@
 }
 
-bool isParentProcessAFullWebBrowser(Optional<audit_token_t> auditToken)
+bool isParentProcessAFullWebBrowser(AuxiliaryProcess& auxiliaryProcess)
 {
     ASSERT(isInWebKitChildProcess());
-    if (!auditToken)
-        return false;
-
-    static bool fullWebBrowser;
-
+
+    static bool fullWebBrowser { false };
     static dispatch_once_t once;
     dispatch_once(&once, ^{
-        fullWebBrowser = WTF::hasEntitlement(auditToken.value(), "com.apple.developer.web-browser");
-    });
-
-    return fullWebBrowser || isRunningTest(WebCore::applicationBundleIdentifier());
+        if (isRunningTest(WebCore::applicationBundleIdentifier())) {
+            fullWebBrowser = true;
+            return;
+        }
+
+        RefPtr<IPC::Connection> connection = auxiliaryProcess.parentProcessConnection();
+        if (!connection) {
+            ASSERT_NOT_REACHED();
+            RELEASE_LOG_ERROR(IPC, "Unable to get parent process connection");
+            return;
+        }
+
+        auto auditToken = connection->getAuditToken();
+        if (!auditToken) {
+            ASSERT_NOT_REACHED();
+            RELEASE_LOG_ERROR(IPC, "Unable to get parent process audit token");
+            return;
+        }
+
+        fullWebBrowser = WTF::hasEntitlement(*auditToken, "com.apple.developer.web-browser");
+    });
+
+    return fullWebBrowser;
 }
 

trunk/Source/WebKit/WebProcess/WebAuthentication/WebAuthenticatorCoordinator.cpp

@@ -51 +51 @@
 
 namespace {
-static bool isWebBrowser()
+inline bool isWebBrowser()
 {
-    if (auto* connection = WebProcess::singleton().parentProcessConnection())
-        return isParentProcessAFullWebBrowser(connection->getAuditToken());
-    return false;
+    return isParentProcessAFullWebBrowser(WebProcess::singleton());
 }
 }

trunk/Source/WebKit/WebProcess/WebPage/WebPage.cpp

@@ -3827 +3827 @@
 {
 #if HAVE(AUDIT_TOKEN)
-    if (auto* connection = WebProcess::singleton().parentProcessConnection())
-        return isParentProcessAFullWebBrowser(connection->getAuditToken());
+    return isParentProcessAFullWebBrowser(WebProcess::singleton());
 #endif
     return false;