| Summary: | Move ephemeral local storage from WebProcess to UIProcess | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Alex Christensen <achristensen> | ||||||||
| Component: | New Bugs | Assignee: | Alex Christensen <achristensen> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | cdumez, ggaren, tsavell, webkit-bug-importer | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Alex Christensen
2019-02-26 15:54:09 PST
Created attachment 363031 [details]
Patch
Comment on attachment 363031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363031&action=review r=me > Source/WebKit/UIProcess/WebStorage/StorageManager.h:113 > + bool m_ephemeral { false }; Let's call this m_isEphemeral. Comment on attachment 363031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363031&action=review > Source/WebKit/UIProcess/WebStorage/StorageManager.cpp:705 > void StorageManager::createTransientLocalStorageMap(IPC::Connection& connection, uint64_t storageMapID, uint64_t storageNamespaceID, SecurityOriginData&& topLevelOriginData, SecurityOriginData&& origin) > { > + ASSERT(!m_ephemeral); I think this code path may be reachable for frames subject to ITP or StorageAccess restriction. Probably worth svn blaming that function to find if tests were added for it. Comment on attachment 363031 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363031&action=review >> Source/WebKit/UIProcess/WebStorage/StorageManager.cpp:705 >> + ASSERT(!m_ephemeral); > > I think this code path may be reachable for frames subject to ITP or StorageAccess restriction. I added a test that loads a cross-origin iframe and uses localStorage, verified that transient in StorageNamespaceProvider::localStorageArea is true, and verified that this is not hit. I'll include that test addition. JK, the test verified that it *is* hit. I'm removing the assertion. Created attachment 363039 [details]
Patch
Created attachment 363041 [details]
Patch
The assertion no longer fires, which is nice. But do we need to update createTransientLocalStorageMap to do something useful for transient storage? Also, same comment about m_isEphemeral. Comment on attachment 363041 [details]
Patch
Transient storage is already transient, so it doesn't matter if it's ephemeral or not, it's always ephemeral. I moved the use of m_ephemeralStorage to after the calls to findStorageArea found nothing to not change transient behavior. I'll replace m_ephemeral with m_isEphemeral when I commit it.
It appears that the changes in https://trac.webkit.org/changeset/242122/webkit Broke an API test on debug: Failed TestWebKitAPI.WKWebView.InitializingWebViewWithEphemeralStorageDoesNotLog Saw unexpected logging: 'ERROR: Unable to create LocalStorage database path ' /Volumes/Data/slave/mojave-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewDoesNotLogDuringInitialization.mm:57 Failed build: https://build.webkit.org/builders/Apple%20Mojave%20Debug%20WK1%20%28Tests%29/builds/2305 |