Bug 206942 – ITP statistics processing should be off by default for tests

• Status: NEW
• Component: WebKit Misc.

[Kate Cheney]

Statistics processing removes website data and causes flaky test failures. It should be off by default in testing and turned on by ITP tests when needed.

[Radar WebKit Bug Importer]

<rdar://problem/58994759>

[Kate Cheney]

Created attachment 389164 [details]
Patch

[youenn fablet]

Comment on attachment 389164 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:239
> +        return;

Another approach would be to make sure shouldRemoveDataRecords return false inside removeDataRecords.
This would make sure we do more of the usual processing.

Looking at shouldRemoveDataRecords, it seems that we should in most cases return false from "!m_lastTimeDataRecordsWereRemoved || MonotonicTime::now() >= (m_lastTimeDataRecordsWereRemoved + m_parameters.minimumTimeBetweenDataRecordsRemoval) || parameters().isRunningTest;":
- WTR is deleting all WebsiteDataStore at the beginning of a test.
- We could thus consider that (or make sure that) ResourceLoadStatisticsStore::m_lastTimeDataRecordsWereRemoved is set to current time.
- m_parameters.minimumTimeBetweenDataRecordsRemoval is by default 1 hour which should be long enough for the test (and maybe the test suite) to complete.
- parameters().isRunningTest should return false except for ITP tests.

Looking at TestController, I do not see where we call WKWebsiteDataStoreSetStatisticsIsRunningTest to set it back to false between each run.
Could it be the cause for flakiness? Should we ensure that it is set false in TestController::resetStateToConsistentValues?

> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:316
> +    void setDisableITPStatisticsProcessingForTesting(boolean value);

I would rename it to setEnableITPStatisticsProcessing and remove ForTesting since it is test runner.

> LayoutTests/ChangeLog:16
> +        SetEnableFeature(true).

We probably do not need/should not need to go back to a consistent state from the JS tes.
WebKitTestRunner should be doing that for us in TestController::resetStateToConsistentValues.

