Bug 207271 - REGRESSION (r252014?): [ Mac wk2 ] http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html is a flaky failure
Summary: REGRESSION (r252014?): [ Mac wk2 ] http/tests/resourceLoadStatistics/log-cros...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: katherine_cheney
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-02-05 09:14 PST by Jason Lawrence
Modified: 2020-02-09 13:37 PST (History)
9 users (show)

See Also:


Attachments
Update Test Expectations (1.51 KB, patch)
2020-02-05 09:17 PST, Jason Lawrence
no flags Details | Formatted Diff | Diff
Patch (1.81 KB, patch)
2020-02-06 14:38 PST, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (48.78 KB, patch)
2020-02-06 16:51 PST, katherine_cheney
no flags Details | Formatted Diff | Diff
Patch (102.79 KB, patch)
2020-02-07 09:02 PST, katherine_cheney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jason Lawrence 2020-02-05 09:14:02 PST
http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html

Description:
This test is flaky failing on Mac wk2 with a possible regression on r252014.

History:
https://results.webkit.org/?platform=ios&platform=mac&limit=50000&suite=layout-tests&test=http%2Ftests%2FresourceLoadStatistics%2Flog-cross-site-load-with-link-decoration.html

Diff:
--- /Volumes/Data/slave/catalina-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration-expected.txt
+++ /Volumes/Data/slave/catalina-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration-actual.txt
@@ -10,7 +10,7 @@
         127.0.0.1
     topFrameLinkDecorationsFrom:
         127.0.0.1
-    gotLinkDecorationFromPrevalentResource: Yes
+    gotLinkDecorationFromPrevalentResource: No
     isPrevalentResource: No
     isVeryPrevalentResource: No
     dataRecordsRemoved: 0
Comment 1 Jason Lawrence 2020-02-05 09:17:32 PST
Created attachment 389817 [details]
Update Test Expectations
Comment 2 Radar WebKit Bug Importer 2020-02-05 09:19:16 PST
<rdar://problem/59190346>
Comment 3 Truitt Savell 2020-02-05 09:23:03 PST
Comment on attachment 389817 [details]
Update Test Expectations

Clearing flags on attachment: 389817

Committed r255820: <https://trac.webkit.org/changeset/255820>
Comment 4 katherine_cheney 2020-02-06 14:38:07 PST
Created attachment 390000 [details]
Patch
Comment 5 John Wilander 2020-02-06 15:05:13 PST
Comment on attachment 390000 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:238
> +    if (parameters().isRunningTest && !m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned)

