Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WPE][GTK] WebProcess crashes at the end of every GLib API test #15904

Conversation

obyknovenius
Copy link
Contributor

@obyknovenius obyknovenius commented Jul 18, 2023

4b27f17

[WPE][GTK] WebProcess crashes at the end of every GLib API test
https://bugs.webkit.org/show_bug.cgi?id=259303

Reviewed by Michael Catanzaro.

To run web process API tests we need to be able to execute code in
the web process. This is done by using web process extensions.

`WebProcessTest.cpp ` is a base class that implements the initialization
of a web process extension. This class also provides an API for
asserting that the `GObject` objects allocated in the web process during
test execution are deleted at the end of the test.

These `GObject` objects are stored in a static `HashSet`
`s_watchedObjects`. When the web process is terminating
`static void __attribute__((destructor)) checkLeaksAtExit()`
is called to verify that `s_watchedObjects` is empty, meaning
no objects were leaked.

The problem is that static variable destructors are invoking *before*
any `__attribute__((destructor))` functions. So by the time
`checkLeaksAtExit` is called, `s_watchedObjects` is already deallocated,
leading the web process to crash.

Because the web process extension modules we need for some tests are
located in the default directory, there are being loaded with every
API test. So we got the crash even if we don't run a web process test.

The solution to this problem is to wrap the watched objects hash set
into a static class and check for leaks in its destructor, before the
hash set is deallocated.

* Tools/TestWebKitAPI/Tests/WebKitGLib/DOMElementTest.cpp:
(DOMElementTest::testAutoFill):
* Tools/TestWebKitAPI/Tests/WebKitGLib/EditorTest.cpp:
(WebKitWebEditorTest::testSelectionChanged):
* Tools/TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp:
(WebKitFrameTest::testJavaScriptContext):
(WebKitFrameTest::testJavaScriptValues):
(WebKitFrameTest::testSubframe):
* Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp:
(runTest):
(webProcessTestRunnerFinalize):
(windowObjectClearedCallback):
(webkit_web_extension_initialize):
(checkLeaks): Deleted.
(WebProcessTest::assertObjectIsDeletedWhenTestFinishes): Deleted.
(checkLeaksAtExit): Deleted.
* Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.h:
(Watcher::~Watcher):
(Watcher::assertObjectIsDeletedWhenTestFinishes):
(Watcher::checkLeaks):
* Tools/TestWebKitAPI/Tests/WebKitGtk/AutocleanupsTest.cpp:
(AutocleanupsTest::testWebProcessAutocleanups):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMClientRectTest.cpp:
(WebKitDOMClientRectTest::testDivBoundingClientRectPosition):
(WebKitDOMClientRectTest::testDivClientRectsPositionAndLength):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMNodeFilterTest.cpp:
(WebKitDOMNodeFilterTest::testTreeWalker):
(WebKitDOMNodeFilterTest::testNodeIterator):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMNodeTest.cpp:
(WebKitDOMNodeTest::testHierarchyNavigation):
(WebKitDOMNodeTest::testInsertion):
(WebKitDOMNodeTest::testTagNamesNodeList):
(WebKitDOMNodeTest::testTagNamesHTMLCollection):
(WebKitDOMNodeTest::testDOMCache):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMXPathNSResolverTest.cpp:
(WebKitDOMXPathNSResolverTest::evaluateFooChildTextAndCheckResult):
(WebKitDOMXPathNSResolverTest::testXPathNSResolverNative):
(WebKitDOMXPathNSResolverTest::testXPathNSResolverCustom):

Canonical link: https://commits.webkit.org/266127@main

d9b6787

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug ✅ 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 ✅ 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 ✅ 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
✅ 🛠 tv ✅ 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 🧪 merge ✅ 🛠 watch
✅ 🛠 watch-sim

@obyknovenius obyknovenius requested a review from a team as a code owner July 18, 2023 11:09
@obyknovenius obyknovenius self-assigned this Jul 18, 2023
@obyknovenius obyknovenius added the WPE WebKit WebKit WPE component label Jul 18, 2023
Copy link
Contributor

@mcatanzaro mcatanzaro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good solution.

Technically static objects are also destroyed in exit handlers, but I suppose they are destroyed sooner than the exit handler for the library destructor. Destructors of static objects and exit handlers are all pretty dangerous and we see crashes from them regularly, but in this case the benefit of being able to check for leaks is clearly worth the risk.

Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp Outdated Show resolved Hide resolved
@obyknovenius obyknovenius force-pushed the fix/api-tests/webkit-process-crash branch from f3bdc78 to 11db54b Compare July 18, 2023 12:48
@obyknovenius
Copy link
Contributor Author

@mcatanzaro It would be nice to have an API function which is called when a web process extension is being destroyed. Something like:

