Skip to content

Commit f3d58ea

Browse files
carlosgcamposcarlosgcampos
committed
[GTK][WPE] Leak checker is not working in WebKitGLib web process tests
https://bugs.webkit.org/show_bug.cgi?id=183404 Reviewed by Michael Catanzaro. Source/WebKit: Add private helper for testing to do a garbage collection when the page is closing. * WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp: (webkitWebExtensionSetGarbageCollectOnPageDestroy): * WebProcess/InjectedBundle/API/glib/WebKitWebExtensionPrivate.h: * WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRectList.h: Fix annotation of webkit_dom_client_rect_list_item(), it should be transfer full. Tools: This might have regressed when we started to use the JSC garbage collector timers. The thing is that we expect that the WebProcessTester object that we expose to JavaScript is released when the web frame is destroyed, but that's no longer the case. On window object cleared a GC is scheduled, but JSC timers do the actual garbage collection later. In the case of tests that never happens because the web process finishes quickly after the test. We need to force a garbage collection at some point when the web page is destroyed. We can't use the WebKitWebPage destroy signal, since we are also checking that WebKitWebPage isn't leaked. The API::InjectedBundle::Client::willDestroyPage() always happen when the page is closed, even if WebKitWebPage is still alive, so we can force the GC at that point. The only problem is that the frame is detached right after that point, so we can't check WebKitFrame leaks. The only frame in the tests is the main one, so we can assume that if WebKitWebPage is released, the frame is too. * TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp: (WebKitFrameTest::testMainFrame): Stop checking we don't leak WebKitFrame. (WebKitFrameTest::testURI): Ditto. (WebKitFrameTest::testJavaScriptContext): Ditto. * TestWebKitAPI/Tests/WebKitGLib/TestFrame.cpp: (testWebKitFrameMainFrame): Use new WebViewTest::runWebProcessTest() API. (testWebKitFrameURI): Ditto. (testWebKitFrameJavaScriptContext): Ditto. (webkitFrameTestRun): Deleted. * TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp: (runTest): We no longer need the special case for dom-cache test. (windowObjectClearedCallback): Only expose test runner object to JavaScript when loading tests. (webkit_web_extension_initialize): Call webkitWebExtensionSetGarbageCollectOnPageDestroy() to ensure a garbage collection is performed when the page is closing. * TestWebKitAPI/Tests/WebKitGtk/DOMClientRectTest.cpp: (WebKitDOMClientRectTest::testDivClientRectsPositionAndLength): Fix memory leak, webkit_dom_client_rect_list_item() returns a full reference. * TestWebKitAPI/Tests/WebKitGtk/TestAutocleanups.cpp: (testWebProcessAutocleanups):Use new WebViewTest::runWebProcessTest() API. * TestWebKitAPI/Tests/WebKitGtk/TestDOMClientRect.cpp: (testWebKitDOMClientRectDivBoundingClientRectPosition): Use new WebViewTest::runWebProcessTest() API. (testWebKitDOMClientRectDivClientRectsPositionAndLength): Use new WebViewTest::runWebProcessTest() API. (prepareDOMForClientRectPositionTests): Deleted. * TestWebKitAPI/Tests/WebKitGtk/TestDOMNode.cpp: (testWebKitDOMNodeHierarchyNavigation): Use new WebViewTest::runWebProcessTest() API. (testWebKitDOMNodeInsertion): Ditto. (testWebKitDOMNodeTagNamesNodeList): Ditto. (testWebKitDOMNodeTagNamesHTMLCollection): Ditto. (testWebKitDOMObjectCache): We no longer need to run the test several times, since runWebProcessTest() loads about blank after every test. (prepareDOMForTagNamesTests): Deleted. * TestWebKitAPI/Tests/WebKitGtk/TestDOMNodeFilter.cpp: (testWebKitDOMNodeFilterTreeWalker): Use new WebViewTest::runWebProcessTest() API. (testWebKitDOMNodeFilterNodeIterator): Ditto. (runTest): Deleted. * TestWebKitAPI/Tests/WebKitGtk/TestDOMXPathNSResolver.cpp: (testWebKitDOMXPathNSResolverNative): Use new WebViewTest::runWebProcessTest() API. (testWebKitDOMXPathNSResolverCustom): Ditto. * TestWebKitAPI/Tests/WebKitGtk/TestEditor.cpp: (testWebKitWebEditorSelectionChanged): Use new WebViewTest::runWebProcessTest() API. * TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp: (WebViewTest::runWebProcessTest): It now receives the contents, so it automatically loads the view using "webprocess://test" as base URI, used to detect tests in the web process. It also loads about:blank after every test to ensure that window object is cleared. * TestWebKitAPI/glib/WebKitGLib/WebViewTest.h: Canonical link: https://commits.webkit.org/199102@main git-svn-id: https://svn.webkit.org/repository/webkit/trunk@229395 268f45cc-cd09-0410-ab3c-d52691b4dbfc
1 parent de6e8f2 commit f3d58ea

