http/tests/storage/storage-map-leaking.html Description: This test is flaky timing out on iOS Debug and Mojave Debug. The flaky timeouts initially appeared in the visible history on 06/29/2020. History: https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fstorage%2Fstorage-map-leaking.html&style=debug&platform=ios&platform=mac&version_name=iOS%2013&version_name=Mojave&limit=50000 Diff: --- /Volumes/Data/slave/ios-simulator-13-debug-tests-wk2/build/layout-test-results/http/tests/storage/storage-map-leaking-expected.txt +++ /Volumes/Data/slave/ios-simulator-13-debug-tests-wk2/build/layout-test-results/http/tests/storage/storage-map-leaking-actual.txt @@ -1,10 +1,3 @@ -Make sure that StorageAreaMap objects do no leak. +#PID UNRESPONSIVE - WebKitTestRunnerApp (pid 50459) +FAIL: Timed out waiting for notifyDone to be called -On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE". - - -PASS StorageAreaMap objects are not leaking -PASS successfullyParsed is true - -TEST COMPLETE -
I have marked this test as flaky timing out while this issue is investigated. https://trac.webkit.org/changeset/263747/webkit
<rdar://problem/64937993>
Created attachment 403311 [details] Patch
Comment on attachment 403311 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=403311&action=review > LayoutTests/ChangeLog:9 > + Because our GC is conservative, a particular JS wrapper is not guaranteed to go away in a timely fashion. Couldn't we instead try gc multiple times, or make a way to completely gc, or gc until it succeeds? > LayoutTests/ChangeLog:11 > + We treat the test as passive if any of the 4 storage wrappers go away after removing the 4 subframes from passive -> passing
(In reply to Alex Christensen from comment #4) > Comment on attachment 403311 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=403311&action=review > > > LayoutTests/ChangeLog:9 > > + Because our GC is conservative, a particular JS wrapper is not guaranteed to go away in a timely fashion. > > Couldn't we instead try gc multiple times, or make a way to completely gc, > or gc until it succeeds? The test already called gc() repeatedly in a setInterval(). From what I have been told, this is still not sufficient but Geoff / Yusuke know these things better. > > > LayoutTests/ChangeLog:11 > > + We treat the test as passive if any of the 4 storage wrappers go away after removing the 4 subframes from > > passive -> passing
Created attachment 403312 [details] Patch
> > Couldn't we instead try gc multiple times, or make a way to completely gc, > > or gc until it succeeds? > > The test already called gc() repeatedly in a setInterval(). From what I have > been told, this is still not sufficient but Geoff / Yusuke know these things > better. Trying multiple objects is indeed the state of the art in WebKit GC testing. The good news is, it works -- even though it's a bit ugly. In addition to working around conservative GC, trying multiple objects also works around other implementation details like single element caches. If a single object sticks around for a bit, one could reasonably wonder if that's really a leak. If hundreds stick around, that's obviously a leak.
Comment on attachment 403312 [details] Patch Seems worth a try. Also seems worth a comment about why there need to be 4 and why these 4 hostnames will work.
Created attachment 403318 [details] Patch
(In reply to Darin Adler from comment #8) > Comment on attachment 403312 [details] > Patch > > Seems worth a try. Also seems worth a comment about why there need to be 4 > and why these 4 hostnames will work. Layout tests only support those 4 origins AFAIK. The objects we are worried about leaking are the StorageAreaMap and there is one of these per origin. Using more frames won't be useful because even though we'll get more Storage JS wrappers, they will share the same underlying StorageMap objects.
(In reply to Chris Dumez from comment #10) > Layout tests only support those 4 origins AFAIK. The objects we are worried > about leaking are the StorageAreaMap and there is one of these per origin. > Using more frames won't be useful because even though we'll get more Storage > JS wrappers, they will share the same underlying StorageMap objects. Yes, I meant leaving a comment behind in the test so people understand it.
(In reply to Darin Adler from comment #11) > Yes, I meant leaving a comment behind in the test so people understand it. Which you did.
Committed r263822: <https://trac.webkit.org/changeset/263822> All reviewed patches have been landed. Closing bug and clearing flags on attachment 403318 [details].