Bug 198317 – WKWebsiteDataStore API fails to fetch web storage data for non-persistent data store

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 370815 [details]
Patch

[Alex Christensen]

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.

[Sihui Liu]

Created attachment 370897 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 370897 [details]
Patch for landing

Clearing flags on attachment: 370897

Committed r245881: <https://trac.webkit.org/changeset/245881>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/51244662>

[Truitt Savell]

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

[Truitt Savell]

Reverted r245881 for reason:

Broke 13 Layout tests on WK2

Committed r245891: <https://trac.webkit.org/changeset/245891>

[Sihui Liu]

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.

[Sihui Liu]

Created attachment 370995 [details]
Patch for landing

[WebKit Commit Bot]

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 [e-mail]...
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

[Sihui Liu]

Created attachment 371012 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 371012 [details]
Patch for landing

Clearing flags on attachment: 371012

Committed r245943: <https://trac.webkit.org/changeset/245943>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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

[Ryan Haddad]

Reverted r245943 for reason:

Caused API test WKWebView.LocalStorageProcessSuspends to fail on release bots.

Committed r245978: <https://trac.webkit.org/changeset/245978>

[Sihui Liu]

(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.

[Sihui Liu]

Created attachment 371325 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 371325 [details]
Patch for landing

Clearing flags on attachment: 371325

Committed r246079: <https://trac.webkit.org/changeset/246079>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Roberts]

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"

[Sihui Liu]

(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?

[Shawn Roberts]

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"

[Sihui Liu]

(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.