Bug 222972 – [WPE][GTK] TestJSC incorrectly expects garbage collector to collect variables still on the stack

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Michael Catanzaro]

In bug #181916 we noticed that TestJSC (the testsuite for WPE and GTK ports' JSC API) started failing after switching the GTK port to build with -fvisibility=hidden. Don and I were pretty dumbfounded, but Mark Lam proposed a theory:

"""
My theory is that the -fvisibility=hidden now makes it such that some functions get inlined, thereby changing the stack values, which in turn potentially keeps a pointer to a dead object on the stack (though the value is not used).  With a conservative GC, that keeps the object alive.

...

The test is making an assumption that there is no temp on the stack / in a register storing the value of weakFoo

try this: 1. move this code into a function and call it:
        weakFoo = adoptGRef(jsc_weak_value_get_value(weak.get()));
        checker.watch(weakFoo.get());
        g_assert_true(jsc_value_is_object(weakFoo.get()));
        weakFoo = nullptr;

2. call sanitizeStackForVM() after that before you do the GC.

See if that makes the test pass

Actually, move all the code that uses weakFoo into the out of line function

If this makes the test pass, then it confirms my suspicion that the test is just badly written and made assumptions about the GC that it shouldn’t have

Alternatively, you can also do this:

1. Set a breakpoint at 3305 and inspect the value of weakFoo
2. Set a breakpoint at 3310, and check if the value of weakFoo is in any registers or in any stack slots
"""

After implementing Mark's suggestion, the test began to pass, so it's probably right. However, my current implementation is messy and not suitable to commit. It also touches only the test that was actually failing, /JSC/weak-value. We should probably audit the entire file.

[Michael Catanzaro]

Created attachment 422692 [details]
My test patch from bug #181916

[Michael Catanzaro]

I'm going to break this test and mark it as flaky in bug #181916. When fixed, we need to update Tools/TestWebKitAPI/glib/TestExpectations.json.

[Michael Catanzaro]

Created attachment 422753 [details]
Yucky test patch that fixes this test

[Michael Catanzaro]

Comment on attachment 422753 [details]
Yucky test patch that fixes this test

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

> a/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:114
> +    JSContextGroupRef jsContextGroup = jscVirtualMachineGetContextGroup(jscVM);

BTW this is probably leaked. Not sure how refcounting works with the C API.

[Carlos Garcia Campos]

Comment on attachment 422753 [details]
Yucky test patch that fixes this test

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

> a/Tools/TestWebKitAPI/Tests/JavaScriptCore/glib/TestJSC.cpp:118
> +static void jscContextSanitizeStack(JSCContext* context)
> +{
> +    JSCVirtualMachine* jscVM = jsc_context_get_virtual_machine(context);
> +    JSContextGroupRef jsContextGroup = jscVirtualMachineGetContextGroup(jscVM);
> +    JSC::VM& vm = *toJS(jsContextGroup);
> +    JSC::JSLockHolder locker(vm);
> +    sanitizeStackForVM(vm);
> +}

I think we could add JSCContextInternal.h with jscContextGarbageCollect() and jscContextSanitizeStack() implemented in JSCContext.cpp. That way we only need to include the internal header from the tests and not use the C API nor internal API from tests

[Carlos Alberto Lopez Perez]

This seems related to bug 229321

[Carlos Garcia Campos]

It seems calling sanitizeStackForVM is enough to make the test pass with hidden symbols, no need to move code to a function.

[Carlos Garcia Campos]

Created attachment 440206 [details]
Patch

[Carlos Garcia Campos]

Committed r283606 (242558@main): <https://commits.webkit.org/242558@main>

[Radar WebKit Bug Importer]

<rdar://problem/83922969>