Bug 83618 - fast/dom/inline-event-attributes-release.html randomly fails
Summary: fast/dom/inline-event-attributes-release.html randomly fails
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Fujii Hironori
URL:
Keywords: InRadar, MakingBotsRed
Depends on:
Blocks:
 
Reported: 2012-04-10 14:28 PDT by Anders Carlsson
Modified: 2022-07-05 23:41 PDT (History)
3 users (show)

See Also:


Attachments
Patch (4.57 KB, patch)
2022-07-05 14:42 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (4.56 KB, patch)
2022-07-05 17:16 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch (4.59 KB, patch)
2022-07-05 19:22 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff
Patch for landing (4.59 KB, patch)
2022-07-05 20:56 PDT, Fujii Hironori
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2012-04-10 14:28:30 PDT
fast/dom/inline-event-attributes-release.html is flakey on Mac and needs to be added to the Skipped list.
Comment 1 Fujii Hironori 2022-07-03 18:20:59 PDT
Another test failure:

Buildbot: builder Apple-BigSur-Release-WK1-Tests build 11350: 251845@main
https://build.webkit.org/#/builders/48/builds/11350

--- /Volumes/Data/worker/Apple-BigSur-Release-WK1-Tests/build/layout-test-results/fast/dom/inline-event-attributes-release-expected.txt
+++ /Volumes/Data/worker/Apple-BigSur-Release-WK1-Tests/build/layout-test-results/fast/dom/inline-event-attributes-release-actual.txt
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS afterCount - beforeCount is 0
+FAIL afterCount - beforeCount should be 0. Was 1.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 2 Fujii Hironori 2022-07-03 18:25:48 PDT
Another test failure:

Buildbot: builder Apple-Monterey-Release-WK2-Tests build 4424
251329@main
https://build.webkit.org/#/builders/365/builds/4424

--- /Volumes/Data/worker/Apple-Monterey-Release-WK2-Tests/build/layout-test-results/fast/dom/inline-event-attributes-release-expected.txt
+++ /Volumes/Data/worker/Apple-Monterey-Release-WK2-Tests/build/layout-test-results/fast/dom/inline-event-attributes-release-actual.txt
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS afterCount - beforeCount is 0
+FAIL afterCount - beforeCount should be 0. Was 1.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 3 Fujii Hironori 2022-07-05 14:42:39 PDT
Created attachment 460694 [details]
Patch
Comment 4 Fujii Hironori 2022-07-05 14:52:12 PDT
Another test failure:

Buildbot: builder Apple-BigSur-Release-WK1-Tests build 10954: 251198@main
https://build.webkit.org/#/builders/48/builds/10954

--- /Volumes/Data/worker/Apple-BigSur-Release-WK1-Tests/build/layout-test-results/fast/dom/inline-event-attributes-release-expected.txt
+++ /Volumes/Data/worker/Apple-BigSur-Release-WK1-Tests/build/layout-test-results/fast/dom/inline-event-attributes-release-actual.txt
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 
-PASS afterCount - beforeCount is 0
+FAIL afterCount - beforeCount should be 0. Was -110.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 5 Ryosuke Niwa 2022-07-05 14:52:42 PDT
Comment on attachment 460694 [details]
Patch

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

> LayoutTests/fast/dom/inline-event-attributes-release.html:27
> +if (delta < iterations / 10)

This is a very high factor. I suggest we use 2 or 3 instead.
Comment 6 Fujii Hironori 2022-07-05 17:09:03 PDT
Comment on attachment 460694 [details]
Patch

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

>> LayoutTests/fast/dom/inline-event-attributes-release.html:27
>> +if (delta < iterations / 10)
> 
> This is a very high factor. I suggest we use 2 or 3 instead.

I retrieved the number from checkForNodeLeaks.
https://github.com/WebKit/WebKit/blob/e36b05647cc7b60f59a9c96872d4f562c9514e7c/LayoutTests/fast/dom/reference-cycle-leaks.html#L21

And, testDocumentIsNotLeaked passes if at least a single document is reclaimed.
https://github.com/WebKit/WebKit/blob/e36b05647cc7b60f59a9c96872d4f562c9514e7c/LayoutTests/resources/gc.js#L49

However, I agree with you. Will fix this test.
Comment 7 Fujii Hironori 2022-07-05 17:16:59 PDT
Created attachment 460696 [details]
Patch
Comment 8 Ryosuke Niwa 2022-07-05 18:08:07 PDT
Comment on attachment 460696 [details]
Patch

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

> LayoutTests/fast/dom/inline-event-attributes-release.html:27
> +if (delta < 3)

Oh, no, I meant to say divide iterations by 3, not literal 3.
Requiring at least one document getting collected is okay too.
Delta to be less than 10 is too aggressive of a policy.
Comment 9 Fujii Hironori 2022-07-05 19:05:02 PDT
Comment on attachment 460696 [details]
Patch

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

>> LayoutTests/fast/dom/inline-event-attributes-release.html:27
>> +if (delta < 3)
> 
> Oh, no, I meant to say divide iterations by 3, not literal 3.
> Requiring at least one document getting collected is okay too.
> Delta to be less than 10 is too aggressive of a policy.

Oh. Will fix.
Comment 10 Fujii Hironori 2022-07-05 19:22:51 PDT
Created attachment 460701 [details]
Patch
Comment 11 Ryosuke Niwa 2022-07-05 20:30:32 PDT
Comment on attachment 460701 [details]
Patch

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

> LayoutTests/fast/dom/inline-event-attributes-release.html:26
> +if (delta < iterations / 3)

iterations / 2 might be better and this might be still flaky but we can try.
Comment 12 Fujii Hironori 2022-07-05 20:37:32 PDT
Comment on attachment 460701 [details]
Patch

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

>> LayoutTests/fast/dom/inline-event-attributes-release.html:26
>> +if (delta < iterations / 3)
> 
> iterations / 2 might be better and this might be still flaky but we can try.

Why do you think it might be still flaky?
fast/dom/reference-cycle-leaks.html is using the condition "if (leaks < repetitions / 10)", but not flaky.
Comment 13 Ryosuke Niwa 2022-07-05 20:40:04 PDT
(In reply to Fujii Hironori from comment #12)
> Comment on attachment 460701 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=460701&action=review
> 
> >> LayoutTests/fast/dom/inline-event-attributes-release.html:26
> >> +if (delta < iterations / 3)
> > 
> > iterations / 2 might be better and this might be still flaky but we can try.
> 
> Why do you think it might be still flaky?
> fast/dom/reference-cycle-leaks.html is using the condition "if (leaks <
> repetitions / 10)", but not flaky.

Because it's conservative GC. The assertion that more than half the nodes get collected is only probabilistically true.
Comment 14 Fujii Hironori 2022-07-05 20:56:33 PDT
Created attachment 460705 [details]
Patch for landing
Comment 15 Fujii Hironori 2022-07-05 21:00:23 PDT
I don't think the conservative GC can make so much false-positive in this test case.
However, it would happen in some cases, for example, if all created element objects were pushed into an array, and the array wasn't zero-filled after used.
Comment 16 EWS 2022-07-05 23:40:51 PDT
Committed 252173@main (45cf55da6e5b): <https://commits.webkit.org/252173@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 460705 [details].
Comment 17 Radar WebKit Bug Importer 2022-07-05 23:41:16 PDT
<rdar://problem/96496965>