Bug 212445 – Minimum user interaction time in ResourceLoadStatistics should handle the case of -1

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

Currently in the plist and database, -1 is used to indicate no user interaction. We should handle this in ResourceLoadStatistics when considering minimum user interaction time.

[Radar WebKit Bug Importer]

<rdar://problem/63696470>

[Kate Cheney]

Created attachment 400406 [details]
Patch

[Brent Fulgham]

Comment on attachment 400406 [details]
Patch

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

> Source/WebKit/ChangeLog:10
> +               http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction.html

Should we have a test for the case where cookies are removed, too?

This seems to be testing the 7-day non-cookie data removal, but what about the 30-day cookie removal?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2542
> +Optional<WallTime> ResourceLoadStatisticsDatabaseStore::mostRecentUserInteractionTime(DomainData& statistic)

Could this be passed as a const DomainData&?

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2572
> +        if (mostRecentUserInteractionTime)

I think this could just be:

if (auto mostRecentInteractionTime = this->mostRecentInteractionTime(statistic))

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)

How long has that been wrong! Yikes.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:971
> +Optional<WallTime> ResourceLoadStatisticsMemoryStore::mostRecentUserInteractionTime(ResourceLoadStatistics& statistic)

Seems like this could be "const ResourceLoadStatistics&"

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1021
> +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)

:-(

[John Wilander]

Comment on attachment 400406 [details]
Patch

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

Looks good! See comment on change log.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544
> +    if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0)

Good that you include 0 here.

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)

😳

> LayoutTests/ChangeLog:7
> +        Reviewed by NOBODY (OOPS!).

Please add a comment on that the case of data deletion are already tested and that these new tests only need to check for the absence of deletion. Also add a comment on which test case(s) started failing once you corrected the issue, if any.

[Kate Cheney]

(In reply to Brent Fulgham from comment #3)
> Comment on attachment 400406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400406&action=review
> 
> > Source/WebKit/ChangeLog:10
> > +               http/tests/resourceLoadStatistics/operating-dates-all-but-cookies-removed-statistics-with-no-user-interaction.html
> 
> Should we have a test for the case where cookies are removed, too?
> 
> This seems to be testing the 7-day non-cookie data removal, but what about
> the 30-day cookie removal?
> 

Since the flawed logic only affects non-cookie website data, the 30 day all website data removal case is covered by operating-dates-all-website-data-removed.html and operating-dates-all-website-data-removed-database.html. Good thought though, I can explain this in the ChangeLog.  

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2542
> > +Optional<WallTime> ResourceLoadStatisticsDatabaseStore::mostRecentUserInteractionTime(DomainData& statistic)
> 
> Could this be passed as a const DomainData&?
> 

Yes, good catch. I'll change that.

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2572
> > +        if (mostRecentUserInteractionTime)
> 
> I think this could just be:
> 
> if (auto mostRecentInteractionTime =
> this->mostRecentInteractionTime(statistic))
> 

Nice, will change. 

> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> > +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)
> 
> How long has that been wrong! Yikes.

:( 
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:971
> > +Optional<WallTime> ResourceLoadStatisticsMemoryStore::mostRecentUserInteractionTime(ResourceLoadStatistics& statistic)
> 
> Seems like this could be "const ResourceLoadStatistics&"

Yes, good catch. I'll change that.

> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsMemoryStore.cpp:1021
> > +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)
> 
> :-(

[Kate Cheney]

(In reply to John Wilander from comment #4)
> Comment on attachment 400406 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=400406&action=review
> 
> Looks good! See comment on change log.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544
> > +    if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0)
> 
> Good that you include 0 here.
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> > +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)
> 
> 😳
> 
> > LayoutTests/ChangeLog:7
> > +        Reviewed by NOBODY (OOPS!).
> 
> Please add a comment on that the case of data deletion are already tested
> and that these new tests only need to check for the absence of deletion.
> Also add a comment on which test case(s) started failing once you corrected
> the issue, if any.

Yes, doing this I realized these new tests can replace the old 7-day deletion tests because they cover that test case already. Patch-for-landing will remove the old ones.

[Kate Cheney]

Created attachment 400508 [details]
Patch for landing

[Kate Cheney]

Thanks for the feedback Brent and John!

[John Wilander]

(In reply to katherine_cheney from comment #6)
> (In reply to John Wilander from comment #4)
> > Comment on attachment 400406 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=400406&action=review
> > 
> > Looks good! See comment on change log.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544
> > > +    if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0)
> > 
> > Good that you include 0 here.
> > 
> > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> > > +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)
> > 
> > 😳
> > 
> > > LayoutTests/ChangeLog:7
> > > +        Reviewed by NOBODY (OOPS!).
> > 
> > Please add a comment on that the case of data deletion are already tested
> > and that these new tests only need to check for the absence of deletion.
> > Also add a comment on which test case(s) started failing once you corrected
> > the issue, if any.
> 
> Yes, doing this I realized these new tests can replace the old 7-day
> deletion tests because they cover that test case already. Patch-for-landing
> will remove the old ones.

That was not what I meant. I thought the old tests check the case when data *should* be deleted. But maybe there are *also* previous tests that data was retained?