This will make us rely on all relevant tests that set isRunningTest to also set shouldNotifyPagesWhenDataRecordsWereScanned. Do they today? An easy way to check could be to grep for both and diff the list. For cases where only one is set, we'd have to audit if that's correct. (Ideally, test results on the bots should give us the answer but …)
Comment 6 katherine_cheney 2020-02-06 15:41:20 PST
(In reply to John Wilander from comment #5)
> Comment on attachment 390000 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390000&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:238
> > +    if (parameters().isRunningTest && !m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned)
> 
> This will make us rely on all relevant tests that set isRunningTest to also
> set shouldNotifyPagesWhenDataRecordsWereScanned. Do they today? An easy way
> to check could be to grep for both and diff the list. For cases where only
> one is set, we'd have to audit if that's correct. (Ideally, test results on
> the bots should give us the answer but …)

Good catch! About to upload a new patch with isRunningTest added to a few stray tests.
Comment 7 katherine_cheney 2020-02-06 16:51:20 PST
Created attachment 390028 [details]
Patch
Comment 8 John Wilander 2020-02-06 16:56:28 PST
Comment on attachment 390028 [details]
Patch

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

> Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:238
> +    if (parameters().isRunningTest && !m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned)

There are the two TestRunner functions installStatisticsDidModifyDataRecordsCallback() and installStatisticsDidScanDataRecordsCallback(). Will they still work with this change? I'm thinking the *Modify* version might get neutered. But I'm not sure we're still using both.

> LayoutTests/ChangeLog:10
> +        setEnableFeature(true) at test setup and setEnableFeature(false) when

Excellent.
Comment 9 Alexey Proskuryakov 2020-02-06 17:16:27 PST
Comment on attachment 390028 [details]
Patch

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

>> LayoutTests/ChangeLog:10
>> +        setEnableFeature(true) at test setup and setEnableFeature(false) when
> 
> Excellent.

Does this mean that later tests can misbehave if an exception is raised in a prior one, resulting in no setEnableFeature(false) call? Can this be guarded against in Internals?
Comment 10 katherine_cheney 2020-02-07 08:34:31 PST
(In reply to John Wilander from comment #8)
> Comment on attachment 390028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390028&action=review
> 
> > Source/WebKit/NetworkProcess/Classifier/ResourceLoadStatisticsStore.cpp:238
> > +    if (parameters().isRunningTest && !m_parameters.shouldNotifyPagesWhenDataRecordsWereScanned)
> 
> There are the two TestRunner functions
> installStatisticsDidModifyDataRecordsCallback() and
> installStatisticsDidScanDataRecordsCallback(). Will they still work with
> this change? I'm thinking the *Modify* version might get neutered. But I'm
> not sure we're still using both.
> 

It looks like installStatisticsDidModifyDataRecordsCallback is the callback for when website data is deleted, and installStatisticsDidScanDataRecordsCallback is for when website data is just scanned. So when we expect website data to be deleted, we should use installStatisticsDidModifyDataRecordsCallback, and manually call setStatisticsNotifyPagesWhenDataRecordsWereScanned.

This also means we can delete any calls to setStatisticsNotifyPagesWhenDataRecordsWereScanned when installStatisticsDidScanDataRecordsCallback is called, because the function does it already!


> > LayoutTests/ChangeLog:10
> > +        setEnableFeature(true) at test setup and setEnableFeature(false) when
> 
> Excellent.
Comment 11 katherine_cheney 2020-02-07 08:45:32 PST
(In reply to Alexey Proskuryakov from comment #9)
> Comment on attachment 390028 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390028&action=review
> 
> >> LayoutTests/ChangeLog:10
> >> +        setEnableFeature(true) at test setup and setEnableFeature(false) when
> > 
> > Excellent.
> 
> Does this mean that later tests can misbehave if an exception is raised in a
> prior one, resulting in no setEnableFeature(false) call? Can this be guarded
> against in Internals?

I definitely think some of the logic in setEnableFeature could be moved to TestController, but I think it's best to do it in another patch, because it would involve changing 150+ files and isn't directly related to this flaky test.
Comment 12 katherine_cheney 2020-02-07 09:02:45 PST
Created attachment 390087 [details]
Patch
Comment 13 Maciej Stachowiak 2020-02-07 19:01:14 PST
Comment on attachment 390087 [details]
Patch

r=me
Comment 14 John Wilander 2020-02-07 20:44:06 PST
Thanks for r+:ing, Maciej. I was going through my earlier comments and checking things and then got distracted. FWIW, looks good to me too.
Comment 15 John Wilander 2020-02-07 20:45:14 PST
Hmm, may be one test case that needs massaging. See the iOS bot.
Comment 16 katherine_cheney 2020-02-09 12:39:15 PST
(In reply to John Wilander from comment #15)
> Hmm, may be one test case that needs massaging. See the iOS bot.

Checked it out and that one was broken (and fixed) in a separate patch. See https://bugs.webkit.org/show_bug.cgi?id=207382
Comment 17 WebKit Commit Bot 2020-02-09 13:36:14 PST
The commit-queue encountered the following flaky tests while processing attachment 390087 [details]:

fetch/fetch-worker-crash.html bug 187257 (author: youennf@gmail.com)
The commit-queue is continuing to process your patch.
Comment 18 WebKit Commit Bot 2020-02-09 13:36:59 PST
Comment on attachment 390087 [details]
Patch

Clearing flags on attachment: 390087

Committed r256104: <https://trac.webkit.org/changeset/256104>
Comment 19 WebKit Commit Bot 2020-02-09 13:37:01 PST
All reviewed patches have been landed.  Closing bug.