Bug 195074 - Move ephemeral local storage from WebProcess to UIProcess
Summary: Move ephemeral local storage from WebProcess to UIProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-02-26 15:54 PST by Alex Christensen
Modified: 2019-02-27 10:05 PST (History)
4 users (show)

See Also:


Attachments
Patch (26.54 KB, patch)
2019-02-26 16:02 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (27.37 KB, patch)
2019-02-26 16:51 PST, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (26.96 KB, patch)
2019-02-26 17:19 PST, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2019-02-26 15:54:09 PST
Move ephemeral local storage from WebProcess to UIProcess
Comment 1 Alex Christensen 2019-02-26 16:02:39 PST
Created attachment 363031 [details]
Patch
Comment 2 Geoffrey Garen 2019-02-26 16:15:39 PST
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 3 Geoffrey Garen 2019-02-26 16:31:26 PST
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.
Comment 4 Geoffrey Garen 2019-02-26 16:31:45 PST
Probably worth svn blaming that function to find if tests were added for it.
Comment 5 Alex Christensen 2019-02-26 16:36:52 PST
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.
Comment 6 Alex Christensen 2019-02-26 16:50:19 PST
JK, the test verified that it *is* hit.  I'm removing the assertion.
Comment 7 Alex Christensen 2019-02-26 16:51:10 PST
Created attachment 363039 [details]
Patch
Comment 8 Alex Christensen 2019-02-26 17:19:17 PST
Created attachment 363041 [details]
Patch
Comment 9 Geoffrey Garen 2019-02-26 18:59:57 PST
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 10 Alex Christensen 2019-02-26 20:37:01 PST
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.
Comment 11 Alex Christensen 2019-02-26 20:41:04 PST
http://trac.webkit.org/r242122
Comment 12 Radar WebKit Bug Importer 2019-02-26 20:42:22 PST
<rdar://problem/48426546>
Comment 13 Truitt Savell 2019-02-27 08:28:21 PST
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
Comment 14 Alex Christensen 2019-02-27 10:05:18 PST
http://trac.webkit.org/r242131