207271 – REGRESSION (r252014?): [ Mac wk2 ] http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html is a flaky failure

RESOLVED FIXED 207271

REGRESSION (r252014?): [ Mac wk2 ] http/tests/resourceLoadStatistics/log-cross-site-load-with-link-decoration.html is a flaky failure

https://bugs.webkit.org/show_bug.cgi?id=207271

Summary REGRESSION (r252014?): [ Mac wk2 ] http/tests/resourceLoadStatistics/log-cros...

Jason Lawrence

Reported 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

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, Kate Cheney	no flags	Details Formatted Diff Diff
Patch (48.78 KB, patch) 2020-02-06 16:51 PST, Kate Cheney	no flags	Details Formatted Diff Diff
Patch (102.79 KB, patch) 2020-02-07 09:02 PST, Kate Cheney	no flags	Details Formatted Diff Diff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.

Jason Lawrence

Comment 1 2020-02-05 09:17:32 PST

Created attachment 389817 [details] Update Test Expectations

Radar WebKit Bug Importer

Comment 2 2020-02-05 09:19:16 PST

<rdar://problem/59190346>

Truitt Savell

Comment 3 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>

Kate Cheney

Comment 4 2020-02-06 14:38:07 PST

Created attachment 390000 [details] Patch

John Wilander

Comment 5 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 …)

Kate Cheney

Comment 6 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.

Kate Cheney

Comment 7 2020-02-06 16:51:20 PST

Created attachment 390028 [details] Patch

John Wilander

Comment 8 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.

Alexey Proskuryakov

Comment 9 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?

Kate Cheney

Comment 10 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.

Kate Cheney

Comment 11 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.

Kate Cheney

Comment 12 2020-02-07 09:02:45 PST

Created attachment 390087 [details] Patch

Maciej Stachowiak

Comment 13 2020-02-07 19:01:14 PST

Comment on attachment 390087 [details] Patch r=me

John Wilander

Comment 14 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.

John Wilander

Comment 15 2020-02-07 20:45:14 PST

Hmm, may be one test case that needs massaging. See the iOS bot.

Kate Cheney

Comment 16 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

WebKit Commit Bot

Comment 17 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.

WebKit Commit Bot

Comment 18 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>

WebKit Commit Bot

Comment 19 2020-02-09 13:37:01 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

Kate Cheney

Reported

2020-02-05 09:14 PST

Modified

2020-02-09 13:37 PST History

CC List

9 users Show

URL

Keywords InRadar

Depends on

Blocks