Bug 198694 – Add mechanism and test case to check if ITP is active

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brent Fulgham]

We need a way to confirm that the NetworkProcess knows that ITP is on (or off) for testing (and perhaps other purposes).

This patch adds new infrastructure to support testing this status.

[Radar WebKit Bug Importer]

<rdar://problem/51557704>

[Brent Fulgham]

Created attachment 371703 [details]
Patch

[John Wilander]

Comment on attachment 371703 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:656
> +void NetworkProcess::resourceLoadStatisticsEnabled(PAL::SessionID sessionID, CompletionHandler<void(bool)>&& completionHandler)

At least for getters which return a boolean, we lead with an auxiliary verb. In this case, isResourceLoadStatisticsEnabled(). I don’t know if we have a rule for async boolean getters but I’d prefer a leading “is.”

Ditto for the rest of the functions with the same name.

> Source/WebKit/UIProcess/WebsiteData/WebsiteDataStore.cpp:1886
> +        processPool->ensureNetworkProcess().resourceLoadStatisticsEnabled(m_sessionID, WTFMove(completionHandler));

It’s a little weird to create a new network process only to check if ITP is on. We could also return that ITP is off if we don’t have a network process, but it’s probably best to do it the way you have to be able to catch race conditions where ITP was enabled before the network process existed.

[Maciej Stachowiak]

Comment on attachment 371703 [details]
Patch

It would be even better if this test could verify that ITP still does its thing when enabled this way, instead of just relying on a network process check. I realize this makes the test even more complicated.

[Sam Weinig]

Comment on attachment 371703 [details]
Patch

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:483
> +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler

I've always been confused by what "resourceLoadStatistics" statics mean in our codebase. It seems to be a synonym for "Intelligent Tracking Prevention", but it doesn't seem like that name is used much in WebKit (I only found it in WebPreferences.yaml). Before committing this to SPI, can try to find a name that encapsulates what this feature does in a clear way and standardize on it.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:72
> +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler;

Is this SPI you expect to be used outside of testing?

[Maciej Stachowiak]

I think this is indeed test$only SPI, so if there is a better way to do this, we should. That would make the name less important, but I tend to agree we should call it ITP in the code instead of the “resource load statistics” fig leaf.

[Sam Weinig]

(In reply to Maciej Stachowiak from comment #6)
> I think this is indeed test$only SPI, so if there is a better way to do
> this, we should. That would make the name less important, but I tend to
> agree we should call it ITP in the code instead of the “resource load
> statistics” fig leaf.

If this is really testing only, I think it would be much better to test that ITP is actually doing something (e.g. blocking / giving out the right cookies, etc). In which case, I don't think the new SPI is needed. Hopefully we have existing tests that of ITP functionality that could be used before and after enabling/disabling the feature.

[Brent Fulgham]

(In reply to Sam Weinig from comment #5)
> Comment on attachment 371703 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371703&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:72
> > +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler;
> 
> Is this SPI you expect to be used outside of testing?

No -- this is really an implementation detail that the API/SPI user shouldn't have to worry about. But we do want to test that things are staying in sync between the UIProcess and the NetworkPorcess.

Maybe this would be better named "isITPEnabledInNetworkProcess" or something?

[Sam Weinig]

(In reply to Brent Fulgham from comment #8)
> (In reply to Sam Weinig from comment #5)
> > Comment on attachment 371703 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=371703&action=review
> > 
> > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStorePrivate.h:72
> > > +- (void)_resourceLoadStatisticsEnabled:(void (^)(BOOL))completionHandler;
> > 
> > Is this SPI you expect to be used outside of testing?
> 
> No -- this is really an implementation detail that the API/SPI user
> shouldn't have to worry about. But we do want to test that things are
> staying in sync between the UIProcess and the NetworkPorcess.
> 
> Maybe this would be better named "isITPEnabledInNetworkProcess" or something?

Our usual strategy for this kind of thing is to ensure that side effects are working. e.g. if ITP is enabled in the NetworkProcess, I see X.

[Brent Fulgham]

Created attachment 371884 [details]
Updated patch.

[Brent Fulgham]

Created attachment 371886 [details]
patch v3

[WebKit Commit Bot]

Comment on attachment 371886 [details]
patch v3

Rejecting attachment 371886 [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', 'validate-changelog', '--check-oops', '--non-interactive', 371886, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Traceback (most recent call last):
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 84, in <module>
    main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch", line 79, in main
    WebKitPatch(os.path.abspath(__file__)).main()
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 305, in main
    result = command.check_arguments_and_execute(options, args, self)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/multicommandtool.py", line 123, in check_arguments_and_execute
    return self.execute(options, args, tool) or 0
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/abstractsequencedcommand.py", line 55, in execute
    self._sequence.run_and_handle_errors(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 73, in run_and_handle_errors
    self._run(tool, options, state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/commands/stepsequence.py", line 67, in _run
    step(tool, options).run(state)
  File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/tool/steps/validatereviewer.py", line 54, in run
    if changelog_entry.has_valid_reviewer():
AttributeError: 'NoneType' object has no attribute 'has_valid_reviewer'

Full output: https://webkit-queues.webkit.org/results/12455955

[Brent Fulgham]

Committed r246360: <https://trac.webkit.org/changeset/246360/webkit>

[Sam Weinig]

Comment on attachment 371886 [details]
patch v3

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

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:491
> +#if ENABLE(RESOURCE_LOAD_STATISTICS)
> +    _websiteDataStore->websiteDataStore().setPrevalentResource(URL(domain), [completionHandler = makeBlockPtr(completionHandler)]() {
> +        completionHandler();
> +    });
> +#endif
> +    completionHandler();
> +}

This calls the completionHandler handler twice. Once right away, once when the async function's completion handler is called.

[Brent Fulgham]

Unreviewed fix landed in r246370: <https://trac.webkit.org/changeset/246370/webkit>

[Brent Fulgham]

(In reply to Sam Weinig from comment #14)
> Comment on attachment 371886 [details]
> patch v3
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=371886&action=review
> 
> > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:491
> > +#if ENABLE(RESOURCE_LOAD_STATISTICS)
> > +    _websiteDataStore->websiteDataStore().setPrevalentResource(URL(domain), [completionHandler = makeBlockPtr(completionHandler)]() {
> > +        completionHandler();
> > +    });
> > +#endif
> > +    completionHandler();
> > +}
> 
> This calls the completionHandler handler twice. Once right away, once when
> the async function's completion handler is called.

Doh! !@$#$#!@#