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.
Created attachment 422692 [details] My test patch from bug #181916
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.
Created attachment 422753 [details] Yucky test patch that fixes this test
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.
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
This seems related to bug 229321
It seems calling sanitizeStackForVM is enough to make the test pass with hidden symbols, no need to move code to a function.
Created attachment 440206 [details] Patch
Committed r283606 (242558@main): <https://commits.webkit.org/242558@main>
<rdar://problem/83922969>