Bug 211595 – Code pattern in GC tests in LayoutTests is broken

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Yusuke Suzuki]

A lot of GC tests in LayoutTests are broken, they are not assuming that JavaScriptCore is using conservative GC.

[Radar WebKit Bug Importer]

<rdar://problem/62993121>

[Yusuke Suzuki]

These tests are doing,

1. Allocate one object
2. Invoking GC repeatedly until we observe that (1)'s object is collected

This is not the right approach. If we allocate only one object without using loop, it is very possible that this one object's address exists in some of CPU register in some case, and it is kept alive.

The right approach is,

1. Allocating many objects with loop. This ensures that stack / register can only include some of them. Not all of them. If the objects are allocated in a loop, then it is super possible that next iteration can overwrite the same register / stack location with new object, and old object is not in the conservative root (that's why we recommend using loop for GC test).
2. Invoking GC repeatedly, and check one of (1) is at least collected.

[Yusuke Suzuki]

For example, LayoutTests/resize-observer/element-leak.html is great.

[Yusuke Suzuki]

Created attachment 398820 [details]
Patch

[Saam Barati]

Comment on attachment 398820 [details]
Patch

r=me

[Keith Miller]

Comment on attachment 398820 [details]
Patch

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

r=me with some comments.

> LayoutTests/ChangeLog:19
> +        This is not being a problem in practice. Web pages are executing various kind of code and they clobber conservative roots. However, tests in

Typo: This is not being a => This is not a

> LayoutTests/ChangeLog:30
> +            3. Repeatedly invoke GC and test whether *one of (1) iframes* gets collected at least. Theoretically this is still possible that completely
> +               unrelated integer value in conservative roots can point to the reference of (1). By allocating many iframes in (1) and testing one of them,
> +               we can reduce the possibility of this conflict.

I have a minor quip with this approach. I think you should only have to invoke GC once. If the first GC fails I doubt any subsequent ones will do any better. Ideally, it would also be an async GC so as much of the stack can be cleared as possible. Maybe we should provide such an API to WebCore? It could be as simple as `setTimeout(syncGC, 0)`

> LayoutTests/http/tests/resources/gc.js:53
> +        await waitFor(50);

Why do we need the await? isn't gc() sync?

[Yusuke Suzuki]

Comment on attachment 398820 [details]
Patch

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

>> LayoutTests/ChangeLog:19
>> +        This is not being a problem in practice. Web pages are executing various kind of code and they clobber conservative roots. However, tests in
> 
> Typo: This is not being a => This is not a

Fixed.

>> LayoutTests/ChangeLog:30
>> +               we can reduce the possibility of this conflict.
> 
> I have a minor quip with this approach. I think you should only have to invoke GC once. If the first GC fails I doubt any subsequent ones will do any better. Ideally, it would also be an async GC so as much of the stack can be cleared as possible. Maybe we should provide such an API to WebCore? It could be as simple as `setTimeout(syncGC, 0)`

I think it would be possible that multiple GC cycles can be effective.
For example,

1. An object has Strong<> or something in its field while it is correctly handled to make non-cyclic leak
2. First GC makes this object dead
3. The destructor of (2) will destroy Strong<>
4. Then, the second GC makes held object of Strong<> dead

>> LayoutTests/http/tests/resources/gc.js:53
>> +        await waitFor(50);
> 
> Why do we need the await? isn't gc() sync?

Three reasons,

1. This `gc()` is async
2. Want to continue the next iteration in the new macro-task (setTimeout) to ensure that RunLoop is iterated (and stack is once cleared)
3. Because the original code was using this :)

[Yusuke Suzuki]

Committed r261391: <https://trac.webkit.org/changeset/261391>

[Yusuke Suzuki]

Note that I picked 20 iframe count which comes from resize-observer/element-leak.html.
And I've ensured that this is enough at least for Release builds in my local environment. But not sure how it is in Debug builds (it will generate code which uses CPU registers / stack in a different manner).
At least, in EWS, they are not failing. But if we still see the flakiness, we will increase it a bit. But I believe 20 is enough.