Bug 201655 – Remove unnecessary abstractions around WebsiteDataStore

• Component: New Bugs
• Status: RESOLVED FIXED

[Alex Christensen]

Remove unnecessary abstractions around WebsiteDataStore

[Alex Christensen]

Created attachment 378483 [details]
Patch

[Alex Christensen]

Created attachment 378484 [details]
Patch

[EWS Watchlist]

Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API

[Alex Christensen]

Created attachment 378485 [details]
Patch

[Alex Christensen]

Created attachment 378486 [details]
Patch

[Alex Christensen]

Created attachment 378487 [details]
Patch

[Alex Christensen]

Created attachment 378488 [details]
Patch

[Alex Christensen]

Created attachment 378493 [details]
Patch

[Alex Christensen]

Created attachment 378494 [details]
Patch

[Alex Christensen]

Created attachment 378501 [details]
Patch

[Alex Christensen]

Comment on attachment 378501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378501&action=review

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:-314
> -static inline Ref<WebsiteDataStoreConfiguration> websiteDataStoreConfigurationForWebProcessPoolConfiguration(const API::ProcessPoolConfiguration& processPoolconfigurarion)

GTK developers:
I think this will not affect anything because the defaults should come from the WebsiteDataStore, but it would probably be worth double checking.

> Source/WebKit/UIProcess/API/glib/WebKitWebContext.cpp:1450
> -void webkit_web_context_set_disk_cache_directory(WebKitWebContext* context, const char* directory)
> +void webkit_web_context_set_disk_cache_directory(WebKitWebContext*, const char*)

Heads up GTK developers:
I'm cutting off the functionality of this function which has been deprecated for many years, anticipating this move.

[Alex Christensen]

Comment on attachment 378501 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378501&action=review

> Source/WebKit/UIProcess/WebProcessPool.cpp:-888
> -    if (parameters.applicationCacheFlatFileSubdirectoryName.isEmpty())

This needs a default on the WebsiteDataStoreConfiguration to maintain existing behavior and continue to pass the appcache video tests.  Uploading what I believe will be the last patch needed...

[Alex Christensen]

Created attachment 378509 [details]
Patch

[Alex Christensen]

Created attachment 378529 [details]
Patch

[Alex Christensen]

Created attachment 378537 [details]
Patch

[youenn fablet]

Comment on attachment 378537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=378537&action=review

