Bug 48429 – [Gtk] Populate DumpRenderTreeSupportGtk

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKitGTK

[Antonio Gomes]

Bug 48199 added DumpRenderTreeSupport, so we extend test coverage in GTK's DRT without needing new API addition. It landed with preliminary structs for now. This bug is about populating it.

There is a bunch of private API's declared in webkitprivate.h. That might be a good start point.

[Antonio Gomes]

Created attachment 73245 [details]
part I - moved webkit_web_frame* method in webkitprivat.h to DRTSupportGtk.

[Martin Robinson]

Comment on attachment 73245 [details]
part I - moved webkit_web_frame* method in webkitprivat.h to DRTSupportGtk.

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

Great patch. One general comment is that I would like to avoid using gchar, gint, etc in DumpRenderTreeSupport API. Our guidelines indicate that we want to avoid using GLib types, unless we are dealing with GLib-style APIs.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:106
> +gchar* DumpRenderTreeSupportGtk::getInnerText(WebKitWebFrame* frame)

I think that since this is a private API we can return a CString here and relieve the burden of memory management from DumpRenderTree.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:108
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

NULL should be 0 in new code.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:133
> +gchar* DumpRenderTreeSupportGtk::dumpRenderTree(WebKitWebFrame* frame)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);
> +

Same comments as above.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:156
> +gchar* DumpRenderTreeSupportGtk::counterValueForElementById(WebKitWebFrame* frame, const gchar* id)
> +{
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

Ditto.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:180
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

NULL -> 0.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:202
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

Ditto.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:242
> +bool DumpRenderTreeSupportGtk::pauseSvgAnimation(WebKitWebFrame* frame, const gchar* animationId, double time, const gchar* elementId)

Svg should be SVG as per the style rules.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:258
> +gchar* DumpRenderTreeSupportGtk::markerTextForListItem(WebKitWebFrame* frame, JSContextRef context, JSValueRef nodeObject)

CString here as well.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:304
> +    g_return_val_if_fail(WEBKIT_IS_WEB_FRAME(frame), NULL);

NULL -> 0 throughout this method.

[Antonio Gomes]

Created attachment 73606 [details]
patch v2

Fixed Martin's requests.

[Antonio Gomes]

Surprisingly enough it runs fine on Debug builds, but I am crashing on release:

Program received signal SIGSEGV, Segmentation fault.
0x08060d83 in WTF::fastFree(void*) ()
(gdb) bt
#0  0x08060d83 in WTF::fastFree(void*) ()
#1  0x080594b7 in dumpFramesAsText(_WebKitWebFrame*) ()
#2  0x08059784 in dump() ()
#3  0x0805d4e6 in LayoutTestController::notifyDone() ()
#4  0x0805155d in notifyDoneCallback(OpaqueJSContext const*, OpaqueJSValue*, OpaqueJSValue*, unsigned int, OpaqueJSValue const* const*, OpaqueJSValue const**) ()
#5  0xb7ca7908 in JSC::JSCallbackFunction::call(JSC::ExecState*) () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#6  0xb7d38281 in cti_op_call_NotJSFunction () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#7  0xadd476d2 in ?? ()
#8  0xb7cfa49d in JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) ()
   from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#9  0xb7d86c3f in JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) ()
   from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#10 0xb70c562f in WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValue, WebCore::ScriptExecutionContext*) ()
   from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#11 0xb70c655c in WebCore::ScheduledAction::execute(WebCore::Document*) () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#12 0xb70c661b in WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext*) () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#13 0xb744cf3e in WebCore::DOMTimer::fired() () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#14 0xb750831d in WebCore::ThreadTimers::sharedTimerFiredInternal() () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#15 0xb7508395 in WebCore::ThreadTimers::sharedTimerFired() () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#16 0xb777a1be in WebCore::timeout_cb(void*) () from /home/agomes/Devel/webkit/webkit/WebKitBuild/Gtk/Release/.libs/libwebkitgtk-1.0.so.0
#17 0xb54fffcc in ?? () from /lib/libglib-2.0.so.0
#18 0xb54ff855 in g_main_context_dispatch () from /lib/libglib-2.0.so.0
#19 0xb5503668 in ?? () from /lib/libglib-2.0.so.0
#20 0xb5503ba7 in g_main_loop_run () from /lib/libglib-2.0.so.0
#21 0xb59c31d9 in gtk_main () from /usr/lib/libgtk-x11-2.0.so.0
#22 0x08059b5f in runTest(std::basic_string<char, std::char_traits<char>, std::allocator<char> > const&) ()
#23 0x0805a337 in main ()

It crashes in dumpFramesAsText if I call DRTSupportGtk::getInnerText() . Ideas?

[Antonio Gomes]

I've trying to get this crash reproducible on debug build without success so far.

As per Martin's suggestion on IRC, I tried enabling fast malloc (which is disable for debug build) as following, and results are ok.


diff --git a/JavaScriptCore/wtf/Platform.h b/JavaScriptCore/wtf/Platform.h
index 3eda2da..982d18a 100644
--- a/JavaScriptCore/wtf/Platform.h
+++ b/JavaScriptCore/wtf/Platform.h
@@ -674,6 +674,9 @@
 #define WTF_USE_PTHREADS 1
 #define HAVE_PTHREAD_RWLOCK 1
 #endif
+#define ENABLE_FAST_MALLOC 1
+#define USE_SYSTEM_MALLOC 0
+#define ENABLE_FAST_MALLOC_MATCH_VALIDATION 0
 #endif


I am enabling it correctly?

[Antonio Gomes]

> diff --git a/JavaScriptCore/wtf/Platform.h b/JavaScriptCore/wtf/Platform.h
> index 3eda2da..982d18a 100644
> --- a/JavaScriptCore/wtf/Platform.h
> +++ b/JavaScriptCore/wtf/Platform.h
> @@ -674,6 +674,9 @@
>  #define WTF_USE_PTHREADS 1
>  #define HAVE_PTHREAD_RWLOCK 1
>  #endif
> +#define ENABLE_FAST_MALLOC 1
> +#define USE_SYSTEM_MALLOC 0
> +#define ENABLE_FAST_MALLOC_MATCH_VALIDATION 0
>  #endif

As a side note, if I enable ENABLE_FAST_MALLOC_MATCH_VALIDATION manually, I get many more crashes, making DRT impossible to run...

[Martin Robinson]

Sorry for the late response. I tried to test this locally, but the patch no longer applies cleanly. :( Another way to test enabling fast malloc for the debug build is to remove the special logic from configure.ac at line 675 and then do a clean rebuild. The final line will look something like this:

[],[enable_fast_malloc="yes"])

[Antonio Gomes]

Created attachment 74123 [details]
v2.1 - rebase against trunk

There we go, Martin :)

