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

Source/core/xml/XMLHttpRequest.cpp

 /*
  *  Copyright (C) 2004, 2006, 2008 Apple Inc. All rights reserved.
  *  Copyright (C) 2005-2007 Alexey Proskuryakov <ap@webkit.org>
  *  Copyright (C) 2007, 2008 Julien Chaffraix <jchaffraix@webkit.org>
  *  Copyright (C) 2008, 2011 Google Inc. All rights reserved.
  *  Copyright (C) 2012 Intel Corporation
  *
  *  This library is free software; you can redistribute it and/or
  *  modify it under the terms of the GNU Lesser General Public
  *  License as published by the Free Software Foundation; either
@@ skipping 519 matching lines @@
 
 void XMLHttpRequest::open(const AtomicString& method, const KURL& url, ExceptionState& exceptionState)
 {
     open(method, url, true, exceptionState);
 }
 
 void XMLHttpRequest::open(const AtomicString& method, const KURL& url, bool async, ExceptionState& exceptionState)
 {
     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;
     m_state = UNSENT;
     m_error = false;
     m_uploadComplete = false;
 
     // clear stuff from possible previous load
     clearResponse();
     clearRequest();
@@ skipping 337 matching lines @@
 void XMLHttpRequest::abort()
 {
     WTF_LOG(Network, "XMLHttpRequest %p abort()", this);
 
     bool sendFlag = m_loader;
 
     // Response is cleared next, save needed progress event data.
     long long expectedLength = m_response.expectedContentLength();
     long long receivedLength = m_receivedLength;
 
-    if (!internalAbort())
+    internalAbort();
+    if (!cancelLoader())
         return;
 
     clearResponse();
 
     // Clear headers as required by the spec
     m_requestHeaders.clear();
 
     if (!((m_state <= OPENED && !sendFlag) || m_state == DONE)) {
         ASSERT(!m_loader);
         handleRequestError(0, EventTypeNames::abort, receivedLength, expectedLength);
     }
     m_state = UNSENT;
 }
 
 void XMLHttpRequest::clearVariablesForLoading()
 {
     m_decoder.clear();
 
     m_finalResponseCharset = String();
 }
 
-bool XMLHttpRequest::internalAbort()
+void XMLHttpRequest::internalAbort()
 {
     m_error = true;
 
     clearVariablesForLoading();
 
     InspectorInstrumentation::didFailXHRLoading(executionContext(), this, this);
 
     if (m_responseLegacyStream && m_state != DONE)
         m_responseLegacyStream->abort();
 
     if (m_responseStream) {
         // When the stream is already closed (including canceled from the
         // user), |error| does nothing.
         // 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;
 
     // Cancelling the ThreadableLoader m_loader may result in calling
     // window.onload synchronously. If such an onload handler contains open()
     // call on the same XMLHttpRequest object, reentry happens.
     //
     // If, window.onload contains open() and send(), m_loader will be set to
     // non 0 value. So, we cannot continue the outer open(). In such case,
     // just abort the outer open() by returning false.
     RefPtrWillBeRawPtr<XMLHttpRequest> protect(this);
     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();
     if (!newLoadStarted)
         m_error = true;
 
     return !newLoadStarted;
 }
 
 void XMLHttpRequest::clearResponse()
@@ skipping 53 matching lines @@
 }
 
 void XMLHttpRequest::handleNetworkError()
 {
     WTF_LOG(Network, "XMLHttpRequest %p handleNetworkError()", this);
 
     // Response is cleared next, save needed progress event data.
     long long expectedLength = m_response.expectedContentLength();
     long long receivedLength = m_receivedLength;
 
-    if (!internalAbort())
+    internalAbort();
+    if (!cancelLoader())
         return;
 
     handleDidFailGeneric();
     handleRequestError(NetworkError, EventTypeNames::error, receivedLength, expectedLength);
 }
 
 void XMLHttpRequest::handleDidCancel()
 {
     WTF_LOG(Network, "XMLHttpRequest %p handleDidCancel()", this);
 
@@ skipping 22 matching lines @@
     // the end here.
     ASSERT(m_error);
     changeState(DONE);
 
     if (!m_uploadComplete) {
         m_uploadComplete = true;
         if (m_upload && m_uploadEventsAllowed)
             m_upload->handleRequestError(type);
     }
 
     // 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);
     dispatchProgressEvent(EventTypeNames::loadend, receivedLength, expectedLength);
 }
 
 void XMLHttpRequest::overrideMimeType(const AtomicString& mimeType, ExceptionState& exceptionState)
 {
     if (m_state == LOADING || m_state == DONE) {
         exceptionState.throwDOMException(InvalidStateError, "MimeType cannot be overridden when the state is LOADING or DONE.");
@@ skipping 367 matching lines @@
 }
 
 void XMLHttpRequest::handleDidTimeout()
 {
     WTF_LOG(Network, "XMLHttpRequest %p handleDidTimeout()", this);
 
     // Response is cleared next, save needed progress event data.
     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();
     handleRequestError(TimeoutError, EventTypeNames::timeout, receivedLength, expectedLength);
 }
 
 void XMLHttpRequest::suspend()
 {
     m_progressEventThrottle.suspend();
 }
 
 void XMLHttpRequest::resume()
 {
     m_progressEventThrottle.resume();
 }
 
 void XMLHttpRequest::stop()
 {
     internalAbort();
+    cancelLoader();
 }
 
 bool XMLHttpRequest::hasPendingActivity() const
 {
     // Neither this object nor the JavaScript wrapper should be deleted while
     // a request is in progress because we need to keep the listeners alive,
     // and they are referenced by the JavaScript wrapper.
     return m_loader;
 }
 
@@ skipping 19 matching lines @@
     visitor->trace(m_responseLegacyStream);
     visitor->trace(m_responseStream);
     visitor->trace(m_streamSource);
     visitor->trace(m_responseDocument);
     visitor->trace(m_progressEventThrottle);
     visitor->trace(m_upload);
     XMLHttpRequestEventTarget::trace(visitor);
 }
 
 } // namespace blink