> Source/WebKit/UIProcess/WebProcessPool.cpp:547
> +    if (!m_websiteDataStore && WebsiteDataStore::defaultDataStoreExists()) {

This check seems strange given we have above an if(m_websiteDataStore) check.
Also it is IDB-support conditional.
Should we move that check at the beginning of WebProcessPool::ensureNetworkProcess?

[Carlos Garcia Campos]

Created attachment 378554 [details]
Updated patch with GLib API fixes

Updated patch to fix GTK and WPE ports.

[Chris Dumez]

Comment on attachment 378554 [details]
Updated patch with GLib API fixes

View in context: https://bugs.webkit.org/attachment.cgi?id=378554&action=review

r=me

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:106
> +    explicit WebsiteDataStore(Ref<WebsiteDataStoreConfiguration>&&, PAL::SessionID);

explicit is not needed.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.h:305
> +    enum ShouldCreateDirectory { CreateDirectory, DontCreateDirectory };

Could you please use an enum class ? E.g. enum class ShouldCreateDirectory { No, Yes };

[Alex Christensen]

(In reply to youenn fablet from comment #16)
> Comment on attachment 378537 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378537&action=review
> 
> > Source/WebKit/UIProcess/WebProcessPool.cpp:547
> > +    if (!m_websiteDataStore && WebsiteDataStore::defaultDataStoreExists()) {
> 
> This check seems strange given we have above an if(m_websiteDataStore) check.
> Also it is IDB-support conditional.
> Should we move that check at the beginning of
> WebProcessPool::ensureNetworkProcess?

All the code that uses m_websiteDataStore will be seriously refactored soon.

http://trac.webkit.org/r249768

[Radar WebKit Bug Importer]

<rdar://problem/55263711>

[Ryan Haddad]

The following API tests are failing an assertion after this change:

TestWebKitAPI.Coding.WKWebsiteDataStore_Default
TestWebKitAPI.Coding.WKWebsiteDataStore_NonPersistent
TestWebKitAPI.Coding.WKWebViewConfiguration
TestWebKitAPI.Coding.WKWebView

        ASSERTION FAILED: dataStores().contains(this)
        /Volumes/Data/slave/mojave-debug/build/Source/WebKit/UIProcess/WebsiteData/Cocoa/WebsiteDataStoreCocoa.mm(220) : void WebKit::WebsiteDataStore::platformDestroy()
        1   0x1094131e9 WTFCrash
        2   0x114b2539b WTFCrashWithInfo(int, char const*, char const*, int)
        3   0x11580ae8b WebKit::WebsiteDataStore::platformDestroy()
        4   0x115ac30b8 WebKit::WebsiteDataStore::~WebsiteDataStore()
        5   0x115ac3595 WebKit::WebsiteDataStore::~WebsiteDataStore()
        6   0x1154161ce -[WKWebsiteDataStore dealloc]
        7   0x11541636a -[WKWebsiteDataStore initWithCoder:]
        8   0x7fff39ac7417 _decodeObjectBinary
        9   0x7fff39ac691e _decodeObject
        10  0x7fff39ac681a -[NSKeyedUnarchiver decodeObjectForKey:]
        11  0x7fff39ac6687 -[NSKeyedUnarchiver decodeObjectOfClasses:forKey:]
        12  0x7fff39ac62a2 -[NSCoder(Exceptions) __tryDecodeObjectForKey:error:decodeBlock:]
        13  0x7fff39ac625b -[NSCoder decodeTopLevelObjectOfClasses:forKey:error:]
        14  0x7fff39ac517a +[NSKeyedUnarchiver unarchivedObjectOfClasses:fromData:error:]
        15  0x10759f36b unarchivedObjectOfClassesFromData(NSSet<objc_class*>*, NSData*)
        16  0x1076238bd WTF::RetainPtr<WKWebsiteDataStore> encodeAndDecode<WKWebsiteDataStore>(WKWebsiteDataStore*)
        17  0x1076236b6 Coding_WKWebsiteDataStore_Default_Test::TestBody()
        18  0x107dad45e void testing::internal::HandleSehExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        19  0x107d7ab5b void testing::internal::HandleExceptionsInMethodIfSupported<testing::Test, void>(testing::Test*, void (testing::Test::*)(), char const*)
        20  0x107d7aa86 testing::Test::Run()
        21  0x107d7bc95 testing::TestInfo::Run()
        22  0x107d7cb7f testing::TestCase::Run()
        23  0x107d88834 testing::internal::UnitTestImpl::RunAllTests()
        24  0x107db15ee bool testing::internal::HandleSehExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
        25  0x107d8830b bool testing::internal::HandleExceptionsInMethodIfSupported<testing::internal::UnitTestImpl, bool>(testing::internal::UnitTestImpl*, bool (testing::internal::UnitTestImpl::*)(), char const*)
        26  0x107d881e0 testing::UnitTest::Run()
        27  0x107b99011 RUN_ALL_TESTS()
        28  0x107b98fa1 TestWebKitAPI::TestsController::run(int, char**)
        29  0x107d4572e main
        30  0x7fff637b53d5 start
        31  0x2

[Ryan Haddad]

Also, the perf bot has 201 tests failing with this in the output:
2019-09-11 12:42:29.146 com.apple.WebKit.Networking.Development[36125:12688643] NetworkStorageDB:_openDBReadConnections: failed to open read connection to DB @ /Users/buildbot/Library/Caches/com.apple.WebKit.Networking/Cache.db.  Error=14. Cause=unable to open database file
2019-09-11 12:42:29.146 com.apple.WebKit.Networking.Development[36125:12688643] CacheRead: unable to open cache files in /Users/buildbot/Library/Caches/com.apple.WebKit.Networking
2019-09-11 12:42:29.147 com.apple.WebKit.Networking.Development[36125:12688643] Failed to obtain sandbox extension for path=/Users/buildbot/Library/Caches/com.apple.WebKit.Networking. Errno:1


https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Perf%29/builds/2400

[Ryan Haddad]

Reverted r249768 for reason:

Caused 4 API tests to assert, perf test failures, and layout test crashes under ASan

Committed r249778: <https://trac.webkit.org/changeset/249778>

[Ryan Haddad]

(In reply to Ryan Haddad from comment #23)
> Reverted r249768 for reason:
> 
> Caused 4 API tests to assert, perf test failures, and layout test crashes
> under ASan
> 
> Committed r249778: <https://trac.webkit.org/changeset/249778>
ASan crash details in radar.

[Alex Christensen]

Created attachment 379279 [details]
Patch

[Alex Christensen]

Created attachment 379280 [details]
Patch

[Alex Christensen]

Created attachment 379282 [details]
Patch

[Alex Christensen]

http://trac.webkit.org/r250169