Bug 201655 - Remove unnecessary abstractions around WebsiteDataStore
Summary: Remove unnecessary abstractions around WebsiteDataStore
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-09-10 13:42 PDT by Alex Christensen
Modified: 2019-09-20 17:13 PDT (History)
18 users (show)

See Also:


Attachments
Patch (182.08 KB, patch)
2019-09-10 13:47 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (180.80 KB, patch)
2019-09-10 13:51 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (182.41 KB, patch)
2019-09-10 13:59 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (185.48 KB, patch)
2019-09-10 14:04 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (192.85 KB, patch)
2019-09-10 14:11 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (193.48 KB, patch)
2019-09-10 14:14 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (199.92 KB, patch)
2019-09-10 14:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (200.76 KB, patch)
2019-09-10 14:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (204.26 KB, patch)
2019-09-10 15:40 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (204.94 KB, patch)
2019-09-10 16:31 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (207.05 KB, patch)
2019-09-10 19:29 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (207.57 KB, patch)
2019-09-10 21:24 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Updated patch with GLib API fixes (202.47 KB, patch)
2019-09-11 03:41 PDT, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Patch (161.71 KB, patch)
2019-09-20 16:12 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (164.04 KB, patch)
2019-09-20 16:20 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (165.45 KB, patch)
2019-09-20 16:35 PDT, 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-09-10 13:42:16 PDT
Remove unnecessary abstractions around WebsiteDataStore
Comment 1 Alex Christensen 2019-09-10 13:47:19 PDT
Created attachment 378483 [details]
Patch
Comment 2 Alex Christensen 2019-09-10 13:51:46 PDT
Created attachment 378484 [details]
Patch
Comment 3 EWS Watchlist 2019-09-10 13:52:20 PDT
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
Comment 4 Alex Christensen 2019-09-10 13:59:48 PDT
Created attachment 378485 [details]
Patch
Comment 5 Alex Christensen 2019-09-10 14:04:35 PDT
Created attachment 378486 [details]
Patch
Comment 6 Alex Christensen 2019-09-10 14:11:05 PDT
Created attachment 378487 [details]
Patch
Comment 7 Alex Christensen 2019-09-10 14:14:14 PDT
Created attachment 378488 [details]
Patch
Comment 8 Alex Christensen 2019-09-10 14:23:45 PDT
Created attachment 378493 [details]
Patch
Comment 9 Alex Christensen 2019-09-10 14:40:03 PDT
Created attachment 378494 [details]
Patch
Comment 10 Alex Christensen 2019-09-10 15:40:49 PDT
Created attachment 378501 [details]
Patch
Comment 11 Alex Christensen 2019-09-10 15:42:48 PDT
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.
Comment 12 Alex Christensen 2019-09-10 16:31:07 PDT
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...
Comment 13 Alex Christensen 2019-09-10 16:31:55 PDT
Created attachment 378509 [details]
Patch
Comment 14 Alex Christensen 2019-09-10 19:29:10 PDT
Created attachment 378529 [details]
Patch
Comment 15 Alex Christensen 2019-09-10 21:24:07 PDT
Created attachment 378537 [details]
Patch
Comment 16 youenn fablet 2019-09-11 03:14:55 PDT
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?
Comment 17 Carlos Garcia Campos 2019-09-11 03:41:50 PDT
Created attachment 378554 [details]
Updated patch with GLib API fixes

Updated patch to fix GTK and WPE ports.
Comment 18 Chris Dumez 2019-09-11 08:31:47 PDT
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 };
Comment 19 Alex Christensen 2019-09-11 09:37:59 PDT
(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
Comment 20 Radar WebKit Bug Importer 2019-09-11 09:38:23 PDT
<rdar://problem/55263711>
Comment 21 Ryan Haddad 2019-09-11 14:27:24 PDT
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
Comment 22 Ryan Haddad 2019-09-11 14:28:24 PDT
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
Comment 23 Ryan Haddad 2019-09-11 14:44:01 PDT
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>
Comment 24 Ryan Haddad 2019-09-11 14:44:28 PDT
(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.
Comment 25 Alex Christensen 2019-09-20 16:12:16 PDT
Created attachment 379279 [details]
Patch
Comment 26 Alex Christensen 2019-09-20 16:20:44 PDT
Created attachment 379280 [details]
Patch
Comment 27 Alex Christensen 2019-09-20 16:35:01 PDT
Created attachment 379282 [details]
Patch
Comment 28 Alex Christensen 2019-09-20 17:13:40 PDT
http://trac.webkit.org/r250169