RESOLVED FIXED 100838
[Qt][WK2] setPlatformStrategies always asserts after r132744 
https://bugs.webkit.org/show_bug.cgi?id=100838
Summary [Qt][WK2] setPlatformStrategies always asserts after r132744
Csaba Osztrogonác
Reported 2012-10-31 03:06:13 PDT
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]
Attachments
Patch (1.32 KB, patch)
2012-10-31 04:12 PDT, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Patch (20.38 KB, patch)
2012-11-01 19:28 PDT, Balazs Kelemen 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Balazs Kelemen
Comment 1 2012-10-31 04:12:35 PDT
Created attachment 171614 [details] Patch
Csaba Osztrogonác
Comment 2 2012-10-31 04:25:44 PDT
Comment on attachment 171614 [details] Patch r-, it doesn't fix the bug.
Csaba Osztrogonác
Comment 3 2012-10-31 04:37:46 PDT
http://trac.webkit.org/changeset/132744 inverted this assertion.
Csaba Osztrogonác
Comment 4 2012-10-31 04:44:57 PDT
And it is Qt specific bug, because assertion doesn't hit on GTK and EFL WK2 debug bots.
Balazs Kelemen
Comment 5 2012-10-31 05:31:25 PDT
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.
Balazs Kelemen
Comment 6 2012-10-31 06:23:44 PDT
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.
Alexey Proskuryakov
Comment 7 2012-10-31 08:42:29 PDT
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.
Balazs Kelemen
Comment 8 2012-11-01 19:28:01 PDT
Created attachment 171974 [details] Patch
Early Warning System Bot
Comment 9 2012-11-01 19:42:10 PDT
Comment on attachment 171974 [details] Patch Attachment 171974 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/14688114
Balazs Kelemen
Comment 10 2012-11-01 19:47:59 PDT
(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?
Simon Hausmann
Comment 11 2012-11-02 02:15:59 PDT
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?
Balazs Kelemen
Comment 12 2012-11-02 05:51:52 PDT
(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.
Alexey Proskuryakov
Comment 13 2012-11-02 08:45:42 PDT
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.
Balazs Kelemen
Comment 14 2012-11-02 09:02:40 PDT
Committed r133303: <http://trac.webkit.org/changeset/133303>
Simon Hausmann
Comment 15 2012-11-02 09:07:11 PDT
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.
Balazs Kelemen
Comment 16 2012-11-02 09:37:34 PDT
(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.
WebKit Review Bot
Comment 17 2012-11-02 09:40:15 PDT
Re-opened since this is blocked by bug 101068
Alexey Proskuryakov
Comment 18 2012-11-02 09:44:31 PDT
> 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.
Csaba Osztrogonác
Comment 19 2012-11-04 00:50:46 PDT
(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.)
Balazs Kelemen
Comment 20 2012-11-04 05:42:39 PST
> > 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.
Balazs Kelemen
Comment 21 2012-11-04 08:50:15 PST
Landed in http://trac.webkit.org/changeset/133421.
Note You need to log in before you can comment on or make changes to this bug.