Bug 196281 - Resource Load Statistics: IPC to the WebsiteDataStore in the UI process from NetworkProcess::deleteWebsiteDataForRegistrableDomains()
Summary: Resource Load Statistics: IPC to the WebsiteDataStore in the UI process from ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: John Wilander
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-26 17:27 PDT by John Wilander
Modified: 2019-03-28 18:16 PDT (History)
5 users (show)

See Also:


Attachments
Patch (49.72 KB, patch)
2019-03-26 18:01 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.37 KB, patch)
2019-03-27 12:05 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.32 KB, patch)
2019-03-27 12:19 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.50 KB, patch)
2019-03-27 16:13 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.52 KB, patch)
2019-03-28 13:32 PDT, John Wilander
no flags Details | Formatted Diff | Diff
Patch (46.53 KB, patch)
2019-03-28 14:31 PDT, John Wilander
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description John Wilander 2019-03-26 17:27:04 PDT
The move of ITP to the network process requires that it calls the UI process when clearing website data (previously the other way around).
Comment 1 John Wilander 2019-03-26 17:27:21 PDT
<rdar://problem/48938748>
Comment 2 John Wilander 2019-03-26 18:01:20 PDT
Created attachment 366029 [details]
Patch
Comment 3 Build Bot 2019-03-26 18:03:30 PDT
Attachment 366029 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/WebsiteData/WebsiteData.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 John Wilander 2019-03-26 18:04:37 PDT
(In reply to Build Bot from comment #3)
> Attachment 366029 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebKit/Shared/WebsiteData/WebsiteData.h:42:  enum members
> should use InterCaps with an initial capital letter or initial 'k' for
> C-style enums.  [readability/enum_casing] [4]
> Total errors found: 1 in 25 files
> 
> 
> If any of these errors are false positives, please file a bug against
> check-webkit-style.

I belive the style checker is wrong, complaining about the capitalization of:

enum class WebsiteDataProcessType { Network, UI };
Comment 5 John Wilander 2019-03-26 19:17:00 PDT
Non-Cocoa build failures seem to be about a new convenience function not being used. It’s currently only used in my code which is behind the Resource Load Statistics flag.

The mac-debug test failures are unrelated.
Comment 6 John Wilander 2019-03-27 12:05:04 PDT
Created attachment 366088 [details]
Patch
Comment 7 Alex Christensen 2019-03-27 12:06:18 PDT
Comment on attachment 366029 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1688
> +        parentProcessConnection()->send(Messages::NetworkProcessProxy::DeleteWebsiteDataInUIProcessForRegistrableDomains(sessionID, dataTypesForUIProcess, fetchOptions, domainsToDeleteAllButCookiesFor, callbackID), 0);

This seems like a prime candidate for sendWithAsyncReply.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.cpp:117
> +bool WebsiteDataRecord::matches(const RegistrableDomain& domain) const

This needs a WebCore:: or we need a using namespace WebCore inside the namespace WebKit.

> Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2075
> +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("IsStatisticsHasLocalStorage"));

auto messageName = adoptWK(...)

> Tools/WebKitTestRunner/TestInvocation.cpp:1435
> +        WKRetainPtr<WKTypeRef> result(AdoptWK, WKBooleanCreate(hasLocalStorage));

auto result = adoptWK(...)
Comment 8 Build Bot 2019-03-27 12:06:50 PDT
Attachment 366088 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/WebsiteData/WebsiteData.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 9 John Wilander 2019-03-27 12:14:34 PDT
Sorry, I saw your comments after preparing the latest patch.

