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.
Created attachment 73245 [details] part I - moved webkit_web_frame* method in webkitprivat.h to DRTSupportGtk.
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.
Created attachment 73606 [details] patch v2 Fixed Martin's requests.
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?
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?
> 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...
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"])
Created attachment 74123 [details] v2.1 - rebase against trunk There we go, Martin :)
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.
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!).
Created attachment 75526 [details] (committed r73325 - r=mrobinson) patch v2.3 - rebased to ToT Working patch.
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 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>
Lets keep it opened as a centralized for related patches.
Created attachment 75609 [details] (committed r73348, r=mrobinson) patch 2 - v1 Moving more methods.
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 on attachment 75609 [details] (committed r73348, r=mrobinson) patch 2 - v1 Thanks!
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>
Created attachment 75905 [details] (committed r73527 - r=mrobinson) patch 3 - v1 A few more methods moved, and I think I am done here :)
Comment on attachment 75905 [details] (committed r73527 - r=mrobinson) patch 3 - v1 Awesome.
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>
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?
I think this should also move to DRTSupport.
Created attachment 76057 [details] patch 4 - v1 As you wish...
Created attachment 76076 [details] (committed r73628, r=mrobinson) patch 4 - v2 unbitrotted patch
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 on attachment 76076 [details] (committed r73628, r=mrobinson) patch 4 - v2 Clearing flags on attachment: 76076 Committed r73628: <http://trac.webkit.org/changeset/73628>