In non-persistent session, we used SessionStorageNameSpace for localStorage and used in-memory map for sessionStorage. This is confusing, and makes it hard to maintain the same behavior for persistent session and non-persistent session.
Created attachment 370815 [details] Patch
Comment on attachment 370815 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370815&action=review R=me with a few comments, mostly stylistic. > Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp:94 > + Ref<StorageArea> getOrCreateStorageArea(SecurityOriginData&&, bool); bool isEphemeral Or enum class IsEphemeral : bool { No, Yes } > Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp:388 > + default: Let's remove this default. > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp:50 > + return createLocalStorageNamespace(identifier, quotaInBytes, true); Let's give this "true" a name. Either use enum class IsEphemeral : bool { No, Yes } or bool isEphemeral = true; > Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h:44 > - static Ref<StorageNamespaceImpl> createLocalStorageNamespace(uint64_t identifier, unsigned quotaInBytes); > + static Ref<StorageNamespaceImpl> createLocalStorageNamespace(uint64_t identifier, unsigned quotaInBytes, bool isEphemeral = false); I think with something like this we shouldn't have a default parameter so we don't forget to state whether it's ephemeral or not.
Created attachment 370897 [details] Patch for landing
Comment on attachment 370897 [details] Patch for landing Clearing flags on attachment: 370897 Committed r245881: <https://trac.webkit.org/changeset/245881>
All reviewed patches have been landed. Closing bug.
<rdar://problem/51244662>
It appears that the changes in https://trac.webkit.org/changeset/245881/webkit has broken 13 tests: imported/w3c/web-platform-tests/html/browsers/windows/auxiliary-browsing-contexts/opener-noopener.html imported/w3c/web-platform-tests/html/browsers/windows/auxiliary-browsing-contexts/opener-noreferrer.html imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_blank-002.html imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_parent-004.html imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_self-002.html imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_top-001.html imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_top-002.html imported/w3c/web-platform-tests/html/browsers/windows/browsing-context-names/choose-_top-003.html imported/w3c/web-platform-tests/html/browsers/windows/noreferrer-null-opener.html imported/w3c/web-platform-tests/html/browsers/windows/noreferrer.html storage/domstorage/events/basic-setattribute.html storage/domstorage/events/basic.html storage/domstorage/events/case-sensitive.html I reproduced this by running the tests on 245881 which failed and on 245880 which all passed. Results: https://build.webkit.org/results/Apple%20Mojave%20Release%20WK2%20(Tests)/r245884%20(4380)/results.html
Reverted r245881 for reason: Broke 13 Layout tests on WK2 Committed r245891: <https://trac.webkit.org/changeset/245891>
Comment on attachment 370897 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=370897&action=review > Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.cpp:80 > + return StorageNamespaceImpl::createLocalStorageNamespace(m_identifier, quota, StorageNamespaceImpl::IsEphemeral::Yes); Oops, this should be NO. Tested the fix on local build.
Created attachment 370995 [details] Patch for landing
Comment on attachment 370995 [details] Patch for landing Rejecting attachment 370995 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'apply-attachment', '--no-update', '--non-interactive', 370995, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit Logging in as commit-queue@webkit.org... Fetching: https://bugs.webkit.org/attachment.cgi?id=370995&action=edit Fetching: https://bugs.webkit.org/show_bug.cgi?id=198317&ctype=xml&excludefield=attachmentdata Processing 1 patch from 1 bug. Processing patch 370995 from bug 198317. Fetching: https://bugs.webkit.org/attachment.cgi?id=370995 Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Parsed 9 diffs from patch file(s). patching file Source/WebKit/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.cpp patching file Source/WebKit/NetworkProcess/WebStorage/StorageManager.h Hunk #1 succeeded at 103 (offset 2 lines). Hunk #2 FAILED at 112. 1 out of 2 hunks FAILED -- saving rejects to file Source/WebKit/NetworkProcess/WebStorage/StorageManager.h.rej patching file Source/WebKit/WebProcess/WebStorage/StorageAreaMap.cpp patching file Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.cpp patching file Source/WebKit/WebProcess/WebStorage/StorageNamespaceImpl.h patching file Source/WebKit/WebProcess/WebStorage/WebStorageNamespaceProvider.cpp patching file Tools/ChangeLog Hunk #1 succeeded at 1 with fuzz 3. patching file Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebsiteDatastore.mm Failed to run "[u'/Volumes/Data/EWS/WebKit/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Full output: https://webkit-queues.webkit.org/results/12333153
Created attachment 371012 [details] Patch for landing
Comment on attachment 371012 [details] Patch for landing Clearing flags on attachment: 371012 Committed r245943: <https://trac.webkit.org/changeset/245943>
This change appears to have caused the following API test failure: TestWebKitAPI.WKWebView.LocalStorageProcessSuspends /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:141 Value of: [@"value" isEqualToString:result] Actual: false Expected: true https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/builds/4400/steps/run-api-tests/logs/stdio
Reverted r245943 for reason: Caused API test WKWebView.LocalStorageProcessSuspends to fail on release bots. Committed r245978: <https://trac.webkit.org/changeset/245978>
(In reply to Ryan Haddad from comment #15) > This change appears to have caused the following API test failure: > > TestWebKitAPI.WKWebView.LocalStorageProcessSuspends > > > /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/ > WebKitCocoa/LocalStoragePersistence.mm:141 > Value of: [@"value" isEqualToString:result] > Actual: false > Expected: true > > https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20(Tests)/ > builds/4400/steps/run-api-tests/logs/stdio I cannot reproduce the test failure, but I do get flaky timeout on this test when running multiple times. I will fix the test first in https://bugs.webkit.org/show_bug.cgi?id=198450. As the failure didn't reveal too much information, I also changed to use EXPECT_WK_STREQ instead of EXPECT_TRUE in the fix above.
Created attachment 371325 [details] Patch for landing
Comment on attachment 371325 [details] Patch for landing Clearing flags on attachment: 371325 Committed r246079: <https://trac.webkit.org/changeset/246079>
It appears after patch re landed as r246079 the same API test is failing flakily on Release bots. I am trying to reproduce it locally and so far I cannot. Trying on a few other machines and will post repro steps if possible. Appears to be a different failure message than previously. https://build.webkit.org/builders/Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/4486/steps/run-api-tests/logs/stdio TestWebKitAPI.WKWebView.LocalStorageProcessSuspends _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:142 Expected equality of these values: @"value" Which is: "value" result Which is: "newValue"
(In reply to Shawn Roberts from comment #21) > It appears after patch re landed as r246079 the same API test is failing > flakily on Release bots. > > I am trying to reproduce it locally and so far I cannot. Trying on a few > other machines and will post repro steps if possible. Appears to be a > different failure message than previously. > > https://build.webkit.org/builders/ > Apple%20Mojave%20Release%20WK2%20%28Tests%29/builds/4486/steps/run-api-tests/ > logs/stdio > > TestWebKitAPI.WKWebView.LocalStorageProcessSuspends > _RegisterApplication(), FAILED TO establish the default connection > to the WindowServer, _CGSDefaultConnection() is NULL. > _RegisterApplication(), FAILED TO establish the default connection > to the WindowServer, _CGSDefaultConnection() is NULL. > > > /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/ > WebKitCocoa/LocalStoragePersistence.mm:142 > Expected equality of these values: > @"value" > Which is: "value" > result > Which is: "newValue" I cannot reproduce the failure either. If you find the failure shows up on some bot very frequently, probably we want to debug on that bot?
Was able to reproduce with a release build. run-api-tests TestWebKitAPI.WKWebView.LocalStorageProcessSuspends --iter 5 Failed TestWebKitAPI.WKWebView.LocalStorageProcessSuspends _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. _RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/LocalStoragePersistence.mm:142 Expected equality of these values: @"value" Which is: "value" result Which is: "newValue"
(In reply to Shawn Roberts from comment #23) > Was able to reproduce with a release build. > > run-api-tests TestWebKitAPI.WKWebView.LocalStorageProcessSuspends --iter 5 > > Failed > > TestWebKitAPI.WKWebView.LocalStorageProcessSuspends > _RegisterApplication(), FAILED TO establish the default connection > to the WindowServer, _CGSDefaultConnection() is NULL. > _RegisterApplication(), FAILED TO establish the default connection > to the WindowServer, _CGSDefaultConnection() is NULL. > > > /Volumes/Data/slave/mojave-release/build/Tools/TestWebKitAPI/Tests/ > WebKitCocoa/LocalStoragePersistence.mm:142 > Expected equality of these values: > @"value" > Which is: "value" > result > Which is: "newValue" https://bugs.webkit.org/show_bug.cgi?id=198582.