Bug 48429 - [Gtk] Populate DumpRenderTreeSupportGtk
Summary: [Gtk] Populate DumpRenderTreeSupportGtk
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Antonio Gomes
URL:
Keywords:
Depends on: 49877
Blocks: 48199
  Show dependency treegraph
 
Reported: 2010-10-27 07:38 PDT by Antonio Gomes
Modified: 2010-12-09 11:01 PST (History)
5 users (show)

See Also:


Attachments
part I - moved webkit_web_frame* method in webkitprivat.h to DRTSupportGtk. (38.48 KB, patch)
2010-11-08 09:28 PST, Antonio Gomes
mrobinson: review-
Details | Formatted Diff | Diff
patch v2 (39.86 KB, patch)
2010-11-11 06:45 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
v2.1 - rebase against trunk (40.24 KB, patch)
2010-11-17 09:01 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed r73325 - r=mrobinson) patch v2.3 - rebased to ToT (40.23 KB, patch)
2010-12-03 12:36 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed r73348, r=mrobinson) patch 2 - v1 (13.42 KB, patch)
2010-12-04 07:43 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed r73527 - r=mrobinson) patch 3 - v1 (11.50 KB, patch)
2010-12-08 08:01 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
patch 4 - v1 (8.81 KB, patch)
2010-12-09 06:54 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff
(committed r73628, r=mrobinson) patch 4 - v2 (9.13 KB, patch)
2010-12-09 09:48 PST, Antonio Gomes
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antonio Gomes 2010-10-27 07:38:44 PDT
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.
Comment 1 Antonio Gomes 2010-11-08 09:28:39 PST
Created attachment 73245 [details]
part I - moved webkit_web_frame* method in webkitprivat.h to DRTSupportGtk.
Comment 2 Martin Robinson 2010-11-08 09:39:23 PST
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.
Comment 3 Antonio Gomes 2010-11-11 06:45:43 PST
Created attachment 73606 [details]
patch v2

Fixed Martin's requests.
Comment 4 Antonio Gomes 2010-11-11 06:47:30 PST
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?
Comment 5 Antonio Gomes 2010-11-14 07:00:06 PST
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?
Comment 6 Antonio Gomes 2010-11-14 07:01:03 PST
> 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...
Comment 7 Martin Robinson 2010-11-16 08:42:11 PST
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"])
Comment 8 Antonio Gomes 2010-11-17 09:01:47 PST
Created attachment 74123 [details]
v2.1 - rebase against trunk

There we go, Martin :)
Comment 9 Antonio Gomes 2010-11-20 10:42:27 PST
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.
Comment 10 Martin Robinson 2010-11-20 21:33:55 PST
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!).
Comment 11 Antonio Gomes 2010-12-03 12:36:27 PST
Created attachment 75526 [details]
(committed r73325 - r=mrobinson) patch v2.3 - rebased to ToT

Working patch.
Comment 12 Martin Robinson 2010-12-03 13:03:16 PST
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?
Comment 13 Antonio Gomes 2010-12-03 21:28:20 PST
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>
Comment 14 Antonio Gomes 2010-12-04 07:09:20 PST
Lets keep it opened as a centralized for related patches.
Comment 15 Antonio Gomes 2010-12-04 07:43:25 PST
Created attachment 75609 [details]
(committed r73348, r=mrobinson) patch 2 - v1

Moving more methods.
Comment 16 Antonio Gomes 2010-12-04 07:43:40 PST
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.
Comment 17 Martin Robinson 2010-12-05 10:03:33 PST
Comment on attachment 75609 [details]
(committed r73348, r=mrobinson) patch 2 - v1

Thanks!
Comment 18 Antonio Gomes 2010-12-05 19:06:19 PST
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>
Comment 19 Antonio Gomes 2010-12-08 08:01:48 PST
Created attachment 75905 [details]
(committed r73527 - r=mrobinson) patch 3 - v1

A few more methods moved, and I think I am done here :)
Comment 20 Martin Robinson 2010-12-08 09:15:09 PST
Comment on attachment 75905 [details]
(committed r73527 - r=mrobinson) patch 3 - v1

Awesome.
Comment 21 Antonio Gomes 2010-12-08 09:48:48 PST
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>
Comment 22 Antonio Gomes 2010-12-08 09:50:55 PST
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?
Comment 23 Martin Robinson 2010-12-08 10:08:04 PST
I think this should also move to DRTSupport.
Comment 24 Antonio Gomes 2010-12-09 06:54:29 PST
Created attachment 76057 [details]
patch 4 - v1

As you wish...
Comment 25 Antonio Gomes 2010-12-09 09:48:49 PST
Created attachment 76076 [details]
(committed r73628, r=mrobinson) patch 4 - v2

unbitrotted patch
Comment 26 Martin Robinson 2010-12-09 09:57:54 PST
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 <joone@kldp.org>
> +=======
> + *  Copyright (C) 2009 Google Inc. All rights reserved.
> +>>>>>>> 2010-12-09  Antonio Gomes  <agomes@rim.com>

Please resolve these changes before landing.
Comment 27 Antonio Gomes 2010-12-09 11:00:26 PST
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>