[XHR] Move the code to clear variables to internalAbort()

Source/core/xml/XMLHttpRequest.cpp

Index: Source/core/xml/XMLHttpRequest.cpp
diff --git a/Source/core/xml/XMLHttpRequest.cpp b/Source/core/xml/XMLHttpRequest.cpp
index 7718b779c385c1b5c47fdfffabcef5e7693da51f..5452bebc08b1274fc203345555a32a8dba90297b 100644
--- a/Source/core/xml/XMLHttpRequest.cpp
+++ b/Source/core/xml/XMLHttpRequest.cpp
@@ -537,7 +537,8 @@ void XMLHttpRequest::open(const AtomicString& method, const KURL& url, bool asyn
 {
     WTF_LOG(Network, "XMLHttpRequest %p open('%s', '%s', %d)", this, method.utf8().data(), url.elidedString().utf8().data(), async);
 
-    if (!internalAbort())
+    internalAbort();
+    if (!cancelLoader())
         return;
 
     State previousState = m_state;
@@ -895,7 +896,8 @@ void XMLHttpRequest::abort()
     long long expectedLength = m_response.expectedContentLength();
     long long receivedLength = m_receivedLength;
 
-    if (!internalAbort())
+    internalAbort();
+    if (!cancelLoader())
         return;
 
     clearResponse();
@@ -917,7 +919,7 @@ void XMLHttpRequest::clearVariablesForLoading()
     m_finalResponseCharset = String();
 }
 
-bool XMLHttpRequest::internalAbort()
+void XMLHttpRequest::internalAbort()
 {
     m_error = true;
 
@@ -934,7 +936,10 @@ bool XMLHttpRequest::internalAbort()
         // FIXME: Create a more specific error.
         m_responseStream->error(DOMException::create(!m_async && m_exceptionCode ? m_exceptionCode : AbortError, "XMLHttpRequest::abort"));
     }
+}
 
+bool XMLHttpRequest::cancelLoader()
+{

[yhirano, 2014/08/21 06:49:20]

ASSERT(m_error) here?

     if (!m_loader)
         return true;
 
@@ -949,7 +954,7 @@ bool XMLHttpRequest::internalAbort()
     RefPtr<ThreadableLoader> loader = m_loader.release();
     loader->cancel();
 
-    // If abort() called internalAbort() and a nested open() ended up
+    // If abort() called cancelLoader() and a nested open() ended up

[yhirano, 2014/08/21 06:49:20]

I'm not sure if this split is good. The latter half of |cancelLoader| is about a
recursion problem of |abort|, not about canceling loader.

[tyoshino (SeeGerritForStatus), 2014/08/21 07:06:31]

Call of abort() is just one of possible scenarios where reentry may happen.

Also, internalAbort() is used not only by abort(). internalAbort() is not good
name. I plan to name it to something different without abort.

After this change, I want to reorganize calls of methods for clearing variables
such as clearResponse() and clearVariableForLoading().

[yhirano, 2014/08/21 08:01:36]

I'm a bit confused.
Are you going to call |cancelLoader| without preceding |internalAbort|, or call
|internalAbort| without following |cancelLoader|?

[tyoshino (SeeGerritForStatus), 2014/08/21 08:06:51]

The latter. See the newly added FIXME below.

I think cancelLoader() is the code most hard to understand. I'd like to split
code in the new internalAbort() from ones in cancelLoader() first and then
reorganize code currently placed in internalAbort() and other variable clearing
code.

     // clearing the error flag, but didn't send(), make sure the error
     // flag is still set.
     bool newLoadStarted = hasPendingActivity();
@@ -1023,7 +1028,8 @@ void XMLHttpRequest::handleNetworkError()
     long long expectedLength = m_response.expectedContentLength();
     long long receivedLength = m_receivedLength;
 
-    if (!internalAbort())
+    internalAbort();
+    if (!cancelLoader())
         return;
 
     handleDidFailGeneric();
@@ -1067,7 +1073,7 @@ void XMLHttpRequest::handleRequestError(ExceptionCode exceptionCode, const Atomi
     }
 
     // Note: The below event dispatch may be called while |hasPendingActivity() == false|,

[yhirano, 2014/08/21 06:49:20]

Please wrap the comment in 80 columns.

-    // when |handleRequestError| is called after |internalAbort()|.
+    // when |handleRequestError| is called after |cancelLoader()|.
     // This is safe, however, as |this| will be kept alive from a strong ref |Event::m_target|.
     dispatchProgressEvent(EventTypeNames::progress, receivedLength, expectedLength);
     dispatchProgressEvent(type, receivedLength, expectedLength);
@@ -1455,7 +1461,11 @@ void XMLHttpRequest::handleDidTimeout()
     long long expectedLength = m_response.expectedContentLength();
     long long receivedLength = m_receivedLength;
 
-    if (!internalAbort())
+    internalAbort();
+    // FIXME: It shouldn't be needed to call cancel() on m_loader. We just need
+    // to clear it. Fix it but after investigating if there's the reentry issue
+    // here, too.
+    if (!cancelLoader())
         return;
 
     handleDidFailGeneric();
@@ -1475,6 +1485,7 @@ void XMLHttpRequest::resume()
 void XMLHttpRequest::stop()
 {
     internalAbort();
+    cancelLoader();
 }
 
 bool XMLHttpRequest::hasPendingActivity() const