webkit_web_process_extension_destroy(WebKitWebProcessExtension* extension)

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Jul 18, 2023
@obyknovenius obyknovenius removed the merging-blocked Applied to prevent a change from being merged label Jul 18, 2023
@obyknovenius obyknovenius force-pushed the fix/api-tests/webkit-process-crash branch from 11db54b to d9b6787 Compare July 18, 2023 13:17
@mcatanzaro
Copy link
Contributor

It would be nice to have an API function which is called when a web process extension is being destroyed.

Feel free to create a bug report if there's something you really want to do that cannot be done otherwise.

@obyknovenius obyknovenius added the merge-queue Applied to send a pull request to merge-queue label Jul 18, 2023
https://bugs.webkit.org/show_bug.cgi?id=259303

Reviewed by Michael Catanzaro.

To run web process API tests we need to be able to execute code in
the web process. This is done by using web process extensions.

`WebProcessTest.cpp ` is a base class that implements the initialization
of a web process extension. This class also provides an API for
asserting that the `GObject` objects allocated in the web process during
test execution are deleted at the end of the test.

These `GObject` objects are stored in a static `HashSet`
`s_watchedObjects`. When the web process is terminating
`static void __attribute__((destructor)) checkLeaksAtExit()`
is called to verify that `s_watchedObjects` is empty, meaning
no objects were leaked.

The problem is that static variable destructors are invoking *before*
any `__attribute__((destructor))` functions. So by the time
`checkLeaksAtExit` is called, `s_watchedObjects` is already deallocated,
leading the web process to crash.

Because the web process extension modules we need for some tests are
located in the default directory, there are being loaded with every
API test. So we got the crash even if we don't run a web process test.

The solution to this problem is to wrap the watched objects hash set
into a static class and check for leaks in its destructor, before the
hash set is deallocated.

* Tools/TestWebKitAPI/Tests/WebKitGLib/DOMElementTest.cpp:
(DOMElementTest::testAutoFill):
* Tools/TestWebKitAPI/Tests/WebKitGLib/EditorTest.cpp:
(WebKitWebEditorTest::testSelectionChanged):
* Tools/TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp:
(WebKitFrameTest::testJavaScriptContext):
(WebKitFrameTest::testJavaScriptValues):
(WebKitFrameTest::testSubframe):
* Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp:
(runTest):
(webProcessTestRunnerFinalize):
(windowObjectClearedCallback):
(webkit_web_extension_initialize):
(checkLeaks): Deleted.
(WebProcessTest::assertObjectIsDeletedWhenTestFinishes): Deleted.
(checkLeaksAtExit): Deleted.
* Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.h:
(Watcher::~Watcher):
(Watcher::assertObjectIsDeletedWhenTestFinishes):
(Watcher::checkLeaks):
* Tools/TestWebKitAPI/Tests/WebKitGtk/AutocleanupsTest.cpp:
(AutocleanupsTest::testWebProcessAutocleanups):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMClientRectTest.cpp:
(WebKitDOMClientRectTest::testDivBoundingClientRectPosition):
(WebKitDOMClientRectTest::testDivClientRectsPositionAndLength):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMNodeFilterTest.cpp:
(WebKitDOMNodeFilterTest::testTreeWalker):
(WebKitDOMNodeFilterTest::testNodeIterator):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMNodeTest.cpp:
(WebKitDOMNodeTest::testHierarchyNavigation):
(WebKitDOMNodeTest::testInsertion):
(WebKitDOMNodeTest::testTagNamesNodeList):
(WebKitDOMNodeTest::testTagNamesHTMLCollection):
(WebKitDOMNodeTest::testDOMCache):
* Tools/TestWebKitAPI/Tests/WebKitGtk/DOMXPathNSResolverTest.cpp:
(WebKitDOMXPathNSResolverTest::evaluateFooChildTextAndCheckResult):
(WebKitDOMXPathNSResolverTest::testXPathNSResolverNative):
(WebKitDOMXPathNSResolverTest::testXPathNSResolverCustom):

Canonical link: https://commits.webkit.org/266127@main
@webkit-commit-queue webkit-commit-queue force-pushed the fix/api-tests/webkit-process-crash branch from d9b6787 to 4b27f17 Compare July 18, 2023 14:43
@webkit-commit-queue
Copy link
Collaborator

Committed 266127@main (4b27f17): https://commits.webkit.org/266127@main

Reviewed commits have been landed. Closing PR #15904 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit 4b27f17 into WebKit:main Jul 18, 2023
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 18, 2023
@obyknovenius obyknovenius deleted the fix/api-tests/webkit-process-crash branch July 18, 2023 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WPE WebKit WebKit WPE component
Projects
None yet
5 participants