17 files changed

Lines changed: 152 additions & 112 deletions

‎Source/WebKit/ChangeLog‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,18 @@
1+
2018-03-07 Carlos Garcia Campos <[email protected]>
2+
3+
[GTK][WPE] Leak checker is not working in WebKitGLib web process tests
4+
https://bugs.webkit.org/show_bug.cgi?id=183404
5+
6+
Reviewed by Michael Catanzaro.
7+
8+
Add private helper for testing to do a garbage collection when the page is closing.
9+
10+
* WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp:
11+
(webkitWebExtensionSetGarbageCollectOnPageDestroy):
12+
* WebProcess/InjectedBundle/API/glib/WebKitWebExtensionPrivate.h:
13+
* WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRectList.h: Fix annotation of
14+
webkit_dom_client_rect_list_item(), it should be transfer full.
15+
116
2018-03-07 Youenn Fablet <[email protected]>
217

318
Match unsupported plugins based on domains and not origin

‎Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtension.cpp‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -26,6 +26,7 @@
2626
#include "WebKitWebExtensionPrivate.h"
2727
#include "WebKitWebPagePrivate.h"
2828
#include "WebProcess.h"
29+
#include <WebCore/GCController.h>
2930
#include <wtf/HashMap.h>
3031
#include <wtf/glib/GRefPtr.h>
3132
#include <wtf/glib/WTFGType.h>
@@ -118,6 +119,9 @@ typedef HashMap<WebPage*, GRefPtr<WebKitWebPage> > WebPageMap;
118119

119120
struct _WebKitWebExtensionPrivate {
120121
WebPageMap pages;
122+
#if ENABLE(DEVELOPER_MODE)
123+
bool garbageCollectOnPageDestroy;
124+
#endif
121125
};
122126

123127
static guint signals[LAST_SIGNAL] = { 0, };
@@ -162,6 +166,10 @@ class WebExtensionInjectedBundleClient final : public API::InjectedBundle::Clien
162166
void willDestroyPage(InjectedBundle&, WebPage& page) override
163167
{
164168
m_extension->priv->pages.remove(&page);
169+
#if ENABLE(DEVELOPER_MODE)
170+
if (m_extension->priv->garbageCollectOnPageDestroy)
171+
WebCore::GCController::singleton().garbageCollectNow();
172+
#endif
165173
}
166174

167175
void didReceiveMessage(InjectedBundle&, const String& messageName, API::Object* messageBody) override
@@ -192,6 +200,13 @@ WebKitWebExtension* webkitWebExtensionCreate(InjectedBundle* bundle)
192200
return extension;
193201
}
194202

203+
void webkitWebExtensionSetGarbageCollectOnPageDestroy(WebKitWebExtension* extension)
204+
{
205+
#if ENABLE(DEVELOPER_MODE)
206+
extension->priv->garbageCollectOnPageDestroy = true;
207+
#endif
208+
}
209+
195210
/**
196211
* webkit_web_extension_get_page:
197212
* @extension: a #WebKitWebExtension

‎Source/WebKit/WebProcess/InjectedBundle/API/glib/WebKitWebExtensionPrivate.h‎

Lines changed: 2 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,10 @@
1717
* Boston, MA 02110-1301, USA.
1818
*/
1919

20-
#ifndef WebKitWebExtensionPrivate_h
21-
#define WebKitWebExtensionPrivate_h
20+
#pragma once
2221

2322
#include "InjectedBundle.h"
2423
#include "WebKitWebExtension.h"
2524

2625
WebKitWebExtension* webkitWebExtensionCreate(WebKit::InjectedBundle*);
27-
28-
#endif // WebKitWebExtensionPrivate_h
26+
void webkitWebExtensionSetGarbageCollectOnPageDestroy(WebKitWebExtension*);

