Bug 195074 – Move ephemeral local storage from WebProcess to UIProcess

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Move ephemeral local storage from WebProcess to UIProcess

[Alex Christensen]

Created attachment 363031 [details]
Patch

[Geoffrey Garen]

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.

[Geoffrey Garen]

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.

[Geoffrey Garen]

Probably worth svn blaming that function to find if tests were added for it.

[Alex Christensen]

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.

[Alex Christensen]

JK, the test verified that it *is* hit.  I'm removing the assertion.

[Alex Christensen]

Created attachment 363039 [details]
Patch

[Alex Christensen]

Created attachment 363041 [details]
Patch

[Geoffrey Garen]

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.

[Alex Christensen]

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.

[Alex Christensen]

http://trac.webkit.org/r242122

[Radar WebKit Bug Importer]

<rdar://problem/48426546>

[Truitt Savell]

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

[Alex Christensen]

http://trac.webkit.org/r242131