Changeset 276983 in webkit

Timestamp:

May 4, 2021 2:32:05 PM (3 years ago)

Author:

achristensen@apple.com

Message:

localStorage changes aren't reflected between WKWebViews using WKWebViewConfiguration._groupIdentifier
​https://bugs.webkit.org/show_bug.cgi?id=225344
<rdar://77496721>

Reviewed by Brady Eidson.

Source/WebKit:

When we switched from using _pageGroup to _groupIdentifier, we also broke localStorage by making a new pageGroupID for each WKWebView.
This is the low-risk solution to this problem that remembers what pageGroupID we used for this same identifier, creating the effect of
using the same StorageNamespaceIdentifier for each _groupIdentifier like we did when we used _pageGroup.
See the comment in StorageAreaMap::dispatchLocalStorageEvent.

The higher-risk solution that needs to be done but probably shouldn't be merged to a branch at this point is to remove StorageNamespaceIdentifier.
I plan to do that in a follow-up patch.

• UIProcess/WebPageGroup.cpp:

(WebKit::pageGroupData):

Tools:

• TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:

(TEST):

Location:

trunk

Files:

• Source/WebKit/ChangeLog (modified) (1 diff)
• Source/WebKit/UIProcess/WebPageGroup.cpp (modified) (1 diff)
• Tools/ChangeLog (modified) (1 diff)
• Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm (modified) (2 diffs)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2021-05-04  Alex Christensen  <achristensen@webkit.org>
+
+        localStorage changes aren't reflected between WKWebViews using WKWebViewConfiguration._groupIdentifier
+        https://bugs.webkit.org/show_bug.cgi?id=225344
+        <rdar://77496721>
+
+        Reviewed by Brady Eidson.
+
+        When we switched from using _pageGroup to _groupIdentifier, we also broke localStorage by making a new pageGroupID for each WKWebView.
+        This is the low-risk solution to this problem that remembers what pageGroupID we used for this same identifier, creating the effect of
+        using the same StorageNamespaceIdentifier for each _groupIdentifier like we did when we used _pageGroup.
+        See the comment in StorageAreaMap::dispatchLocalStorageEvent.
+
+        The higher-risk solution that needs to be done but probably shouldn't be merged to a branch at this point is to remove StorageNamespaceIdentifier.
+        I plan to do that in a follow-up patch.
+
+        * UIProcess/WebPageGroup.cpp:
+        (WebKit::pageGroupData):
+
 2021-05-04  Jer Noble  <jer.noble@apple.com>
 

trunk/Source/WebKit/UIProcess/WebPageGroup.cpp

@@ -78 +78 @@
     WebPageGroupData data;
 
-    data.pageGroupID = generatePageGroupID();
+    static NeverDestroyed<HashMap<String, uint64_t>> map;
+    if (HashMap<String, uint64_t>::isValidKey(identifier)) {
+        data.pageGroupID = map.get().ensure(identifier, [] {
+            return generatePageGroupID();
+        }).iterator->value;
+    } else
+        data.pageGroupID = generatePageGroupID();
 
     if (!identifier.isEmpty())

trunk/Tools/ChangeLog

@@ +1 @@
+2021-05-04  Alex Christensen  <achristensen@webkit.org>
+
+        localStorage changes aren't reflected between WKWebViews using WKWebViewConfiguration._groupIdentifier
+        https://bugs.webkit.org/show_bug.cgi?id=225344
+        <rdar://77496721>
+
+        Reviewed by Brady Eidson.
+
+        * TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:
+        (TEST):
+
 2021-05-04  Aakash Jain  <aakash_jain@apple.com>
 

trunk/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm

@@ -28 +28 @@
 #import "PlatformUtilities.h"
 #import "Test.h"
+#import "TestNavigationDelegate.h"
+#import "TestUIDelegate.h"
 #import <WebKit/WKProcessPoolPrivate.h>
 #import <WebKit/WKUserContentControllerPrivate.h>
@@ -416 +418 @@
     finishedRunningScript = false;
 }
+
+TEST(WKWebView, LocalStorageGroup)
+{
+    auto runTest = [] (bool setGroupIdentifier) {
+        auto configuration = adoptNS([[WKWebViewConfiguration alloc] init]);
+        [configuration setWebsiteDataStore:[WKWebsiteDataStore nonPersistentDataStore]];
+        if (setGroupIdentifier)
+            [configuration _setGroupIdentifier:@"testgroupidentifier"];
+        auto webView1 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+        NSString *html1 = @""
+        "<script>"
+        "localStorage.setItem('testkey', 'testvalue1');"
+        "window.onstorage = function(e) { alert('storage changed key ' + e.key + ' value from ' + e.oldValue + ' to ' + e.newValue) };"
+        "</script>";
+        [webView1 loadHTMLString:html1 baseURL:[NSURL URLWithString:@"https://webkit.org/"]];
+        [webView1 _test_waitForDidFinishNavigation];
+
+        auto webView2 = adoptNS([[WKWebView alloc] initWithFrame:NSMakeRect(0, 0, 800, 600) configuration:configuration.get()]);
+        NSString *html2 = @""
+        "<script>"
+        "alert('second web view got value ' + localStorage.getItem('testkey'));"
+        "localStorage.setItem('testkey', 'testvalue2');"
+        "</script>";
+        [webView2 loadHTMLString:html2 baseURL:[NSURL URLWithString:@"https://webkit.org/"]];
+        EXPECT_WK_STREQ([webView2 _test_waitForAlert], "second web view got value testvalue1");
+
+        EXPECT_WK_STREQ([webView1 _test_waitForAlert], "storage changed key testkey value from testvalue1 to testvalue2");
+    };
+    runTest(true);
+    runTest(false);
+}