‎Source/WebKit/WebProcess/InjectedBundle/API/gtk/DOM/WebKitDOMClientRectList.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ webkit_dom_client_rect_list_get_length(WebKitDOMClientRectList* self);
6868
*
6969
* Returns the #WebKitDOMClientRect object that @self contains at @index.
7070
*
71-
* Returns: (transfer none): A #WebKitDOMClientRect
71+
* Returns: (transfer full): A #WebKitDOMClientRect
7272
*
7373
* Since: 2.18
7474
**/

‎Tools/ChangeLog‎

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,67 @@
1+
2018-03-07 Carlos Garcia Campos <[email protected]>
2+
3+
[GTK][WPE] Leak checker is not working in WebKitGLib web process tests
4+
https://bugs.webkit.org/show_bug.cgi?id=183404
5+
6+
Reviewed by Michael Catanzaro.
7+
8+
This might have regressed when we started to use the JSC garbage collector timers. The thing is that we expect
9+
that the WebProcessTester object that we expose to JavaScript is released when the web frame is destroyed, but
10+
that's no longer the case. On window object cleared a GC is scheduled, but JSC timers do the actual garbage
11+
collection later. In the case of tests that never happens because the web process finishes quickly after the
12+
test. We need to force a garbage collection at some point when the web page is destroyed. We can't use the
13+
WebKitWebPage destroy signal, since we are also checking that WebKitWebPage isn't leaked. The
14+
API::InjectedBundle::Client::willDestroyPage() always happen when the page is closed, even if WebKitWebPage is
15+
still alive, so we can force the GC at that point. The only problem is that the frame is detached right after
16+
that point, so we can't check WebKitFrame leaks. The only frame in the tests is the main one, so we can assume
17+
that if WebKitWebPage is released, the frame is too.
18+
19+
* TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp:
20+
(WebKitFrameTest::testMainFrame): Stop checking we don't leak WebKitFrame.
21+
(WebKitFrameTest::testURI): Ditto.
22+
(WebKitFrameTest::testJavaScriptContext): Ditto.
23+
* TestWebKitAPI/Tests/WebKitGLib/TestFrame.cpp:
24+
(testWebKitFrameMainFrame): Use new WebViewTest::runWebProcessTest() API.
25+
(testWebKitFrameURI): Ditto.
26+
(testWebKitFrameJavaScriptContext): Ditto.
27+
(webkitFrameTestRun): Deleted.
28+
* TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp:
29+
(runTest): We no longer need the special case for dom-cache test.
30+
(windowObjectClearedCallback): Only expose test runner object to JavaScript when loading tests.
31+
(webkit_web_extension_initialize): Call webkitWebExtensionSetGarbageCollectOnPageDestroy() to ensure a garbage
32+
collection is performed when the page is closing.
33+
* TestWebKitAPI/Tests/WebKitGtk/DOMClientRectTest.cpp:
34+
(WebKitDOMClientRectTest::testDivClientRectsPositionAndLength): Fix memory leak,
35+
webkit_dom_client_rect_list_item() returns a full reference.
36+
* TestWebKitAPI/Tests/WebKitGtk/TestAutocleanups.cpp:
37+
(testWebProcessAutocleanups):Use new WebViewTest::runWebProcessTest() API.
38+
* TestWebKitAPI/Tests/WebKitGtk/TestDOMClientRect.cpp:
39+
(testWebKitDOMClientRectDivBoundingClientRectPosition): Use new WebViewTest::runWebProcessTest() API.
40+
(testWebKitDOMClientRectDivClientRectsPositionAndLength): Use new WebViewTest::runWebProcessTest() API.
41+
(prepareDOMForClientRectPositionTests): Deleted.
42+
* TestWebKitAPI/Tests/WebKitGtk/TestDOMNode.cpp:
43+
(testWebKitDOMNodeHierarchyNavigation): Use new WebViewTest::runWebProcessTest() API.
44+
(testWebKitDOMNodeInsertion): Ditto.
45+
(testWebKitDOMNodeTagNamesNodeList): Ditto.
46+
(testWebKitDOMNodeTagNamesHTMLCollection): Ditto.
47+
(testWebKitDOMObjectCache): We no longer need to run the test several times, since runWebProcessTest() loads
48+
about blank after every test.
49+
(prepareDOMForTagNamesTests): Deleted.
50+
* TestWebKitAPI/Tests/WebKitGtk/TestDOMNodeFilter.cpp:
51+
(testWebKitDOMNodeFilterTreeWalker): Use new WebViewTest::runWebProcessTest() API.
52+
(testWebKitDOMNodeFilterNodeIterator): Ditto.
53+
(runTest): Deleted.
54+
* TestWebKitAPI/Tests/WebKitGtk/TestDOMXPathNSResolver.cpp:
55+
(testWebKitDOMXPathNSResolverNative): Use new WebViewTest::runWebProcessTest() API.
56+
(testWebKitDOMXPathNSResolverCustom): Ditto.
57+
* TestWebKitAPI/Tests/WebKitGtk/TestEditor.cpp:
58+
(testWebKitWebEditorSelectionChanged): Use new WebViewTest::runWebProcessTest() API.
59+
* TestWebKitAPI/glib/WebKitGLib/WebViewTest.cpp:
60+
(WebViewTest::runWebProcessTest): It now receives the contents, so it automatically loads the view using
61+
"webprocess://test" as base URI, used to detect tests in the web process. It also loads about:blank after every
62+
test to ensure that window object is cleared.
63+
* TestWebKitAPI/glib/WebKitGLib/WebViewTest.h:
64+
165
2018-03-07 Youenn Fablet <[email protected]>
266