[Antonio Gomes]

Right, was I able to build debug with FAST_MALLOC enabled, but the crash is still not reproducible in debug builds.

 259     CString innerText = DumpRenderTreeSupportGtk::getInnerText(frame);
 261     if (isMainFrame)
 263         result = g_strdup_printf("%s\n", "oi" /*innerText.data()*/);
 265     else {
 266         const gchar* frameName = webkit_web_frame_get_name(frame);
 267         result = g_strdup_printf("\n--------\nFrame: '%s'\n--------\n%s\n", frameName, innerText.data());
 268     }
 269     
 271     if (gLayoutTestController->dumpChildFramesAsText()) {
 272         GSList* children = DumpRenderTreeSupportGtk::getChildren(frame);
 273         for (GSList* child = children; child; child = g_slist_next(child))
 274             appendString(result, dumpFramesAsText(static_cast<WebKitWebFrame* >(child->data)));
 275         g_slist_free(children);
 276     }
 277 
 279     return result;

'innerText' is refcount'ed and it crashes as soon as the method ends, i.e. innerText is going to be deleted.

[Martin Robinson]

It seems the issue here is that there are two copies of JSC in DumpRenderTree (bad news!) and the fastAlloc is happening in one copy and the fastFree in another (even badder news!).

[Antonio Gomes]