> LayoutTests/http/tests/resourceLoadStatistics/prevalent-resource-without-user-interaction.html:20
> +        setEnableFeature(false, function() {

This is easy to forget a call to testRunner.statisticsResetToConsistentState/setEnableFeature(false).
We should make sure this is not needed and WTR does it for us.

In practice, it seems that TestController calls WKWebsiteDataStoreStatisticsResetToConsistentState from TestController::resetStateToConsistentValues.
Maybe we just need to set isRunningTest to false in WKWebsiteDataStoreStatisticsResetToConsistentState.

> LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:8
> +        testRunner.setDisableITPStatisticsProcessingForTesting(false);

Do we still need internals.setResourceLoadStatisticsEnabled(true)?
It seems enabled by default by TestController.

[Kate Cheney]

Comment on attachment 389164 [details]
Patch

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

In response to two of the comments, think we could move the setEnableFeature(false) functionality to TestController::resetStateToConsistentValues, but would it be better in a separate patch, or does it make sense to include it here?

>> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:239
>> +        return;
> 
> Another approach would be to make sure shouldRemoveDataRecords return false inside removeDataRecords.
> This would make sure we do more of the usual processing.
> 
> Looking at shouldRemoveDataRecords, it seems that we should in most cases return false from "!m_lastTimeDataRecordsWereRemoved || MonotonicTime::now() >= (m_lastTimeDataRecordsWereRemoved + m_parameters.minimumTimeBetweenDataRecordsRemoval) || parameters().isRunningTest;":
> - WTR is deleting all WebsiteDataStore at the beginning of a test.
> - We could thus consider that (or make sure that) ResourceLoadStatisticsStore::m_lastTimeDataRecordsWereRemoved is set to current time.
> - m_parameters.minimumTimeBetweenDataRecordsRemoval is by default 1 hour which should be long enough for the test (and maybe the test suite) to complete.
> - parameters().isRunningTest should return false except for ITP tests.
> 
> Looking at TestController, I do not see where we call WKWebsiteDataStoreSetStatisticsIsRunningTest to set it back to false between each run.
> Could it be the cause for flakiness? Should we ensure that it is set false in TestController::resetStateToConsistentValues?

I see now that shouldRemoveDataRecords is a more logical place for this check, and that ITP tests (since they always set isRunningTest) will always removeDataRecords. So your suggestion is that we always set m_lastTimeDataRecordsWereRemoved to the current time, maybe in TestController::resetStateToConsistentValues?

testRunner.setStatisticsIsRunningTest(false) is called in http/tests/resourceLoadStatistics/resources/util.js setEnableFeature(false), which is called by ITP tests, so probably not a source of flakiness.

>> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:316
>> +    void setDisableITPStatisticsProcessingForTesting(boolean value);
> 
> I would rename it to setEnableITPStatisticsProcessing and remove ForTesting since it is test runner.

Sounds good. We might not even need this call if we just adjust the m_lastTimeDataRecordsWereRemoved parameter.

>> LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:8
>> +        testRunner.setDisableITPStatisticsProcessingForTesting(false);
> 
> Do we still need internals.setResourceLoadStatisticsEnabled(true)?
> It seems enabled by default by TestController.

This is an interesting observation, I think it's correct and this call can be deleted. I wonder why it was included here originally.

[youenn fablet]

(In reply to katherine_cheney from comment #4)
> Comment on attachment 389164 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389164&action=review
> 
> In response to two of the comments, think we could move the
> setEnableFeature(false) functionality to
> TestController::resetStateToConsistentValues, but would it be better in a
> separate patch, or does it make sense to include it here?

Either way is fine.

> >> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:239
> >> +        return;
> > 
> > Another approach would be to make sure shouldRemoveDataRecords return false inside removeDataRecords.
> > This would make sure we do more of the usual processing.
> > 
> > Looking at shouldRemoveDataRecords, it seems that we should in most cases return false from "!m_lastTimeDataRecordsWereRemoved || MonotonicTime::now() >= (m_lastTimeDataRecordsWereRemoved + m_parameters.minimumTimeBetweenDataRecordsRemoval) || parameters().isRunningTest;":
> > - WTR is deleting all WebsiteDataStore at the beginning of a test.
> > - We could thus consider that (or make sure that) ResourceLoadStatisticsStore::m_lastTimeDataRecordsWereRemoved is set to current time.
> > - m_parameters.minimumTimeBetweenDataRecordsRemoval is by default 1 hour which should be long enough for the test (and maybe the test suite) to complete.
> > - parameters().isRunningTest should return false except for ITP tests.
> > 
> > Looking at TestController, I do not see where we call WKWebsiteDataStoreSetStatisticsIsRunningTest to set it back to false between each run.
> > Could it be the cause for flakiness? Should we ensure that it is set false in TestController::resetStateToConsistentValues?
> 
> I see now that shouldRemoveDataRecords is a more logical place for this
> check, and that ITP tests (since they always set isRunningTest) will always
> removeDataRecords. So your suggestion is that we always set
> m_lastTimeDataRecordsWereRemoved to the current time, maybe in
> TestController::resetStateToConsistentValues?

That would be the best approach yes.
We also need to make sure that the timeout value is back to 1 hour. and isITPTesting is false.


> testRunner.setStatisticsIsRunningTest(false) is called in
> http/tests/resourceLoadStatistics/resources/util.js setEnableFeature(false),
> which is called by ITP tests, so probably not a source of flakiness.
> 
> >> Tools/WebKitTestRunner/InjectedBundle/Bindings/TestRunner.idl:316
> >> +    void setDisableITPStatisticsProcessingForTesting(boolean value);
> > 
> > I would rename it to setEnableITPStatisticsProcessing and remove ForTesting since it is test runner.
> 
> Sounds good. We might not even need this call if we just adjust the
> m_lastTimeDataRecordsWereRemoved parameter.
> 
> >> LayoutTests/http/tests/resourceLoadStatistics/resources/util.js:8
> >> +        testRunner.setDisableITPStatisticsProcessingForTesting(false);
> > 
> > Do we still need internals.setResourceLoadStatisticsEnabled(true)?
> > It seems enabled by default by TestController.
> 
> This is an interesting observation, I think it's correct and this call can
> be deleted. I wonder why it was included here originally.

[Alexey Proskuryakov]

Comment on attachment 389164 [details]
Patch

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

> LayoutTests/ChangeLog:10
> +        All ITP Tests that rely on processing should start with
> +        âSetEnableFeature(trueâ¦.â to enable the ITPStatisticsProcessing flag,

As someone who didn't look at ITP tests before, I found it very confusing to see a function named "setEnableFeature". Is there something that makes it clear what "feature" this is talking about? Wondering if this function should be renamed for clarity.

[Kate Cheney]

(In reply to Alexey Proskuryakov from comment #6)
> Comment on attachment 389164 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389164&action=review
> 
> > LayoutTests/ChangeLog:10
> > +        All ITP Tests that rely on processing should start with
> > +        âSetEnableFeature(trueâ¦.â to enable the ITPStatisticsProcessing flag,
> 
> As someone who didn't look at ITP tests before, I found it very confusing to
> see a function named "setEnableFeature". Is there something that makes it
> clear what "feature" this is talking about? Wondering if this function
> should be renamed for clarity.

I was just thinking the same thing actually. Since we are getting rid of the need for a boolean input, we can name it something like "enableStatisticsTestSettings"