[Gnash-commit] gnash ChangeLog libbase/LoadThread.cpp

[Sandro Santilli]

CVSROOT:        /sources/gnash
Module name:    gnash
Changes by:     Sandro Santilli <strk>  08/04/28 10:35:04

Modified files:
        .              : ChangeLog 
        libbase        : LoadThread.cpp 

Log message:
        don't trust advertised streamSize for detecting the "completed" state.
        Fixes bug #23076.

CVSWeb URLs:
http://cvs.savannah.gnu.org/viewcvs/gnash/ChangeLog?cvsroot=gnash&r1=1.6429&r2=1.6430
http://cvs.savannah.gnu.org/viewcvs/gnash/libbase/LoadThread.cpp?cvsroot=gnash&r1=1.21&r2=1.22

Patches:
Index: ChangeLog
===================================================================
RCS file: /sources/gnash/gnash/ChangeLog,v
retrieving revision 1.6429
retrieving revision 1.6430
diff -u -b -r1.6429 -r1.6430
--- ChangeLog   28 Apr 2008 09:40:20 -0000      1.6429
+++ ChangeLog   28 Apr 2008 10:35:03 -0000      1.6430
@@ -1,3 +1,8 @@
+2008-04-28 Sandro Santilli <address@hidden>
+
+       * libbase/LoadThread.cpp: don't trust advertised streamSize
+         for detecting the "completed" state. Fixes bug #23076.
+
 2008-04-28 Benjamin Wolsey <address@hidden>
 
        * server/edit_text_character.{h,cpp}: make parseHTML a const

Index: libbase/LoadThread.cpp
===================================================================
RCS file: /sources/gnash/gnash/libbase/LoadThread.cpp,v
retrieving revision 1.21
retrieving revision 1.22
diff -u -b -r1.21 -r1.22
--- libbase/LoadThread.cpp      5 Mar 2008 03:55:52 -0000       1.21
+++ libbase/LoadThread.cpp      28 Apr 2008 10:35:04 -0000      1.22
@@ -27,6 +27,8 @@
 # include "unistd.h" // for usleep()
 #endif
 
+using namespace gnash;
+
 LoadThread::LoadThread()
        :
        _completed(false),
@@ -270,6 +272,7 @@
 
 long LoadThread::getBytesTotal()
 {
+       // TODO: proxy to underlying stream instead ?
        return _streamSize;
 }
 
@@ -297,6 +300,9 @@
 
        if ( ret < setupSize )
        {
+#ifdef GNASH_DEBUG_LOAD_THREAD
+               log_debug("LoadThread completed during setupCache");
+#endif
                _completed = true;
                if ( _streamSize < _loadPosition ) _streamSize = _loadPosition;
        }
@@ -321,19 +327,14 @@
 
 void LoadThread::fillCache()
 {
-       // TODO: is this needed ? Should it be an assertion ?
-       if (_loadPosition >= _streamSize) {
-               _completed = true;
-               _streamSize = _loadPosition;
-               // I don't see how should this happen...
-               gnash::log_error("LoadThread::fillCache: _loadPosition:%ld, 
_streamSize:%ld", _loadPosition, _streamSize);
-               return;
-       }
-
 #ifdef THREADED_LOADS
        boost::mutex::scoped_lock lock(_mutex);
 #endif
 
+
+       // don't call me if download was completed
+       assert(!_completed);
+
        // If we're not at the reading head, move to it
        if (_loadPosition != _actualPosition) 
_stream->set_position(_loadPosition);
 
@@ -345,11 +346,19 @@
 
                _cachedData += ret;
                if (ret != _cacheSize - _cachedData) {
+#ifdef GNASH_DEBUG_LOAD_THREAD
+                       log_debug("LoadThread completed during fillCache (read 
%d bytes when %d were requested)",
+                               ret, _cacheSize-_cachedData);
+#endif
                        _completed = true;
                } else {
                        _stream->set_position(_loadPosition + _chunkSize);
                        long pos = _stream->get_position();
                        if (pos != _loadPosition + _chunkSize) {
+#ifdef GNASH_DEBUG_LOAD_THREAD
+                               log_debug("LoadThread completed during 
fillCache (attempted to go to position %d, but only got to %d",
+                                       _loadPosition+_chunkSize, pos);
+#endif
                                _completed = true;
                        }
                        ret += pos - (_loadPosition + _chunkSize);
@@ -358,6 +367,10 @@
        } else {
                ret = _stream->read_bytes(_cache.get() + _cachedData, 
_chunkSize);
                if (ret != _chunkSize) {
+#ifdef GNASH_DEBUG_LOAD_THREAD
+                       log_debug("LoadThread completed during fillCache (tried 
to read %d bytes, but read %d)",
+                               _chunkSize, ret);
+#endif
                        _completed = true;
                }
                _cachedData += ret;
@@ -372,28 +385,31 @@
 
 void LoadThread::download()
 {
-       // TODO: is this needed ? Should it be an assertion ?
-       if (_loadPosition >= _streamSize) {
-               _loadPosition = _streamSize;
-               _completed = true;
-               _streamSize = _loadPosition;
-               gnash::log_error("LoadThread::download: _loadPosition:%ld, 
_streamSize:%ld", _loadPosition, _streamSize);
-               return;
-       }
-
 #ifdef THREADED_LOADS
        boost::mutex::scoped_lock lock(_mutex);
 #endif
 
+       // ::download shouldn't be called if we completed
+       assert ( !_completed );
+
        long nextpos = _loadPosition + _chunkSize;
-       if ( nextpos > _streamSize ) nextpos = _streamSize;
-       _stream->set_position(nextpos);
 
+       // Can't rely on _streamSize right ?
+       //if ( nextpos > _streamSize ) nextpos = _streamSize;
+
+       _stream->set_position(nextpos);
        long pos = _stream->get_position();
-       assert(pos != -1); // TODO: unhandled error !
+       if ( pos == -1 )
+       {
+               log_error("Error in get_position");
+               abort();
+       }
 
-       assert(pos == nextpos);
-       if (pos != _loadPosition + _chunkSize) {
+       if (pos < nextpos) {
+#ifdef GNASH_DEBUG_LOAD_THREAD
+               log_debug("LoadThread completed during download (get_position 
was %d after set_position(%d))",
+                       pos, nextpos);
+#endif
                _completed = true;
        }