Bug 213795 – [ iOS Debug and Mojave Debug ] http/tests/storage/storage-map-leaking.html is flaky timing out.

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Jason Lawrence]

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
-

[Jason Lawrence]

I have marked this test as flaky timing out while this issue is investigated.
https://trac.webkit.org/changeset/263747/webkit

[Radar WebKit Bug Importer]

<rdar://problem/64937993>

[Chris Dumez]

Created attachment 403311 [details]
Patch

[Alex Christensen]

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

[Chris Dumez]

(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

[Chris Dumez]

Created attachment 403312 [details]
Patch

[Geoffrey Garen]

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

[Darin Adler]

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.

[Chris Dumez]

Created attachment 403318 [details]
Patch

[Chris Dumez]

(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.

[Darin Adler]

(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.

[Darin Adler]

(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.

[EWS]

Committed r263822: <https://trac.webkit.org/changeset/263822>

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