(In reply to Alex Christensen from comment #7)
> Comment on attachment 366029 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366029&action=review
> 
> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1688
> > +        parentProcessConnection()->send(Messages::NetworkProcessProxy::DeleteWebsiteDataInUIProcessForRegistrableDomains(sessionID, dataTypesForUIProcess, fetchOptions, domainsToDeleteAllButCookiesFor, callbackID), 0);
> 
> This seems like a prime candidate for sendWithAsyncReply.

Yes. Chris pointed this out in-person. Fixed in the latest patch.

> > Source/WebKit/UIProcess/WebsiteData/WebsiteDataRecord.cpp:117
> > +bool WebsiteDataRecord::matches(const RegistrableDomain& domain) const
> 
> This needs a WebCore:: or we need a using namespace WebCore inside the
> namespace WebKit.

Will fix.

> > Tools/WebKitTestRunner/InjectedBundle/TestRunner.cpp:2075
> > +    WKRetainPtr<WKStringRef> messageName(AdoptWK, WKStringCreateWithUTF8CString("IsStatisticsHasLocalStorage"));
> 
> auto messageName = adoptWK(...)

Will fix.

> > Tools/WebKitTestRunner/TestInvocation.cpp:1435
> > +        WKRetainPtr<WKTypeRef> result(AdoptWK, WKBooleanCreate(hasLocalStorage));
> 
> auto result = adoptWK(...)

Will fix.

Thanks!
Comment 10 John Wilander 2019-03-27 12:19:40 PDT
Created attachment 366091 [details]
Patch
Comment 11 Build Bot 2019-03-27 12:21:31 PDT
Attachment 366091 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/WebsiteData/WebsiteData.h:42:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 John Wilander 2019-03-27 13:06:16 PDT
Non-Cocoa build failures are probably because of a missing header include.
Comment 13 John Wilander 2019-03-27 16:13:06 PDT
Created attachment 366125 [details]
Patch
Comment 14 Build Bot 2019-03-27 16:20:52 PDT
Attachment 366125 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/WebsiteData/WebsiteData.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 15 Alex Christensen 2019-03-27 17:07:05 PDT
Comment on attachment 366125 [details]
Patch

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

> Source/WebKit/Shared/WebsiteData/WebsiteData.cpp:98
> +    case WebsiteDataType::MemoryCache:
> +        return WebsiteDataProcessType::UI;

This is in the web process.

> Source/WebKit/Shared/WebsiteData/WebsiteData.cpp:106
> +    case WebsiteDataType::WebSQLDatabases:
> +        return WebsiteDataProcessType::UI;

I'm not sure if this is correct.  I thought this was in the Network process.
Comment 16 Alex Christensen 2019-03-27 17:08:39 PDT
Comment on attachment 366125 [details]
Patch

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

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:627
> +void WebsiteDataStore::fetchDataForRegistrableDomains(OptionSet<WebsiteDataType> dataTypes, OptionSet<WebsiteDataFetchOption> fetchOptions, const Vector<RegistrableDomain>& domains, CompletionHandler<void(Vector<WebsiteDataRecord>&&, HashSet<RegistrableDomain>&&)>&& completionHandler)

Same needing WebCore:: or using namespace WebCore here.
Comment 17 John Wilander 2019-03-28 13:28:55 PDT
(In reply to Alex Christensen from comment #15)
> Comment on attachment 366125 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=366125&action=review
> 
> > Source/WebKit/Shared/WebsiteData/WebsiteData.cpp:98
> > +    case WebsiteDataType::MemoryCache:
> > +        return WebsiteDataProcessType::UI;
> 
> This is in the web process.

Sure. My thinking was that the WebsiteDataStore in the UI process manages removal of this type but you're absolutely right.

> > Source/WebKit/Shared/WebsiteData/WebsiteData.cpp:106
> > +    case WebsiteDataType::WebSQLDatabases:
> > +        return WebsiteDataProcessType::UI;
> 
> I'm not sure if this is correct.  I thought this was in the Network process.

Doesn't look like it. WebKit::NetworkProcess doesn't handle the type whereas WebKit::WebsiteDataStore does, like so:

    if (dataTypes.contains(WebsiteDataType::WebSQLDatabases) && isPersistent()) {
        callbackAggregator->addPendingCallback();

        m_queue->dispatch([webSQLDatabaseDirectory = m_configuration->webSQLDatabaseDirectory().isolatedCopy(), callbackAggregator, modifiedSince] {
            WebCore::DatabaseTracker::trackerWithDatabasePath(webSQLDatabaseDirectory)->deleteDatabasesModifiedSince(modifiedSince);

            RunLoop::main().dispatch([callbackAggregator] {
                callbackAggregator->removePendingCallback();
            });
        });
    }
Comment 18 John Wilander 2019-03-28 13:32:25 PDT
Created attachment 366198 [details]
Patch
Comment 19 Build Bot 2019-03-28 13:34:50 PDT
Attachment 366198 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/WebsiteData/WebsiteData.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 20 John Wilander 2019-03-28 14:31:23 PDT
Created attachment 366206 [details]
Patch
Comment 21 John Wilander 2019-03-28 14:31:59 PDT
Trying to fix the Wincairo build failure. :/
Comment 22 Build Bot 2019-03-28 14:34:18 PDT
Attachment 366206 [details] did not pass style-queue:


ERROR: Source/WebKit/Shared/WebsiteData/WebsiteData.h:43:  enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums.  [readability/enum_casing] [4]
Total errors found: 1 in 24 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 23 John Wilander 2019-03-28 16:53:47 PDT
Filed https://bugs.webkit.org/show_bug.cgi?id=196374 on the style checker issue.
Comment 24 John Wilander 2019-03-28 17:48:30 PDT
Comment on attachment 366206 [details]
Patch

Thank you, Alex!
Comment 25 WebKit Commit Bot 2019-03-28 18:16:05 PDT
Comment on attachment 366206 [details]
Patch

Clearing flags on attachment: 366206

Committed r243632: <https://trac.webkit.org/changeset/243632>
Comment 26 WebKit Commit Bot 2019-03-28 18:16:07 PDT
All reviewed patches have been landed.  Closing bug.