Finalize a throttled FileReader in stages.

Source/core/fileapi/FileReader.cpp

Index: Source/core/fileapi/FileReader.cpp
diff --git a/Source/core/fileapi/FileReader.cpp b/Source/core/fileapi/FileReader.cpp
index 012d118b6a57988c1a56fbdc0da7cbf0870b2c48..bbb24070da9ba5386b478a0ae4a61dbcdd03d373 100644
--- a/Source/core/fileapi/FileReader.cpp
+++ b/Source/core/fileapi/FileReader.cpp
@@ -75,12 +75,32 @@ public:
     ThrottlingController() : m_maxRunningReaders(kMaxOutstandingRequestsPerThread) { }
     ~ThrottlingController() { }
 
+    // Final deactivation steps for a FileReader.
+    class RemovedReader {

[kinuko, 2014/02/21 15:25:48]

nit: could we rename RemoveReader to ReaderRemover (per our naming convention)

+    public:
+        RemovedReader(FileReader* reader, bool wasRunning)

[kinuko, 2014/02/21 15:25:48]

This is internal impl so it's not strongly required but using enum is preferred
over bool parameter

+            : m_reader(reader)
+            , m_wasRunning(wasRunning) { }
+
+        ~RemovedReader()
+        {
+            m_reader->unsetPendingActivity(m_reader);
+            if (m_wasRunning)
+                throttlingController()->executeReaders();
+        }
+
+    private:
+        FileReader* m_reader;
+        bool m_wasRunning;
+    };
+
     void pushReader(FileReader* reader)
     {
         reader->setPendingActivity(reader);
         if (m_pendingReaders.isEmpty()
             && m_runningReaders.size() < m_maxRunningReaders) {
             reader->executePendingRead();
+            ASSERT(!m_runningReaders.contains(reader));
             m_runningReaders.add(reader);
             return;
         }
@@ -88,23 +108,21 @@ public:
         executeReaders();
     }
 
-    void removeReader(FileReader* reader)
+    RemovedReader removeReader(FileReader* reader)
     {
         HashSet<FileReader*>::const_iterator hashIter = m_runningReaders.find(reader);
         if (hashIter != m_runningReaders.end()) {
             m_runningReaders.remove(hashIter);
-            reader->unsetPendingActivity(reader);
-            executeReaders();
-            return;
+            return RemovedReader(reader, true);
         }
         Deque<FileReader*>::const_iterator dequeEnd = m_pendingReaders.end();
         for (Deque<FileReader*>::const_iterator it = m_pendingReaders.begin(); it != dequeEnd; ++it) {
             if (*it == reader) {
                 m_pendingReaders.remove(it);
-                reader->unsetPendingActivity(reader);
-                return;
+                break;
             }
         }
+        return RemovedReader(reader, false);
     }
 
 private:
@@ -267,12 +285,14 @@ void FileReader::doAbort()
 
     m_error = FileError::create(FileError::ABORT_ERR);
 
+    // Unregister the reader (two-phased; see method's result.)
+    throttlingController()->removeReader(this);

[kinuko, 2014/02/21 15:25:48]

Is the intention here is to execute RemoveReader's dtor after firing events?  If
so don't we need to receive the return value?

[sof, 2014/02/21 15:31:39]

No need, the ignored value will be destructed on leaving the scope.

[kinuko, 2014/02/21 15:43:10]

Hmm, my very rough local testing doesn't say so...

+
     fireEvent(EventTypeNames::error);
     fireEvent(EventTypeNames::abort);
     fireEvent(EventTypeNames::loadend);
 
     // All possible events have fired and we're done, no more pending activity.
-    throttlingController()->removeReader(this);
 }
 
 void FileReader::terminate()
@@ -318,11 +338,13 @@ void FileReader::didFinishLoading()
     ASSERT(m_state != DONE);
     m_state = DONE;
 
+    // Unregister the reader (two-phased; see method's result.)
+    throttlingController()->removeReader(this);
+
     fireEvent(EventTypeNames::load);
     fireEvent(EventTypeNames::loadend);
 
     // All possible events have fired and we're done, no more pending activity.
-    throttlingController()->removeReader(this);
 }
 
 void FileReader::didFail(FileError::ErrorCode errorCode)
@@ -336,11 +358,14 @@ void FileReader::didFail(FileError::ErrorCode errorCode)
     m_state = DONE;
 
     m_error = FileError::create(static_cast<FileError::ErrorCode>(errorCode));
+
+    // Unregister the reader (two-phased; see method's result.)
+    throttlingController()->removeReader(this);
+
     fireEvent(EventTypeNames::error);
     fireEvent(EventTypeNames::loadend);
 
     // All possible events have fired and we're done, no more pending activity.
-    throttlingController()->removeReader(this);
 }
 
 void FileReader::fireEvent(const AtomicString& type)