367
Match unsupported plugins based on domains and not origin

‎Tools/TestWebKitAPI/Tests/WebKitGLib/FrameTest.cpp‎

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,7 +31,6 @@ class WebKitFrameTest : public WebProcessTest {
3131
{
3232
WebKitFrame* frame = webkit_web_page_get_main_frame(page);
3333
g_assert(WEBKIT_IS_FRAME(frame));
34-
assertObjectIsDeletedWhenTestFinishes(G_OBJECT(frame));
3534
g_assert(webkit_frame_is_main_frame(frame));
3635

3736
return true;
@@ -41,7 +40,6 @@ class WebKitFrameTest : public WebProcessTest {
4140
{
4241
WebKitFrame* frame = webkit_web_page_get_main_frame(page);
4342
g_assert(WEBKIT_IS_FRAME(frame));
44-
assertObjectIsDeletedWhenTestFinishes(G_OBJECT(frame));
4543
g_assert_cmpstr(webkit_web_page_get_uri(page), ==, webkit_frame_get_uri(frame));
4644

4745
return true;
@@ -51,7 +49,6 @@ class WebKitFrameTest : public WebProcessTest {
5149
{
5250
WebKitFrame* frame = webkit_web_page_get_main_frame(page);
5351
g_assert(WEBKIT_IS_FRAME(frame));
54-
assertObjectIsDeletedWhenTestFinishes(G_OBJECT(frame));
5552
g_assert(webkit_frame_get_javascript_global_context(frame));
5653

5754
return true;

‎Tools/TestWebKitAPI/Tests/WebKitGLib/TestFrame.cpp‎

Lines changed: 3 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -21,28 +21,19 @@
2121

2222
#include "WebViewTest.h"
2323

24-
static void webkitFrameTestRun(WebViewTest* test, const char* testName)
25-
{
26-
static const char* testHTML = "<html><body></body></html>";
27-
test->loadHtml(testHTML, 0);
28-
test->waitUntilLoadFinished();
29-
30-
g_assert(test->runWebProcessTest("WebKitFrame", testName));
31-
}
32-
3324
static void testWebKitFrameMainFrame(WebViewTest* test, gconstpointer)
3425
{
35-
webkitFrameTestRun(test, "main-frame");
26+
g_assert(test->runWebProcessTest("WebKitFrame", "main-frame"));
3627
}
3728

3829
static void testWebKitFrameURI(WebViewTest* test, gconstpointer)
3930
{
40-
webkitFrameTestRun(test, "uri");
31+
g_assert(test->runWebProcessTest("WebKitFrame", "uri"));
4132
}
4233

4334
static void testWebKitFrameJavaScriptContext(WebViewTest* test, gconstpointer)
4435
{
45-
webkitFrameTestRun(test, "javascript-context");
36+
g_assert(test->runWebProcessTest("WebKitFrame", "javascript-context"));
4637
}
4738

4839
void beforeAll()

‎Tools/TestWebKitAPI/Tests/WebKitGLib/WebProcessTest.cpp‎

Lines changed: 6 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -20,6 +20,7 @@
2020
#include "config.h"
2121
#include "WebProcessTest.h"
2222

23+
#include "WebKitWebExtensionPrivate.h"
2324
#include <JavaScriptCore/JSRetainPtr.h>
2425
#include <gio/gio.h>
2526
#include <wtf/HashSet.h>
@@ -64,14 +65,7 @@ static JSValueRef runTest(JSContextRef context, JSObjectRef function, JSObjectRe
6465

6566
WebKitWebPage* webPage = WEBKIT_WEB_PAGE(JSObjectGetPrivate(thisObject));
6667
g_assert(WEBKIT_IS_WEB_PAGE(webPage));
67-
// Test /WebKitDOMNode/dom-cache is an exception, because it's called 3 times, so
68-
// the WebPage is destroyed after the third time.
69-
if (g_str_equal(testPath.get(), "WebKitDOMNode/dom-cache")) {
70-
static unsigned domCacheTestRunCount = 0;
71-
if (++domCacheTestRunCount == 3)
72-
WebProcessTest::assertObjectIsDeletedWhenTestFinishes(G_OBJECT(webPage));
73-
} else
74-
WebProcessTest::assertObjectIsDeletedWhenTestFinishes(G_OBJECT(webPage));
68+
WebProcessTest::assertObjectIsDeletedWhenTestFinishes(G_OBJECT(webPage));
7569

7670
std::unique_ptr<WebProcessTest> test = WebProcessTest::create(String::fromUTF8(testPath.get()));
7771
return JSValueMakeBoolean(context, test->runTest(g_strrstr(testPath.get(), "/") + 1, webPage));
@@ -100,6 +94,9 @@ static void webProcessTestRunnerFinalize(JSObjectRef object)
10094

10195
static void windowObjectClearedCallback(WebKitScriptWorld* world, WebKitWebPage* webPage, WebKitFrame* frame, WebKitWebExtension* extension)
10296
{
97+
if (g_strcmp0(webkit_web_page_get_uri(webPage), "webprocess://test"))
98+
return;
99+
103100
JSGlobalContextRef context = webkit_frame_get_javascript_context_for_script_world(frame, world);
104101
JSObjectRef globalObject = JSContextGetGlobalObject(context);
105102

@@ -117,5 +114,6 @@ static void windowObjectClearedCallback(WebKitScriptWorld* world, WebKitWebPage*
117114

118115
extern "C" void webkit_web_extension_initialize(WebKitWebExtension* extension)
119116
{
117+
webkitWebExtensionSetGarbageCollectOnPageDestroy(extension);
120118
g_signal_connect(webkit_script_world_get_default(), "window-object-cleared", G_CALLBACK(windowObjectClearedCallback), extension);
121119
}

‎Tools/TestWebKitAPI/Tests/WebKitGtk/DOMClientRectTest.cpp‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -74,13 +74,14 @@ class WebKitDOMClientRectTest : public WebProcessTest {
7474

7575
g_assert_cmpuint(webkit_dom_client_rect_list_get_length(clientRectList.get()), ==, 1);
7676

77-
WebKitDOMClientRect* clientRect = webkit_dom_client_rect_list_item(clientRectList.get(), 0);
78-
g_assert(WEBKIT_DOM_IS_CLIENT_RECT(clientRect));
79-
assertObjectIsDeletedWhenTestFinishes(G_OBJECT(clientRect));
80-
checkClientRectPosition(clientRect);
77+
GRefPtr<WebKitDOMClientRect> clientRect = adoptGRef(webkit_dom_client_rect_list_item(clientRectList.get(), 0));
78+
g_assert(WEBKIT_DOM_IS_CLIENT_RECT(clientRect.get()));
79+
assertObjectIsDeletedWhenTestFinishes(G_OBJECT(clientRect.get()));
80+
checkClientRectPosition(clientRect.get());
8181

8282
// Getting the clientRect twice should return the same pointer.
83-
g_assert(webkit_dom_client_rect_list_item(clientRectList.get(), 0) == webkit_dom_client_rect_list_item(clientRectList.get(), 0));
83+
GRefPtr<WebKitDOMClientRect> clientRect2 = adoptGRef(webkit_dom_client_rect_list_item(clientRectList.get(), 0));
84+
g_assert(clientRect.get() == clientRect2.get());
8485

8586
return true;
8687
}

‎Tools/TestWebKitAPI/Tests/WebKitGtk/TestAutocleanups.cpp‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,10 +44,6 @@ static void testUIProcessAutocleanups(WebViewTest* test, gconstpointer)
4444

4545
static void testWebProcessAutocleanups(WebViewTest* test, gconstpointer)
4646
{
47-
static const char* testHTML = "<html><body></body></html>";
48-
test->loadHtml(testHTML, nullptr);
49-
test->waitUntilLoadFinished();
50-
5147
g_assert(test->runWebProcessTest("Autocleanups", "web-process-autocleanups"));
5248
}
5349

0 commit comments

Comments
 (0)