[John Wilander]

(In reply to John Wilander from comment #9)
> (In reply to katherine_cheney from comment #6)
> > (In reply to John Wilander from comment #4)
> > > Comment on attachment 400406 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=400406&action=review
> > > 
> > > Looks good! See comment on change log.
> > > 
> > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544
> > > > +    if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0)
> > > 
> > > Good that you include 0 here.
> > > 
> > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> > > > +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)
> > > 
> > > 😳
> > > 
> > > > LayoutTests/ChangeLog:7
> > > > +        Reviewed by NOBODY (OOPS!).
> > > 
> > > Please add a comment on that the case of data deletion are already tested
> > > and that these new tests only need to check for the absence of deletion.
> > > Also add a comment on which test case(s) started failing once you corrected
> > > the issue, if any.
> > 
> > Yes, doing this I realized these new tests can replace the old 7-day
> > deletion tests because they cover that test case already. Patch-for-landing
> > will remove the old ones.
> 
> That was not what I meant. I thought the old tests check the case when data
> *should* be deleted. But maybe there are *also* previous tests that data was
> retained?

I think I see what you're saying. This is just about the non-cookie website data deletion.

[Kate Cheney]

(In reply to John Wilander from comment #10)
> (In reply to John Wilander from comment #9)
> > (In reply to katherine_cheney from comment #6)
> > > (In reply to John Wilander from comment #4)
> > > > Comment on attachment 400406 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=400406&action=review
> > > > 
> > > > Looks good! See comment on change log.
> > > > 
> > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544
> > > > > +    if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0)
> > > > 
> > > > Good that you include 0 here.
> > > > 
> > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> > > > > +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)
> > > > 
> > > > 😳
> > > > 
> > > > > LayoutTests/ChangeLog:7
> > > > > +        Reviewed by NOBODY (OOPS!).
> > > > 
> > > > Please add a comment on that the case of data deletion are already tested
> > > > and that these new tests only need to check for the absence of deletion.
> > > > Also add a comment on which test case(s) started failing once you corrected
> > > > the issue, if any.
> > > 
> > > Yes, doing this I realized these new tests can replace the old 7-day
> > > deletion tests because they cover that test case already. Patch-for-landing
> > > will remove the old ones.
> > 
> > That was not what I meant. I thought the old tests check the case when data
> > *should* be deleted. But maybe there are *also* previous tests that data was
> > retained?
> 
> I think I see what you're saying. This is just about the non-cookie website
> data deletion.

All tests check for proper data deletion for expired statistics. The new 7-day test checks for deletion when including a statistic with -1 as the mostRecentUserInteraction time and with parameters().isRunningTest disabled. This covered the other case, and it didn't make sense to keep the other 7-day data deletion test because it doesn't test actual behavior (because of the parameters().isRunningTest()).

[Kate Cheney]

(In reply to katherine_cheney from comment #11)
> (In reply to John Wilander from comment #10)
> > (In reply to John Wilander from comment #9)
> > > (In reply to katherine_cheney from comment #6)
> > > > (In reply to John Wilander from comment #4)
> > > > > Comment on attachment 400406 [details]
> > > > > Patch
> > > > > 
> > > > > View in context:
> > > > > https://bugs.webkit.org/attachment.cgi?id=400406&action=review
> > > > > 
> > > > > Looks good! See comment on change log.
> > > > > 
> > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2544
> > > > > > +    if (statistic.mostRecentUserInteractionTime.secondsSinceEpoch().value() <= 0)
> > > > > 
> > > > > Good that you include 0 here.
> > > > > 
> > > > > > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsDatabaseStore.cpp:2596
> > > > > > +    if (!parameters().isRunningTest && now - oldestUserInteraction < parameters().minimumTimeBetweenDataRecordsRemoval)
> > > > > 
> > > > > 😳
> > > > > 
> > > > > > LayoutTests/ChangeLog:7
> > > > > > +        Reviewed by NOBODY (OOPS!).
> > > > > 
> > > > > Please add a comment on that the case of data deletion are already tested
> > > > > and that these new tests only need to check for the absence of deletion.
> > > > > Also add a comment on which test case(s) started failing once you corrected
> > > > > the issue, if any.
> > > > 
> > > > Yes, doing this I realized these new tests can replace the old 7-day
> > > > deletion tests because they cover that test case already. Patch-for-landing
> > > > will remove the old ones.
> > > 
> > > That was not what I meant. I thought the old tests check the case when data
> > > *should* be deleted. But maybe there are *also* previous tests that data was
> > > retained?
> > 
> > I think I see what you're saying. This is just about the non-cookie website
> > data deletion.
> 
> All tests check for proper data deletion for expired statistics. The new
> 7-day test checks for deletion when including a statistic with -1 as the
> mostRecentUserInteraction time and with parameters().isRunningTest disabled.
> This covered the other case, and it didn't make sense to keep the other
> 7-day data deletion test because it doesn't test actual behavior (because of
> the parameters().isRunningTest()).

In fact, the other test was giving us a false-positive, which is probably worse.

[EWS]

Committed r262265: <https://trac.webkit.org/changeset/262265>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400508 [details].