Now all tests fail with assertion on Qt-WK2 in debug mode: ASSERTION FAILED: platformStrategies == s_platformStrategies /home/oszi/WebKit/Source/WebCore/platform/PlatformStrategies.cpp(52) : void WebCore::setPlatformStrategies(WebCore::PlatformStrategies*) 1 0x7f35219e2e2c /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebCore.so.1(WebCore::setPlatformStrategies(WebCore::PlatformStrategies*)+0x56) [0x7f35219e2e2c] 2 0x7f3524a9d0e0 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit1.so.1(PlatformStrategiesQt::PlatformStrategiesQt()+0xf4) [0x7f3524a9d0e0] 3 0x7f3524a9ce4c /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit1.so.1(PlatformStrategiesQt::initialize()+0x44) [0x7f3524a9ce4c] 4 0x7f3524a861a7 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit1.so.1(WebCore::initializeWebCoreQt()+0x36) [0x7f3524a861a7] 5 0x7f3524a5b377 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit1.so.1(QWebSettings::clearMemoryCaches()+0xd) [0x7f3524a5b377] 6 0x7f34cf85570e /home/oszi/WebKit/WebKitBuild/Debug/lib/libWTRInjectedBundle.so(WTR::TestRunner::platformInitialize()+0x14) [0x7f34cf85570e] 7 0x7f34cf84d4c3 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWTRInjectedBundle.so(WTR::TestRunner::TestRunner()+0x15d) [0x7f34cf84d4c3] 8 0x7f34cf84d1e7 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWTRInjectedBundle.so(WTR::TestRunner::create()+0x2b) [0x7f34cf84d1e7] 9 0x7f34cf838152 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWTRInjectedBundle.so(WTR::InjectedBundle::beginTesting(OpaqueWKDictionary const*)+0x60) [0x7f34cf838152] 10 0x7f34cf837b9e /home/oszi/WebKit/WebKitBuild/Debug/lib/libWTRInjectedBundle.so(WTR::InjectedBundle::didReceiveMessage(OpaqueWKString const*, void const*)+0x210) [0x7f34cf837b9e] 11 0x7f34cf8376db /home/oszi/WebKit/WebKitBuild/Debug/lib/libWTRInjectedBundle.so(WTR::InjectedBundle::didReceiveMessage(OpaqueWKBundle const*, OpaqueWKString const*, void const*, void const*)+0x2f) [0x7f34cf8376db] 12 0x7f3524301799 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WebKit::InjectedBundleClient::didReceiveMessage(WebKit::InjectedBundle*, WTF::String const&, WebKit::APIObject*)+0x93) [0x7f3524301799] 13 0x7f35242fd5d4 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WebKit::InjectedBundle::didReceiveMessage(WTF::String const&, WebKit::APIObject*)+0x34) [0x7f35242fd5d4] 14 0x7f35243edd7e /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WebKit::WebProcess::postInjectedBundleMessage(CoreIPC::DataReference const&)+0x11e) [0x7f35243edd7e] 15 0x7f352447b143 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(void CoreIPC::callMemberFunction<WebKit::WebProcess, void (WebKit::WebProcess::*)(CoreIPC::DataReference const&), CoreIPC::DataReference>(CoreIPC::Arguments1<CoreIPC::DataReference> const&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(CoreIPC::DataReference const&))+0x5a) [0x7f352447b143] 16 0x7f352447a805 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(void CoreIPC::handleMessage<Messages::WebProcess::PostInjectedBundleMessage, WebKit::WebProcess, void (WebKit::WebProcess::*)(CoreIPC::DataReference const&)>(CoreIPC::MessageDecoder&, WebKit::WebProcess*, void (WebKit::WebProcess::*)(CoreIPC::DataReference const&))+0x5d) [0x7f352447a805] 17 0x7f3524479900 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WebKit::WebProcess::didReceiveWebProcessMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)+0x18ee) [0x7f3524479900] 18 0x7f35243ec0fe /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WebKit::WebProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)+0x68) [0x7f35243ec0fe] 19 0x7f35243e9f10 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WebKit::WebConnectionToUIProcess::didReceiveMessage(CoreIPC::Connection*, CoreIPC::MessageID, CoreIPC::MessageDecoder&)+0x72) [0x7f35243e9f10] 20 0x7f3524147b80 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(CoreIPC::Connection::dispatchMessage(CoreIPC::MessageID, CoreIPC::MessageDecoder&)+0x42) [0x7f3524147b80] 21 0x7f3524147cbf /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(CoreIPC::Connection::dispatchMessage(CoreIPC::Connection::Message<CoreIPC::MessageDecoder>&)+0x137) [0x7f3524147cbf] 22 0x7f3524147e5b /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(CoreIPC::Connection::dispatchOneMessage()+0xa7) [0x7f3524147e5b] 23 0x7f3524151b45 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>::operator()(CoreIPC::Connection*)+0x59) [0x7f3524151b45] 24 0x7f35241518f6 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WTF::BoundFunctionImpl<WTF::FunctionWrapper<void (CoreIPC::Connection::*)()>, void ()(CoreIPC::Connection*)>::operator()()+0x32) [0x7f35241518f6] 25 0x7f35241f690c /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebKit2.so.1(WTF::Function<void ()()>::operator()() const+0x72) [0x7f35241f690c] 26 0x7f35219afb5b /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebCore.so.1(WebCore::RunLoop::performWork()+0xbf) [0x7f35219afb5b] 27 0x7f3521cf0ec8 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebCore.so.1(WebCore::RunLoop::TimerObject::performWork()+0x1c) [0x7f3521cf0ec8] 28 0x7f3521cf1a15 /home/oszi/WebKit/WebKitBuild/Debug/lib/libWebCore.so.1(+0x2d62a15) [0x7f3521cf1a15] 29 0x7f35168f128e /usr/local/Trolltech/Qt5/Qt-5.0.0-r39/lib/libQtCore.so.5(QObject::event(QEvent*)+0x36e) [0x7f35168f128e] 30 0x7f3517c87d5c /usr/local/Trolltech/Qt5/Qt-5.0.0-r39/lib/libQtWidgets.so.5(QApplicationPrivate::notify_helper(QObject*, QEvent*)+0xac) [0x7f3517c87d5c] 31 0x7f3517c8f61b /usr/local/Trolltech/Qt5/Qt-5.0.0-r39/lib/libQtWidgets.so.5(QApplication::notify(QObject*, QEvent*)+0x11b) [0x7f3517c8f61b]
Created attachment 171614 [details] Patch
Comment on attachment 171614 [details] Patch r-, it doesn't fix the bug.
http://trac.webkit.org/changeset/132744 inverted this assertion.
And it is Qt specific bug, because assertion doesn't hit on GTK and EFL WK2 debug bots.
The assert was inverted because it was wrong, now it is correct. I think the patch is trivially a fix, although it's still possible that we call setPlatformStrategies twice in which case the assert still triggered (although I could not reproduce it locally yet). The fact that it does not asserted on bots yet can be because some compilers init statics by default (?), or it is just 0 "by luck". I'm going to check when and why we can call it twice.
Ok, I found the place. In TestRunner::platformInitialize we call QWebSettings::clearMemoryCaches. This is obviously an error, we should not call WebKit1 API from WebKit2 the web process. The reason why this cache clear is necessary is to clear the font cache between tests, see bug 53427. In order to remove the wk1 api call and keep the same behavior we should add publicly available (private) API, i.e. move the initializeTestFonts code to WebCore. /me: preparing the patch.
You've probably seen the discussion in webkit-dev, I think that clearing caches between tests is not the best approach anyway. I'm only mentioning this for completeness, changing that is not necessary to fix the assertion.
Created attachment 171974 [details] Patch
Comment on attachment 171974 [details] Patch Attachment 171974 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14688114
(In reply to comment #9) > (From update of attachment 171974 [details]) > Attachment 171974 [details] did not pass qt-wk2-ews (qt): > Output: http://queues.webkit.org/results/14688114 It is building finely for me. Is the ews fails to do a qmake_all or a clean build if the incremental build fails?
Comment on attachment 171974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171974&action=review r=me with a few minor glitches that should be fixed before landing :) > Source/WebCore/ChangeLog:16 > + in non production mode. Very good reasoning, I fully agree. There are some dependencies left from WTR to the WK1 API, right? (DumpRenderTreeSupport?) If not, then we can elimiate the dependency mentioned in Tools/Tools.pro to build wtr only if build?(webkit1) is true. > Source/WebCore/Target.pri:4084 > +!production_build { While this will work most of the time, I think the correct condition to use is if(build?(drt)|build?(wtr)) Those will be disabled if !production_build of course. > Source/WebCore/WebCore.pri:289 > +!production_build:have?(FONTCONFIG) PKGCONFIG += fontconfig I think a colon is missing at the end here, at least to be on the safe side :) > Tools/DumpRenderTree/qt/DumpRenderTree.pro:-35 > - QtInitializeTestFonts.h \ I don't see those files as removed in the diff. Is that a bug in the review tool?
(In reply to comment #11) > (From update of attachment 171974 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171974&action=review > > r=me with a few minor glitches that should be fixed before landing :) > > > Source/WebCore/ChangeLog:16 > > + in non production mode. > > Very good reasoning, I fully agree. > > There are some dependencies left from WTR to the WK1 API, right? (DumpRenderTreeSupport?) > > If not, then we can elimiate the dependency mentioned in Tools/Tools.pro to build wtr only if build?(webkit1) is true. Yes, there are dependencies on DumpRenderTreeSupport yet. > > > Source/WebCore/Target.pri:4084 > > +!production_build { > > While this will work most of the time, I think the correct condition to use is > > if(build?(drt)|build?(wtr)) > > Those will be disabled if !production_build of course. > > > Source/WebCore/WebCore.pri:289 > > +!production_build:have?(FONTCONFIG) PKGCONFIG += fontconfig > > I think a colon is missing at the end here, at least to be on the safe side :) > > > Tools/DumpRenderTree/qt/DumpRenderTree.pro:-35 > > - QtInitializeTestFonts.h \ > > I don't see those files as removed in the diff. Is that a bug in the review tool? No, I was forgetting to remove them. Thanks for the review, I will incorporate your comments at landing.
Comment on attachment 171974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171974&action=review > Source/WebCore/platform/PlatformStrategies.cpp:34 > -static PlatformStrategies* s_platformStrategies; > +static PlatformStrategies* s_platformStrategies = 0; This is not needed, please don't land this change.
Committed r133303: <http://trac.webkit.org/changeset/133303>
Comment on attachment 171974 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=171974&action=review >> Source/WebCore/platform/PlatformStrategies.cpp:34 >> +static PlatformStrategies* s_platformStrategies = 0; > > This is not needed, please don't land this change. AFAIK this doesn't make a difference anymore with current GCC/clang, the variable ends up in BSS anyway.
(In reply to comment #15) > (From update of attachment 171974 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=171974&action=review > > >> Source/WebCore/platform/PlatformStrategies.cpp:34 > >> +static PlatformStrategies* s_platformStrategies = 0; > > > > This is not needed, please don't land this change. > > AFAIK this doesn't make a difference anymore with current GCC/clang, the variable ends up in BSS anyway. Sorry, I did not noticed Alexey's comment before landing. Btw it's completely new to me and I think the majority of the code base don't depend on this, but ok. Anyway, the patch fails to build on qt bots fail: make[3]: *** No rule to make target `/ramdisk/qt-linux-release-minimal/build/Tools/DumpRenderTree/qt/QtInitializeTestFonts.cpp', needed by `obj/release/ramdisk/qt-linux-release-minimal/build/Tools/DumpRenderTree/qt/QtInitializeTestFonts.o'. Of course no target since I removed the file and also removed it from the project files. I was thinking that nowadays the bots try a quick incremental build but if it's fails than it reruns qmake, but it seems like it doesn't do the latter. This is quite bad, how can one land something that affects the build system at all? I think a forced clean build would fix that but Ossy is offline now (since this is public holiday here) so I will roll out.
Re-opened since this is blocked by bug 101068
> AFAIK this doesn't make a difference anymore with current GCC/clang, the variable ends up in BSS anyway. Yes, I think so. I was objecting against unneeded code churn and added visual noise, not about inefficiency.
(In reply to comment #16) > Anyway, the patch fails to build on qt bots fail: > > make[3]: *** No rule to make target `/ramdisk/qt-linux-release-minimal/build/Tools/DumpRenderTree/qt/QtInitializeTestFonts.cpp', needed by `obj/release/ramdisk/qt-linux-release-minimal/build/Tools/DumpRenderTree/qt/QtInitializeTestFonts.o'. > > Of course no target since I removed the file and also removed it from the project files. I was thinking that nowadays the bots try a quick incremental build but if it's fails than it reruns qmake, but it seems like it doesn't do the latter. This is quite bad, how can one land something that affects the build system at all? > > I think a forced clean build would fix that but Ossy is offline now (since this is public holiday here) so I will roll out. Let's check the build tomorrow. But I don't think if it is an incremental build issue, because bots run clean build always when a Qt project file changes. Otherwise qmake runs always, you can't avoid running it. (See https://bugs.webkit.org/show_bug.cgi?id=100360 for details.)
> > Let's check the build tomorrow. But I don't think if it is an incremental build issue, because bots run clean build always when a Qt project file changes. Otherwise qmake runs always, you can't avoid running it. (See https://bugs.webkit.org/show_bug.cgi?id=100360 for details.) You are right, it was my fault because I failed to remove the deleted files from some project files. The weird fact is that locally incremental build was working for me because non-updated call sites was using functions from the object files of the removed sources. Now I have a version of the patch that was already build on bots yesterday evening but it broke tests so I rolled out again.
Landed in http://trac.webkit.org/changeset/133421.