Changeset 263545 in webkit

Timestamp:

Jun 25, 2020 4:43:38 PM (4 years ago)

Author:

Chris Dumez

Message:

[iOS] Network process is crashing when launching TJMaxx app due to invalid NetworkProcess::DestroySession IPC message
​https://bugs.webkit.org/show_bug.cgi?id=213625
<rdar://problem/64737890>

Reviewed by Geoffrey Garen.

The app is calling [WKWebsiteDataStore init] despite the method being marked as unavailable in
WKWebsiteDataStore.h. As a result, they end up with a WKWebsiteDataStore object whose internal
_websiteDataStore is bad because its constructor was never called. When [WKWebsiteDataStore dealloc]
gets called later own, it calls the ~WebsiteDataStore() destructor for _websiteDataStore but its
m_sessionID is 0 because we never called the constructor. This causes us to send a
NetworkProcess::DestroySession IPC with a sessionID that is 0, which is not valid so the
NetworkProcess crashes.

To address the issue, we now provide an implementation of [WKWebsiteDataStore init] which returns nil
behind a linked-on-after check (since this crashes the app). To keep the app working,
[WKWebsiteDataStore init] returns the default data store until rebuilt with the new SDK. Note that
I tried returning a new ephemeral data store instead but this was getting the app in a bad state.

• UIProcess/API/Cocoa/WKWebsiteDataStore.h:

Mark "new" as unavailable, otherwise [WKWebsiteDataStore new] builds.

• UIProcess/API/Cocoa/WKWebsiteDataStore.mm:

(-[WKWebsiteDataStore init]):
Return nil with latest SDK, the default data store otherwise.

• UIProcess/Cocoa/VersionChecks.h:

Add linked-on-after check.

• UIProcess/WebsiteData/WebsiteDataStore.cpp:

(WebKit::WebsiteDataStore::~WebsiteDataStore):
Add a release assertion to make sure that m_sessionID is always valid when the destructor is called.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/API/Cocoa/WKWebsiteDataStore.h (modified) (1 diff)
• UIProcess/API/Cocoa/WKWebsiteDataStore.mm (modified) (2 diffs)
• UIProcess/Cocoa/VersionChecks.h (modified) (2 diffs)
• UIProcess/WebsiteData/WebsiteDataStore.cpp (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2020-06-25  Chris Dumez  <cdumez@apple.com>
+
+        [iOS] Network process is crashing when launching TJMaxx app due to invalid NetworkProcess::DestroySession IPC message
+        https://bugs.webkit.org/show_bug.cgi?id=213625
+        <rdar://problem/64737890>
+
+        Reviewed by Geoffrey Garen.
+
+        The app is calling [WKWebsiteDataStore init] despite the method being marked as unavailable in
+        WKWebsiteDataStore.h. As a result, they end up with a WKWebsiteDataStore object whose internal
+        _websiteDataStore is bad because its constructor was never called. When [WKWebsiteDataStore dealloc]
+        gets called later own, it calls the ~WebsiteDataStore() destructor for _websiteDataStore but its
+        m_sessionID is 0 because we never called the constructor. This causes us to send a
+        NetworkProcess::DestroySession IPC with a sessionID that is 0, which is not valid so the
+        NetworkProcess crashes.
+
+        To address the issue, we now provide an implementation of [WKWebsiteDataStore init] which returns nil
+        behind a linked-on-after check (since this crashes the app). To keep the app working,
+        [WKWebsiteDataStore init] returns the default data store until rebuilt with the new SDK. Note that
+        I tried returning a new ephemeral data store instead but this was getting the app in a bad state.
+
+        * UIProcess/API/Cocoa/WKWebsiteDataStore.h:
+        Mark "new" as unavailable, otherwise [WKWebsiteDataStore new] builds.
+
+        * UIProcess/API/Cocoa/WKWebsiteDataStore.mm:
+        (-[WKWebsiteDataStore init]):
+        Return nil with latest SDK, the default data store otherwise.
+
+        * UIProcess/Cocoa/VersionChecks.h:
+        Add linked-on-after check.
+
+        * UIProcess/WebsiteData/WebsiteDataStore.cpp:
+        (WebKit::WebsiteDataStore::~WebsiteDataStore):
+        Add a release assertion to make sure that m_sessionID is always valid when the destructor is called.
+
 2020-06-25  Daniel Bates  <dabates@apple.com>
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.h

@@ -48 +48 @@
 + (WKWebsiteDataStore *)nonPersistentDataStore;
 
+- (instancetype)new NS_UNAVAILABLE;
 - (instancetype)init NS_UNAVAILABLE;
 

trunk/Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm

@@ -31 +31 @@
 #import "CompletionHandlerCallChecker.h"
 #import "ShouldGrandfatherStatistics.h"
+#import "VersionChecks.h"
 #import "WKHTTPCookieStoreInternal.h"
 #import "WKNSArray.h"
@@ -116 +117 @@
 }
 
+- (instancetype)init
+{
+    // This is a workaround for apps that were managing to call [WKWebsiteDataStore init].
+    // FIXME: We should eventually drop this and always return nil.
+    if (!WebKit::linkedOnOrAfter(WebKit::SDKVersion::FirstWithWKWebsiteDataStoreInitReturningNil)) {
+        RELEASE_LOG_ERROR(Process, "Application is calling [WKWebsiteDataStore init], which is not supported");
+        return [WKWebsiteDataStore defaultDataStore];
+    }
+    return nil;
+}
+
 - (void)dealloc
 {

trunk/Source/WebKit/UIProcess/Cocoa/VersionChecks.h

@@ -95 +95 @@
     FirstThatSendsNativeMouseEvents = DYLD_IOS_VERSION_13_4,
     FirstWithInitializeWebKit2MainThreadAssertion = DYLD_IOS_VERSION_14_0,
+    FirstWithWKWebsiteDataStoreInitReturningNil = DYLD_IOS_VERSION_14_0,
 #elif PLATFORM(MAC)
     FirstWithNetworkCache = DYLD_MACOSX_VERSION_10_11,
@@ -108 +109 @@
     FirstWithSessionCleanupByDefault = DYLD_MACOS_VERSION_FIRST_WITH_SESSION_CLEANUP_BY_DEFAULT,
     FirstWithInitializeWebKit2MainThreadAssertion = DYLD_MACOSX_VERSION_10_16,
+    FirstWithWKWebsiteDataStoreInitReturningNil = DYLD_MACOSX_VERSION_10_16,
 #endif
 };

trunk/Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp

@@ -125 +125 @@
 {
     ASSERT(RunLoop::isMain());
+    RELEASE_ASSERT(m_sessionID.isValid());
 
     platformDestroy();