Bug 100838

Summary:

[Qt][WK2] setPlatformStrategies always asserts after r132744

Product:

WebKit

Reporter:

Csaba Osztrogonác <ossy>

Component:

New Bugs

Assignee:

Balazs Kelemen <kbalazs>

Status:

RESOLVED FIXED

Severity:

Critical

CC:

abecsi, ap, hausmann, kbalazs, noam, ossy, sam, vestbo, webkit.review.bot, zarvai

Priority:

Keywords:

Qt, QtTriaged

Version:

420+

Hardware:

Unspecified

OS:

All

Bug Depends on:

101068, 101155, 101158

Bug Blocks:

79668

Attachments:

Description	Flags
Patch	none
Patch	none

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	Details Formatted Diff Diff
Patch (20.38 KB, patch) 2012-11-01 19:28 PDT, Balazs Kelemen	no flags	Details Formatted Diff Diff
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.