Fix XHR's logic to determine whether or not to dispatch upload progress events

third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp

Index: third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
diff --git a/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp b/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
index 91188b57fbf107336e00b9d22827900354745acf..1149f754bd133d33461db8389108a5108f7603db 100644
--- a/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
+++ b/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.cpp
@@ -237,8 +237,8 @@ XMLHttpRequest::XMLHttpRequest(
       m_includeCredentials(false),
       m_parsedResponse(false),
       m_error(false),
-      m_uploadEventsAllowed(true),
-      m_uploadComplete(false),
+      m_uploadListenerFlag(false),
+      m_uploadCompleteFlag(false),
       m_sameOriginRequest(true),
       m_downloadingToFile(false),
       m_responseTextOverflow(false) {}
@@ -622,7 +622,7 @@ void XMLHttpRequest::open(const AtomicString& method,
   State previousState = m_state;
   m_state = kUnsent;
   m_error = false;
-  m_uploadComplete = false;
+  m_uploadCompleteFlag = false;
 
   if (!ContentSecurityPolicy::shouldBypassMainWorld(getExecutionContext()) &&
       !getExecutionContext()->contentSecurityPolicy()->allowConnectToSource(
@@ -940,17 +940,18 @@ void XMLHttpRequest::createRequest(PassRefPtr<EncodedFormData> httpBody,
   DCHECK(getExecutionContext());
   ExecutionContext& executionContext = *getExecutionContext();
 
-  // The presence of upload event listeners forces us to use preflighting
-  // because POSTing to an URL that does not permit cross origin requests should
-  // look exactly like POSTing to an URL that does not respond at all.
-  // Also, only async requests support upload progress events.
-  bool uploadEvents = false;
+  m_uploadListenerFlag = false;

[yhirano, 2016/10/20 11:27:54]

In the spec this flag is initialized in open function. How about moving the
statement to there?

   if (m_async) {
     InspectorInstrumentation::asyncTaskScheduled(
         &executionContext, "XMLHttpRequest.send", this, true);
+
     dispatchProgressEvent(EventTypeNames::loadstart, 0, 0);
-    if (httpBody && m_upload) {
-      uploadEvents = m_upload->hasEventListeners();
+
+    // For optimization, we set the upload listener flag only when m_async is
+    // true unlike the spec.
+    if (m_upload && m_upload->hasEventListeners()) {
+      m_uploadListenerFlag = true;
+
       m_upload->dispatchEvent(
           ProgressEvent::create(EventTypeNames::loadstart, false, 0, 0));
     }
@@ -962,12 +963,6 @@ void XMLHttpRequest::createRequest(PassRefPtr<EncodedFormData> httpBody,
     UseCounter::count(&executionContext,
                       UseCounter::XMLHttpRequestCrossOriginWithCredentials);
 
-  // We also remember whether upload events should be allowed for this request
-  // in case the upload listeners are added after the request is started.
-  m_uploadEventsAllowed =
-      m_sameOriginRequest || uploadEvents ||
-      !FetchUtils::isSimpleRequest(m_method, m_requestHeaders);
-
   ResourceRequest request(m_url);
   request.setHTTPMethod(m_method);
   request.setRequestContext(WebURLRequest::RequestContextXMLHttpRequest);
@@ -995,7 +990,8 @@ void XMLHttpRequest::createRequest(PassRefPtr<EncodedFormData> httpBody,
     request.addHTTPHeaderFields(m_requestHeaders);
 
   ThreadableLoaderOptions options;
-  options.preflightPolicy = uploadEvents ? ForcePreflight : ConsiderPreflight;
+  options.preflightPolicy =
+      m_uploadListenerFlag ? ForcePreflight : ConsiderPreflight;
   options.crossOriginRequestPolicy = UseAccessControl;
   options.initiator = FetchInitiatorTypeNames::xmlhttprequest;
   options.contentSecurityPolicyEnforcement =
@@ -1028,7 +1024,12 @@ void XMLHttpRequest::createRequest(PassRefPtr<EncodedFormData> httpBody,
   if (m_async) {
     UseCounter::count(&executionContext,
                       UseCounter::XMLHttpRequestAsynchronous);
-    if (m_upload)
+
+    // The presence of upload event listeners forces us to use preflighting
+    // because POSTing to an URL that does not permit cross origin requests
+    // should look exactly like POSTing to an URL that does not respond at all.
+    // Also, only async requests support upload progress events.
+    if (m_uploadListenerFlag)
       request.setReportUploadProgress(true);
 
     DCHECK(!m_loader);
@@ -1244,10 +1245,13 @@ void XMLHttpRequest::handleRequestError(ExceptionCode exceptionCode,
   DCHECK(m_error);
   changeState(kDone);
 
-  if (!m_uploadComplete) {
-    m_uploadComplete = true;
-    if (m_upload && m_uploadEventsAllowed)
+  if (!m_uploadCompleteFlag) {
+    m_uploadCompleteFlag = true;

[yhirano, 2016/10/20 11:27:54]

Is substituting unconditionally enough?

+
+    if (m_uploadListenerFlag) {
+      DCHECK(m_upload);
       m_upload->handleRequestError(type);
+    }
   }
 
   // Note: The below event dispatch may be called while |hasPendingActivity() ==
@@ -1632,21 +1636,30 @@ void XMLHttpRequest::endLoading() {
 
 void XMLHttpRequest::didSendData(unsigned long long bytesSent,
                                  unsigned long long totalBytesToBeSent) {
+  DCHECK(m_async);

[yhirano, 2016/10/20 12:17:02]

You call setReportUploadProgress only when m_uploadListenerFlag is set. Can we
have DCHECK(m_uploadListenerFlag)?

   NETWORK_DVLOG(1) << this << " didSendData(" << bytesSent << ", "
                    << totalBytesToBeSent << ")";
   ScopedEventDispatchProtect protect(&m_eventDispatchRecursionLevel);
 
-  if (!m_upload)
-    return;
-
-  if (m_uploadEventsAllowed)
+  if (m_uploadListenerFlag) {
+    DCHECK(m_upload);
     m_upload->dispatchProgressEvent(bytesSent, totalBytesToBeSent);
+  }
+

[yhirano, 2016/10/20 12:17:02]

Not related to this CL but we may need an early-return here for the case where
open and/or send are called in the progress handler.

+  // TODO(tyoshino): Since AsyncResourceHandler doesn't send us any

[yhirano, 2016/10/20 11:27:54]

"Since" is not needed?

+  // notification when the total size is 0. Therefore, we cannot depend on
+  // didSendData() invocation to dispatch the progress events for "process
+  // request end-of-body" hook as specified in the XHR spec. Fix this.
+  if (bytesSent == totalBytesToBeSent && !m_uploadCompleteFlag) {
+    m_uploadCompleteFlag = true;
 
-  if (bytesSent == totalBytesToBeSent && !m_uploadComplete) {
-    m_uploadComplete = true;
-    if (m_uploadEventsAllowed)
+    if (m_uploadListenerFlag) {
+      DCHECK(m_upload);
       m_upload->dispatchEventAndLoadEnd(EventTypeNames::load, true, bytesSent,
                                         totalBytesToBeSent);
+    }
+
+    return;

[yhirano, 2016/10/20 11:27:54]

This statement is redundant.

   }
 }