Fix bug in the implementation of synchronous network jobs.
authorhausmann <hausmann@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Nov 2007 08:20:27 +0000 (08:20 +0000)
committerhausmann <hausmann@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 8 Nov 2007 08:20:27 +0000 (08:20 +0000)
* George (pmax) reviewed the networking patches and found the following bug (thanks for reviewing)
-    if (jobMode == AsynchronousJob) {
+    if (jobMode == SynchronousJob) {
        add job to synchronous list/hash

* Just applying the above change will lead to crashes because we can finish
  jobs before we started them.

* Avoid these issues by saving all work (starting a job, sending data and
  finishing it) inside one list. JobWork will contain any
  of the above three work types and doWork will just work on this list
  (m_pendingWork). As foreach takes a copy of the list calling started, data
  and finished will not add new work and we gurantee that if we have JobStarted
  it will be in the list before JobData and JobFinished.

* Observation: We might just kill the code to handle sync jobs.

Signed-off-by: Lars Knoll <lars@trolltech.com>
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@27592 268f45cc-cd09-0410-ab3c-d52691b4dbfc

WebKit/qt/Api/qwebnetworkinterface.cpp
WebKit/qt/Api/qwebnetworkinterface_p.h
WebKit/qt/ChangeLog

index 8b65748e7244ddc6998c9fda521be2f7231eee7c..cf5ebe0a2471ef32c326173883bd75a473d2404f 100644 (file)
@@ -462,7 +462,7 @@ bool QWebNetworkManager::add(ResourceHandle *handle, QWebNetworkInterface *inter
 
     DEBUG() << "QWebNetworkManager::add:" <<  job->d->request.httpHeader.toString();
 
-    if (jobMode == AsynchronousJob) {
+    if (jobMode == SynchronousJob) {
         Q_ASSERT(!m_synchronousJobs.contains(job));
         m_synchronousJobs[job] = 1;
     }
@@ -756,7 +756,7 @@ void QWebNetworkManager::queueStart(QWebNetworkJob* job)
 
     QMutexLocker locker(&m_queueMutex);
     job->ref();
-    m_startedJobs.append(job);
+    m_pendingWork.append(new JobWork(job));
     doScheduleWork();
 }
 
@@ -766,17 +766,17 @@ void QWebNetworkManager::queueData(QWebNetworkJob* job, const QByteArray& data)
 
     QMutexLocker locker(&m_queueMutex);
     job->ref();
-    m_receivedData.append(new JobData(job,data));
+    m_pendingWork.append(new JobWork(job, data));
     doScheduleWork();
 }
 
 void QWebNetworkManager::queueFinished(QWebNetworkJob* job, int errorCode)
 {
     Q_ASSERT(job->d);
-    
+
     QMutexLocker locker(&m_queueMutex);
     job->ref();
-    m_finishedJobs.append(new JobFinished(job, errorCode));
+    m_pendingWork.append(new JobWork(job, errorCode));
     doScheduleWork();
 }
 
@@ -790,9 +790,9 @@ void QWebNetworkManager::doScheduleWork()
 
 
 /*
- * WARNING: This is very fragile. doWork will dispatch jobs which then
- * can create new jobs in the meantime, which can fill the queue while
- * we are working on it.
+ * We will work on a copy of m_pendingWork. While dispatching m_pendingWork
+ * new work will be added that will be handled at a later doWork call. doWork
+ * will be called we set m_scheduledWork to false early in this method.
  */
 void QWebNetworkManager::doWork()
 {
@@ -802,51 +802,33 @@ void QWebNetworkManager::doWork()
     const QHash<QWebNetworkJob*, int> syncJobs = m_synchronousJobs;
     m_queueMutex.unlock();
 
-    foreach(QWebNetworkJob* job, m_startedJobs) {
-        if (hasSyncJobs && !syncJobs.contains(job))
-            continue;
-
-        m_queueMutex.lock();
-        m_startedJobs.removeAll(job);
-        m_queueMutex.unlock();
-
-        started(job);
-        job->deref();
-    }
-
-    foreach(JobData* jobData, m_receivedData) {
-        if (hasSyncJobs && !syncJobs.contains(jobData->job))
+    foreach (JobWork* work, m_pendingWork) {
+        if (hasSyncJobs && !syncJobs.contains(work->job))
             continue;
 
-        // This job was not yet started
-        if (static_cast<int>(jobData->job->status()) < QWebNetworkJob::JobStarted)
-            continue;
-
-        m_queueMutex.lock();
-        m_receivedData.removeAll(jobData);
-        m_queueMutex.unlock();
-        
-        data(jobData->job, jobData->data);
-        jobData->job->deref();
-        delete jobData;
-    }
-
-    foreach(JobFinished* jobFinished, m_finishedJobs) {
-        if (hasSyncJobs && !syncJobs.contains(jobFinished->job))
-            continue;
-
-        // This job was not yet started... we have no idea if data comes by...
-        // and it won't start in case of errors
-        if (static_cast<int>(jobFinished->job->status()) < QWebNetworkJob::JobStarted && jobFinished->errorCode != 1)
-            continue;
+        if (work->workType == JobWork::JobStarted)
+            started(work->job);
+        else if (work->workType == JobWork::JobData) {
+            // This job was not yet started
+            if (static_cast<int>(work->job->status()) < QWebNetworkJob::JobStarted)
+                continue;
+
+            data(work->job, work->data);
+        } else if (work->workType == JobWork::JobFinished) {
+            // This job was not yet started... we have no idea if data comes by...
+            // and it won't start in case of errors
+            if (static_cast<int>(work->job->status()) < QWebNetworkJob::JobStarted && work->errorCode != 1)
+                continue;
+
+            finished(work->job, work->errorCode);
+        }
 
         m_queueMutex.lock();
-        m_finishedJobs.removeAll(jobFinished);
+        m_pendingWork.removeAll(work);
         m_queueMutex.unlock();
 
-        finished(jobFinished->job, jobFinished->errorCode);
-        jobFinished->job->deref();
-        delete jobFinished;
+        work->job->deref();
+        delete work;
     }
 
     m_queueMutex.lock();
index 41f5ab5673ce4335af888ef641c41a8e371b70f2..1e595200afd4c9dd379b5a218ceaedd6cca42cec 100644 (file)
@@ -114,33 +114,41 @@ private:
     QWebNetworkManager();
     QHash<WebCore::HostInfo, WebCore::WebCoreHttp *> m_hostMapping;
 
-    struct JobData {
-        JobData(QWebNetworkJob* _job, const QByteArray& _data)
-            : job(_job)
-            , data(_data)
-        {}
+    struct JobWork {
+        enum WorkType {
+            JobStarted,
+            JobData,
+            JobFinished
+        };
 
-        QWebNetworkJob* job;
-        QByteArray data;
-    };
+        explicit JobWork(QWebNetworkJob* _job)
+            : workType(JobStarted)
+            , errorCode(-1)
+            , job(_job)
+        {}
 
-    struct JobFinished {
-        JobFinished(QWebNetworkJob* _job, int _errorCode)
-            : job(_job)
+        explicit JobWork(QWebNetworkJob* _job, int _errorCode)
+            : workType(JobFinished)
             , errorCode(_errorCode)
+            , job(_job)
         {}
 
-        QWebNetworkJob* job;
+        explicit JobWork(QWebNetworkJob* _job, const QByteArray& _data)
+            : workType(JobData)
+            , errorCode(-1)
+            , job(_job)
+            , data(_data)
+        {}
+
+        const WorkType workType;
         int errorCode;
+        QByteArray data;
+        QWebNetworkJob* job;
     };
 
-
-
     QMutex m_queueMutex;
     bool m_scheduledWork;
-    QList<QWebNetworkJob*> m_startedJobs;
-    QList<JobData*> m_receivedData;
-    QList<JobFinished*> m_finishedJobs;
+    QList<JobWork*> m_pendingWork;
     QHash<QWebNetworkJob*, int> m_synchronousJobs;
 };
 
index dc91f8d6fcd20e12c20c262bf5dafdd5bb882d11..a93edfc5e493913b521ce7b8196337d3178c763d 100644 (file)
@@ -1,3 +1,37 @@
+2007-11-08  Holger Hans Peter Freyther  <holger.freyther@trolltech.com>
+
+        Reviewed by Lars Knoll <lars@trolltech.com>.
+
+        Fix bug in the implementation of synchronous network jobs.
+        
+        * George (pmax) reviewed the networking patches and found the following bug (thanks for reviewing)
+        -    if (jobMode == AsynchronousJob) {
+        +    if (jobMode == SynchronousJob) {
+        add job to synchronous list/hash
+        
+        * Just applying the above change will lead to crashes because we can finish
+        jobs before we started them.
+        
+        * Avoid these issues by saving all work (starting a job, sending data and
+        finishing it) inside one list. JobWork will contain any
+        of the above three work types and doWork will just work on this list
+        (m_pendingWork). As foreach takes a copy of the list calling started, data
+        and finished will not add new work and we gurantee that if we have JobStarted
+        it will be in the list before JobData and JobFinished.
+        
+        * Observation: We might just kill the code to handle sync jobs.
+        
+
+        * Api/qwebnetworkinterface.cpp:
+        (QWebNetworkManager::add):
+        (QWebNetworkManager::queueStart):
+        (QWebNetworkManager::queueData):
+        (QWebNetworkManager::queueFinished):
+        (QWebNetworkManager::doWork):
+        * Api/qwebnetworkinterface_p.h:
+        (QWebNetworkManager::JobWork::):
+        (QWebNetworkManager::JobWork::JobWork):
+
 2007-11-07  Simon Hausmann  <hausmann@kde.org>
 
         Build fix, reviewed by nobody.