[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 527 matching lines @@
     WTF_LOG(Network, "XMLHttpRequest %p open('%s', '%s', %d)", this, method.utf8().data(), url.elidedString().utf8().data(), async);
 
     if (!internalAbort())
         return;
 
     State previousState = m_state;
     m_state = UNSENT;
     m_error = false;
     m_uploadComplete = false;
 
-    // clear stuff from possible previous load
-    clearResponse();
-    clearRequest();
-
     ASSERT(m_state == UNSENT);

[yhirano, 2014/08/21 11:23:09]

We don't need this assertion: it's too obvious.

[tyoshino (SeeGerritForStatus), 2014/08/21 14:14:58]

Done.

 
     if (!isValidHTTPToken(method)) {
         exceptionState.throwDOMException(SyntaxError, "'" + method + "' is not a valid HTTP method.");
         return;
     }
 
     if (FetchUtils::isForbiddenMethod(method)) {
         exceptionState.throwSecurityError("'" + method + "' HTTP method is unsupported.");
         return;
     }
@@ skipping 328 matching lines @@
 
     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())
         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();
@@ skipping 12 matching lines @@
     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"));
     }
 
+    clearResponse();
+    clearRequest();

[yhirano, 2014/08/21 11:23:09]

Doesn't Moving clearing code beyond loader->cancel() cause a behavior change?

[tyoshino (SeeGerritForStatus), 2014/08/21 12:19:48]

If internalAbort() returned true, it means send() didn't happen, but open() may
still have happened. So, m_requestHeaders may contain after cancel(), but we
still have m_error set. So, there shouldn't be any problem.

[tyoshino (SeeGerritForStatus), 2014/08/21 14:14:58]

And, as we're clearing the variables before cancel(), in window.onload (see
below):
BEFORE THIS CL: Some properties e.g. responseURL was working
AFTER THIS CL: Everything are cleared
I think we don't have to care this.

+
     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.
@@ skipping 35 matching lines @@
     // this only when we clear the response holder variables above.
     m_binaryResponseBuilder.clear();
     m_responseArrayBuffer.clear();
 }
 
 void XMLHttpRequest::clearRequest()
 {
     m_requestHeaders.clear();
 }
 
-void XMLHttpRequest::handleDidFailGeneric()
-{
-    clearResponse();
-    clearRequest();
-
-    m_error = true;
-}
-
 void XMLHttpRequest::dispatchProgressEvent(const AtomicString& type, long long receivedLength, long long expectedLength)
 {
     bool lengthComputable = expectedLength > 0 && receivedLength <= expectedLength;
     unsigned long long loaded = receivedLength >= 0 ? static_cast<unsigned long long>(receivedLength) : 0;
     unsigned long long total = lengthComputable ? static_cast<unsigned long long>(expectedLength) : 0;
 
     m_progressEventThrottle.dispatchProgressEvent(type, lengthComputable, loaded, total);
 
     if (type == EventTypeNames::loadend)
         InspectorInstrumentation::didDispatchXHRLoadendEvent(executionContext(), this);
 }
 
 void XMLHttpRequest::dispatchProgressEventFromSnapshot(const AtomicString& type)
 {
     dispatchProgressEvent(type, m_receivedLength, m_response.expectedContentLength());
 }
 
 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())
         return;
 
-    handleDidFailGeneric();
     handleRequestError(NetworkError, EventTypeNames::error, receivedLength, expectedLength);
 }
 
 void XMLHttpRequest::handleDidCancel()
 {
     WTF_LOG(Network, "XMLHttpRequest %p handleDidCancel()", this);
 
     // Response is cleared next, save needed progress event data.
     long long expectedLength = m_response.expectedContentLength();
     long long receivedLength = m_receivedLength;
 
-    handleDidFailGeneric();
+    if (!internalAbort())
+        return;
+
     handleRequestError(AbortError, EventTypeNames::abort, receivedLength, expectedLength);
 }
 
 void XMLHttpRequest::handleRequestError(ExceptionCode exceptionCode, const AtomicString& type, long long receivedLength, long long expectedLength)
 {
     WTF_LOG(Network, "XMLHttpRequest %p handleRequestError()", this);
 
     // The request error steps for event 'type' and exception 'exceptionCode'.
 
     if (!m_async && exceptionCode) {
@@ skipping 399 matching lines @@
 {
     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())
         return;
 
-    handleDidFailGeneric();
     handleRequestError(TimeoutError, EventTypeNames::timeout, receivedLength, expectedLength);
 }
 
 void XMLHttpRequest::suspend()
 {
     m_progressEventThrottle.suspend();
 }
 
 void XMLHttpRequest::resume()
 {
@@ skipping 36 matching lines @@
     visitor->trace(m_responseStream);
     visitor->trace(m_streamSource);
     visitor->trace(m_responseDocument);
     visitor->trace(m_responseDocumentParser);
     visitor->trace(m_progressEventThrottle);
     visitor->trace(m_upload);
     XMLHttpRequestEventTarget::trace(visitor);
 }
 
 } // namespace blink