Finalize a throttled FileReader in stages.

Source/core/fileapi/FileReader.cpp

 /*
  * Copyright (C) 2010 Google Inc.  All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
  *     * Redistributions in binary form must reproduce the above
@@ skipping 57 matching lines @@
 // excessive IPC congestion. We limit this to 100 per thread to throttle the
 // requests (the value is arbitrarily chosen).
 static const size_t kMaxOutstandingRequestsPerThread = 100;
 static const double progressNotificationIntervalMS = 50;
 
 class FileReader::ThrottlingController {
 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;
         }
         m_pendingReaders.append(reader);
         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:
     void executeReaders()
     {
         while (m_runningReaders.size() < m_maxRunningReaders) {
             if (m_pendingReaders.isEmpty())
                 return;
             FileReader* reader = m_pendingReaders.takeFirst();
             reader->executePendingRead();
@@ skipping 142 matching lines @@
 }
 
 void FileReader::doAbort()
 {
     ASSERT(m_state != DONE);
 
     terminate();
 
     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()
 {
     if (m_loader) {
         m_loader->cancel();
         m_loader = nullptr;
     }
     m_state = DONE;
     m_loadingState = LoadingStateNone;
@@ skipping 25 matching lines @@
     // It's important that we change m_loadingState before firing any events
     // since any of the events could call abort(), which internally checks
     // if we're still loading (therefore we need abort process) or not.
     m_loadingState = LoadingStateNone;
 
     fireEvent(EventTypeNames::progress);
 
     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)
 {
     if (m_loadingState == LoadingStateAborted)
         return;
     ASSERT(m_loadingState == LoadingStateLoading);
     m_loadingState = LoadingStateNone;
 
     ASSERT(m_state != DONE);
     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)
 {
     if (!m_loader) {
         dispatchEvent(ProgressEvent::create(type, false, 0, 0));
         return;
     }
 
     if (m_loader->totalBytes() >= 0)
@@ skipping 16 matching lines @@
 }
 
 String FileReader::stringResult()
 {
     if (!m_loader || m_error)
         return String();
     return m_loader->stringResult();
 }
 
 } // namespace WebCore