Created attachment 75526 [details]
(committed r73325 - r=mrobinson) patch v2.3 - rebased to ToT

Working patch.

[Martin Robinson]

Comment on attachment 75526 [details]
(committed r73325 - r=mrobinson) patch v2.3 - rebased to ToT

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

Looks good! Some of the methods are named slightly differently than others. It's not a huge deal though.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:106
> +GSList* DumpRenderTreeSupportGtk::getChildren(WebKitWebFrame* frame)

It might make sense to call this getFrameChildren, since it's no longer a method of the frame.

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:223
> +int DumpRenderTreeSupportGtk::numberOfPages(WebKitWebFrame* frame, float pageWidth, float pageHeight)

Maybe numberOfPagesForFrame?

[Antonio Gomes]

Comment on attachment 75526 [details]
(committed r73325 - r=mrobinson) patch v2.3 - rebased to ToT

Clearing flags on attachment: 75526

Committed r73325: <http://trac.webkit.org/changeset/73325>

[Antonio Gomes]

Lets keep it opened as a centralized for related patches.

[Antonio Gomes]

Created attachment 75609 [details]
(committed r73348, r=mrobinson) patch 2 - v1

Moving more methods.

[Antonio Gomes]

Note: as a followup I will try to fix stuff like:

276     gchar* sourceOriginGChar = JSStringCopyUTF8CString(sourceOrigin);
277     gchar* protocolGChar = JSStringCopyUTF8CString(protocol);
278     gchar* hostGChar = JSStringCopyUTF8CString(host);
279     DumpRenderTreeSupportGtk::whiteListAccessFromOrigin(sourceOriginGChar, protocolGChar, hostGChar, includeSubdomains);
280     g_free(sourceOriginGChar);
281     g_free(protocolGChar);
282     g_free(hostGChar);

We do not need all this gchar's and g_free's.

[Martin Robinson]

Comment on attachment 75609 [details]
(committed r73348, r=mrobinson) patch 2 - v1

Thanks!

[Antonio Gomes]

Comment on attachment 75609 [details]
(committed r73348, r=mrobinson) patch 2 - v1

Clearing flags on attachment: 75609

Committed r73348: <http://trac.webkit.org/changeset/73348>

[Antonio Gomes]

Created attachment 75905 [details]
(committed r73527 - r=mrobinson) patch 3 - v1

A few more methods moved, and I think I am done here :)

[Martin Robinson]

Comment on attachment 75905 [details]
(committed r73527 - r=mrobinson) patch 3 - v1

Awesome.

[Antonio Gomes]

Comment on attachment 75905 [details]
(committed r73527 - r=mrobinson) patch 3 - v1

Clearing flags on attachment: 75905

Committed r73527: <http://trac.webkit.org/changeset/73527>

[Antonio Gomes]

I am wondering about 

356     WEBKIT_API unsigned int
357     webkit_worker_thread_count();

in webkitprivate. It has a WebKit/gtk/webkit/webkitworkers.cpp just to implement this method, and it just used by DRT.

Thoughts?

[Martin Robinson]

I think this should also move to DRTSupport.

[Antonio Gomes]

Created attachment 76057 [details]
patch 4 - v1

As you wish...

[Antonio Gomes]

Created attachment 76076 [details]
(committed r73628, r=mrobinson) patch 4 - v2

unbitrotted patch

[Martin Robinson]

Comment on attachment 76076 [details]
(committed r73628, r=mrobinson) patch 4 - v2

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

Yay!

> WebKit/gtk/WebCoreSupport/DumpRenderTreeSupportGtk.cpp:7
> +<<<<<<< HEAD
>   *  Copyright (C) 2010 Joone Hur <[e-mail]>
> +=======
> + *  Copyright (C) 2009 Google Inc. All rights reserved.
> +>>>>>>> 2010-12-09  Antonio Gomes  <[e-mail]>

Please resolve these changes before landing.

[Antonio Gomes]

Comment on attachment 76076 [details]
(committed r73628, r=mrobinson) patch 4 - v2

Clearing flags on attachment: 76076

Committed r73628: <http://trac.webkit.org/changeset/73628>