From 0d284ae2d62707e6c35c8d97ca73e974d35c4167 Mon Sep 17 00:00:00 2001 From: Kai Koehne Date: Tue, 24 Apr 2012 14:39:38 +0200 Subject: [PATCH] Debugger: Fix race conditions in block mode Using waitForMessage() in the constructor after registerService() is _not_ safe: You might get the first message already after the registerService() and before the waitForMessage() call. Instead, use QMutex/QWaitCondition to block the initialization. Also make the use of the block mode explicit, since the service might already be enabled also for non-blocking modes ... Change-Id: I387bfe0627c80e2029acff71f86d12cd9ab58de1 Reviewed-by: Kai Koehne --- src/qml/debugger/qdebugmessageservice.cpp | 6 ++++- src/qml/debugger/qqmldebugserver.cpp | 33 +++++++++---------------- src/qml/debugger/qqmldebugserver_p.h | 2 +- src/qml/debugger/qqmldebugservice.cpp | 16 +++++-------- src/qml/debugger/qqmldebugservice_p.h | 2 +- src/qml/debugger/qqmlprofilerservice.cpp | 32 +++++++++++++++++-------- src/qml/debugger/qqmlprofilerservice_p.h | 6 +++-- src/qml/debugger/qv8debugservice.cpp | 40 +++++++++++++++---------------- src/qml/debugger/qv8profilerservice.cpp | 23 ++++++++++++++---- 9 files changed, 89 insertions(+), 71 deletions(-) diff --git a/src/qml/debugger/qdebugmessageservice.cpp b/src/qml/debugger/qdebugmessageservice.cpp index a41f95c..bcd7985 100644 --- a/src/qml/debugger/qdebugmessageservice.cpp +++ b/src/qml/debugger/qdebugmessageservice.cpp @@ -43,6 +43,7 @@ #include "qqmldebugservice_p_p.h" #include +#include QT_BEGIN_NAMESPACE @@ -65,6 +66,7 @@ public: QMessageHandler oldMsgHandler; QQmlDebugService::State prevState; + QMutex initMutex; }; QDebugMessageService::QDebugMessageService(QObject *parent) : @@ -73,6 +75,8 @@ QDebugMessageService::QDebugMessageService(QObject *parent) : { Q_D(QDebugMessageService); + // don't execute stateChanged() in parallel + QMutexLocker lock(&d->initMutex); registerService(); if (state() == Enabled) { d->oldMsgHandler = qInstallMessageHandler(DebugMessageHandler); @@ -108,6 +112,7 @@ void QDebugMessageService::sendDebugMessage(QtMsgType type, void QDebugMessageService::stateChanged(State state) { Q_D(QDebugMessageService); + QMutexLocker lock(&d->initMutex); if (state != Enabled && d->prevState == Enabled) { QMessageHandler handler = qInstallMessageHandler(d->oldMsgHandler); @@ -117,7 +122,6 @@ void QDebugMessageService::stateChanged(State state) } else if (state == Enabled && d->prevState != Enabled) { d->oldMsgHandler = qInstallMessageHandler(DebugMessageHandler); - } d->prevState = state; diff --git a/src/qml/debugger/qqmldebugserver.cpp b/src/qml/debugger/qqmldebugserver.cpp index d49d04f..b247e43 100644 --- a/src/qml/debugger/qqmldebugserver.cpp +++ b/src/qml/debugger/qqmldebugserver.cpp @@ -99,6 +99,7 @@ public: mutable QReadWriteLock pluginsLock; QStringList clientPlugins; bool gotHello; + bool blockingMode; QMutex messageArrivedMutex; QWaitCondition messageArrivedCondition; @@ -139,6 +140,7 @@ private: QQmlDebugServerPrivate::QQmlDebugServerPrivate() : connection(0), gotHello(false), + blockingMode(false), thread(0) { // used in _q_sendMessages @@ -244,6 +246,12 @@ bool QQmlDebugServer::hasDebuggingClient() const && d->gotHello; } +bool QQmlDebugServer::blockingMode() const +{ + Q_D(const QQmlDebugServer); + return d->blockingMode; +} + static QQmlDebugServer *qQmlDebugServer = 0; @@ -305,10 +313,12 @@ QQmlDebugServer *QQmlDebugServer::instance() thread->setPort(port, block, hostAddress); QQmlDebugServerPrivate *d = qQmlDebugServer->d_func(); + d->blockingMode = block; + QMutexLocker locker(&d->messageArrivedMutex); thread->start(); - if (block) + if (d->blockingMode) d->messageArrivedCondition.wait(&d->messageArrivedMutex); } else { @@ -576,27 +586,6 @@ void QQmlDebugServer::sendMessages(QQmlDebugService *service, Q_ARG(QList, prefixedMessages)); } -bool QQmlDebugServer::waitForMessage(QQmlDebugService *service) -{ - // to be executed in GUI thread - Q_ASSERT(QThread::currentThread() == QCoreApplication::instance()->thread()); - - Q_D(QQmlDebugServer); - QReadLocker lock(&d->pluginsLock); - - if (!service - || !d->plugins.contains(service->name())) - return false; - - d->messageArrivedMutex.lock(); - d->waitingForMessageNames << service->name(); - do { - d->messageArrivedCondition.wait(&d->messageArrivedMutex); - } while (d->waitingForMessageNames.contains(service->name())); - d->messageArrivedMutex.unlock(); - return true; -} - QT_END_NAMESPACE #include "moc_qqmldebugserver_p.cpp" diff --git a/src/qml/debugger/qqmldebugserver_p.h b/src/qml/debugger/qqmldebugserver_p.h index 5de7e73..5e853da 100644 --- a/src/qml/debugger/qqmldebugserver_p.h +++ b/src/qml/debugger/qqmldebugserver_p.h @@ -76,6 +76,7 @@ public: void setConnection(QQmlDebugServerConnection *connection); bool hasDebuggingClient() const; + bool blockingMode() const; QList services() const; QStringList serviceNames() const; @@ -86,7 +87,6 @@ public: void receiveMessage(const QByteArray &message); - bool waitForMessage(QQmlDebugService *service); void sendMessages(QQmlDebugService *service, const QList &messages); private: diff --git a/src/qml/debugger/qqmldebugservice.cpp b/src/qml/debugger/qqmldebugservice.cpp index 4594849..1fe5aa3 100644 --- a/src/qml/debugger/qqmldebugservice.cpp +++ b/src/qml/debugger/qqmldebugservice.cpp @@ -213,6 +213,12 @@ bool QQmlDebugService::hasDebuggingClient() && QQmlDebugServer::instance()->hasDebuggingClient(); } +bool QQmlDebugService::blockingMode() +{ + return QQmlDebugServer::instance() != 0 + && QQmlDebugServer::instance()->blockingMode(); +} + QString QQmlDebugService::objectToString(QObject *obj) { if(!obj) @@ -243,16 +249,6 @@ void QQmlDebugService::sendMessages(const QList &messages) d->server->sendMessages(this, messages); } -bool QQmlDebugService::waitForMessage() -{ - Q_D(QQmlDebugService); - - if (state() != Enabled) - return false; - - return d->server->waitForMessage(this); -} - void QQmlDebugService::stateAboutToBeChanged(State) { } diff --git a/src/qml/debugger/qqmldebugservice_p.h b/src/qml/debugger/qqmldebugservice_p.h index f19b64f..abd9c1b 100644 --- a/src/qml/debugger/qqmldebugservice_p.h +++ b/src/qml/debugger/qqmldebugservice_p.h @@ -81,7 +81,6 @@ public: void sendMessage(const QByteArray &); void sendMessages(const QList &); - bool waitForMessage(); static int idForObject(QObject *); static QObject *objectForId(int); @@ -90,6 +89,7 @@ public: static bool isDebuggingEnabled(); static bool hasDebuggingClient(); + static bool blockingMode(); protected: QQmlDebugService(QQmlDebugServicePrivate &dd, const QString &name, float version, QObject *parent = 0); diff --git a/src/qml/debugger/qqmlprofilerservice.cpp b/src/qml/debugger/qqmlprofilerservice.cpp index 7109286..12db9e1 100644 --- a/src/qml/debugger/qqmlprofilerservice.cpp +++ b/src/qml/debugger/qqmlprofilerservice.cpp @@ -78,16 +78,17 @@ QByteArray QQmlProfilerData::toByteArray() const QQmlProfilerService::QQmlProfilerService() : QQmlDebugService(QStringLiteral("CanvasFrameRate"), 1), - m_enabled(false), m_messageReceived(false) + m_enabled(false) { m_timer.start(); - if (registerService() == Enabled) { - // wait for first message indicating whether to trace or not - while (!m_messageReceived) - waitForMessage(); + // don't execute stateAboutToBeChanged(), messageReceived() in parallel + QMutexLocker lock(&m_initializeMutex); - QUnifiedTimer::instance()->registerProfilerCallback( &animationFrame ); + if (registerService() == Enabled) { + QUnifiedTimer::instance()->registerProfilerCallback(&animationFrame); + if (blockingMode()) + m_initializeCondition.wait(&m_initializeMutex); } } @@ -248,7 +249,7 @@ void QQmlProfilerService::animationFrameImpl(qint64 delta) */ void QQmlProfilerService::processMessage(const QQmlProfilerData &message) { - QMutexLocker locker(&m_mutex); + QMutexLocker locker(&m_dataMutex); m_data.append(message); } @@ -267,7 +268,7 @@ void QQmlProfilerService::setProfilingEnabled(bool enable) */ void QQmlProfilerService::sendMessages() { - QMutexLocker locker(&m_mutex); + QMutexLocker locker(&m_dataMutex); QList messages; for (int i = 0; i < m_data.count(); ++i) messages << m_data.at(i).toByteArray(); @@ -284,6 +285,8 @@ void QQmlProfilerService::sendMessages() void QQmlProfilerService::stateAboutToBeChanged(QQmlDebugService::State newState) { + QMutexLocker lock(&m_initializeMutex); + if (state() == newState) return; @@ -292,24 +295,33 @@ void QQmlProfilerService::stateAboutToBeChanged(QQmlDebugService::State newState stopProfilingImpl(); sendMessages(); } + + if (state() != Enabled) { + // wake up constructor in blocking mode + // (we might got disabled before first message arrived) + m_initializeCondition.wakeAll(); + } } void QQmlProfilerService::messageReceived(const QByteArray &message) { + QMutexLocker lock(&m_initializeMutex); + QByteArray rwData = message; QDataStream stream(&rwData, QIODevice::ReadOnly); bool enabled; stream >> enabled; - m_messageReceived = true; - if (enabled) { startProfilingImpl(); } else { if (stopProfilingImpl()) sendMessages(); } + + // wake up constructor in blocking mode + m_initializeCondition.wakeAll(); } QT_END_NAMESPACE diff --git a/src/qml/debugger/qqmlprofilerservice_p.h b/src/qml/debugger/qqmlprofilerservice_p.h index 208fde5..eeaf871 100644 --- a/src/qml/debugger/qqmlprofilerservice_p.h +++ b/src/qml/debugger/qqmlprofilerservice_p.h @@ -61,6 +61,7 @@ #include #include #include +#include QT_BEGIN_HEADER @@ -164,9 +165,10 @@ private: private: QElapsedTimer m_timer; bool m_enabled; - bool m_messageReceived; QVector m_data; - QMutex m_mutex; + QMutex m_dataMutex; + QMutex m_initializeMutex; + QWaitCondition m_initializeCondition; static QQmlProfilerService *instance; diff --git a/src/qml/debugger/qv8debugservice.cpp b/src/qml/debugger/qv8debugservice.cpp index f8831db..3ef1f32 100644 --- a/src/qml/debugger/qv8debugservice.cpp +++ b/src/qml/debugger/qv8debugservice.cpp @@ -48,6 +48,7 @@ #include #include #include +#include //V8 DEBUG SERVICE PROTOCOL //
@@ -106,8 +107,7 @@ class QV8DebugServicePrivate : public QQmlDebugServicePrivate { public: QV8DebugServicePrivate() - : connectReceived(false) - , engine(0) + : engine(0) { } @@ -115,8 +115,8 @@ public: static QByteArray packMessage(const QString &type, const QString &message = QString()); - bool connectReceived; QMutex initializeMutex; + QWaitCondition initializeCondition; QStringList breakOnSignals; const QV8Engine *engine; }; @@ -127,16 +127,13 @@ QV8DebugService::QV8DebugService(QObject *parent) { Q_D(QV8DebugService); v8ServiceInstancePtr = this; - // wait for stateChanged() -> initialize() - d->initializeMutex.lock(); + // don't execute stateChanged, messageReceived in parallel + QMutexLocker lock(&d->initializeMutex); + if (registerService() == Enabled) { init(); - // ,block mode, client attached - while (!d->connectReceived) { - waitForMessage(); - } - } else { - d->initializeMutex.unlock(); + if (blockingMode()) + d->initializeCondition.wait(&d->initializeMutex); } } @@ -189,11 +186,9 @@ void QV8DebugService::signalEmitted(const QString &signal) // executed in the gui thread void QV8DebugService::init() { - Q_D(QV8DebugService); v8::Debug::SetMessageHandler2(DebugMessageHandler); v8::Debug::SetDebugMessageDispatchHandler(DebugMessageDispatchHandler); QV4Compiler::enableV4(false); - d->initializeMutex.unlock(); } // executed in the gui thread @@ -209,10 +204,16 @@ void QV8DebugService::scheduledDebugBreak(bool schedule) void QV8DebugService::stateChanged(QQmlDebugService::State newState) { Q_D(QV8DebugService); + QMutexLocker lock(&d->initializeMutex); + if (newState == Enabled) { - // execute in GUI thread - d->initializeMutex.lock(); - QMetaObject::invokeMethod(this, "init", Qt::QueuedConnection); + // execute in GUI thread, bock to make sure messageReceived isn't called + // before it finished. + QMetaObject::invokeMethod(this, "init", Qt::BlockingQueuedConnection); + } else { + // wake up constructor in blocking mode + // (we might got disabled before first message arrived) + d->initializeCondition.wakeAll(); } } @@ -220,6 +221,7 @@ void QV8DebugService::stateChanged(QQmlDebugService::State newState) void QV8DebugService::messageReceived(const QByteArray &message) { Q_D(QV8DebugService); + QMutexLocker lock(&d->initializeMutex); QDataStream ds(message); QByteArray header; @@ -231,10 +233,9 @@ void QV8DebugService::messageReceived(const QByteArray &message) ds >> command >> data; if (command == V8_DEBUGGER_KEY_CONNECT) { - QMutexLocker locker(&d->initializeMutex); - d->connectReceived = true; sendMessage(QV8DebugServicePrivate::packMessage(QLatin1String(V8_DEBUGGER_KEY_CONNECT))); - + // wake up constructor in blocking mode + d->initializeCondition.wakeAll(); } else if (command == V8_DEBUGGER_KEY_INTERRUPT) { // break has to be executed in gui thread QMetaObject::invokeMethod(this, "scheduledDebugBreak", Qt::QueuedConnection, Q_ARG(bool, true)); @@ -260,7 +261,6 @@ void QV8DebugService::messageReceived(const QByteArray &message) else d->breakOnSignals.removeOne(signalName); sendMessage(QV8DebugServicePrivate::packMessage(QLatin1String(V8_DEBUGGER_KEY_BREAK_ON_SIGNAL))); - } } } diff --git a/src/qml/debugger/qv8profilerservice.cpp b/src/qml/debugger/qv8profilerservice.cpp index 1cd7606..5cddea7 100644 --- a/src/qml/debugger/qv8profilerservice.cpp +++ b/src/qml/debugger/qv8profilerservice.cpp @@ -45,6 +45,8 @@ #include #include +#include +#include QT_BEGIN_NAMESPACE @@ -96,6 +98,8 @@ public: QList m_data; bool initialized; + QMutex initializeMutex; + QWaitCondition initializeCondition; QList m_ongoing; }; @@ -104,10 +108,12 @@ QV8ProfilerService::QV8ProfilerService(QObject *parent) { Q_D(QV8ProfilerService); - if (registerService() == Enabled) { - // ,block mode, client attached - while (!d->initialized) - waitForMessage(); + QMutexLocker lock(&d->initializeMutex); + + if (registerService() == Enabled + && QQmlDebugService::blockingMode()) { + // let's wait for first message ... + d->initializeCondition.wait(&d->initializeMutex); } } @@ -139,6 +145,10 @@ void QV8ProfilerService::stateAboutToBeChanged(QQmlDebugService::State newState) Q_ARG(QString, title)); } QMetaObject::invokeMethod(this, "sendProfilingData", Qt::BlockingQueuedConnection); + } else { + // wake up constructor in blocking mode + // (we might got disabled before first message arrived) + d->initializeCondition.wakeAll(); } } @@ -152,6 +162,8 @@ void QV8ProfilerService::messageReceived(const QByteArray &message) QByteArray title; ds >> command >> option; + QMutexLocker lock(&d->initializeMutex); + if (command == "V8PROFILER") { ds >> title; QString titleStr = QString::fromUtf8(title); @@ -172,6 +184,9 @@ void QV8ProfilerService::messageReceived(const QByteArray &message) } } + // wake up constructor in blocking mode + d->initializeCondition.wakeAll(); + QQmlDebugService::messageReceived(message); } -- 2.7.4