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

third_party/WebKit/Source/core/xmlhttprequest/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 219 matching lines @@
       m_exceptionCode(0),
       m_progressEventThrottle(
           XMLHttpRequestProgressEventThrottle::create(this)),
       m_responseTypeCode(ResponseTypeDefault),
       m_isolatedWorldSecurityOrigin(isolatedWorldSecurityOrigin),
       m_eventDispatchRecursionLevel(0),
       m_async(true),
       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) {}
 
 XMLHttpRequest::~XMLHttpRequest() {}
 
 Document* XMLHttpRequest::document() const {
   DCHECK(getExecutionContext()->isDocument());
   return toDocument(getExecutionContext());
 }
@@ skipping 363 matching lines @@
                    << ", " << async << ")";
 
   DCHECK(validateOpenArguments(method, url, exceptionState));
 
   if (!internalAbort())
     return;
 
   State previousState = m_state;
   m_state = kUnsent;
   m_error = false;
-  m_uploadComplete = false;
+  m_uploadCompleteFlag = false;
 
   if (!ContentSecurityPolicy::shouldBypassMainWorld(getExecutionContext()) &&
       !getExecutionContext()->contentSecurityPolicy()->allowConnectToSource(
           url)) {
     // We can safely expose the URL to JavaScript, as these checks happen
     // synchronously before redirection. JavaScript receives no new information.
     exceptionState.throwSecurityError(
         "Refused to connect to '" + url.elidedString() +
         "' because it violates the document's Content Security Policy.");
     return;
@@ skipping 297 matching lines @@
     if (!m_async) {
       throwForLoadFailureIfNeeded(
           exceptionState, "'GET' is the only method allowed for 'blob:' URLs.");
     }
     return;
   }
 
   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));
     }
   }
 
   m_sameOriginRequest = getSecurityOrigin()->canRequestNoSuborigin(m_url);
 
   if (!m_sameOriginRequest && m_includeCredentials)
     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);
   request.setFetchCredentialsMode(
       m_includeCredentials ? WebURLRequest::FetchCredentialsModeInclude
                            : WebURLRequest::FetchCredentialsModeSameOrigin);
   request.setSkipServiceWorker(m_isolatedWorldSecurityOrigin.get()
                                    ? WebURLRequest::SkipServiceWorker::All
                                    : WebURLRequest::SkipServiceWorker::None);
   request.setExternalRequestStateFromRequestorAddressSpace(
       executionContext.securityContext().addressSpace());
 
   InspectorInstrumentation::willLoadXHR(
       &executionContext, this, this, m_method, m_url, m_async,
       httpBody ? httpBody->deepCopy() : nullptr, m_requestHeaders,
       m_includeCredentials);
 
   if (httpBody) {
     DCHECK_NE(m_method, HTTPNames::GET);
     DCHECK_NE(m_method, HTTPNames::HEAD);
     request.setHTTPBody(std::move(httpBody));
   }
 
   if (m_requestHeaders.size() > 0)
     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 =
       ContentSecurityPolicy::shouldBypassMainWorld(&executionContext)
           ? DoNotEnforceContentSecurityPolicy
           : EnforceContentSecurityPolicy;
   options.timeoutMilliseconds = m_timeoutMilliseconds;
 
   ResourceLoaderOptions resourceLoaderOptions;
   resourceLoaderOptions.allowCredentials =
@@ skipping 12 matching lines @@
     request.setDownloadToFile(true);
     resourceLoaderOptions.dataBufferingPolicy = DoNotBufferData;
   }
 
   m_exceptionCode = 0;
   m_error = false;
 
   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);
     m_loader = ThreadableLoader::create(executionContext, this, options,
                                         resourceLoaderOptions);
     m_loader->start(request);
 
     return;
   }
 
@@ skipping 195 matching lines @@
     return;
   }
 
   // With m_error set, the state change steps are minimal: any pending
   // progress event is flushed + a readystatechange is dispatched.
   // No new progress events dispatched; as required, that happens at
   // the end here.
   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() ==
   // false|, when |handleRequestError| is called after |internalAbort()|.  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,
@@ skipping 364 matching lines @@
     return;
 
   if (status() >= 200 && status() < 300) {
     document()->frame()->page()->chromeClient().ajaxSucceeded(
         document()->frame());
   }
 }
 
 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_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.

-  if (m_uploadEventsAllowed)
-    m_upload->dispatchProgressEvent(bytesSent, totalBytesToBeSent);
+  // 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.

   }
 }
 
 void XMLHttpRequest::didReceiveResponse(
     unsigned long identifier,
     const ResourceResponse& response,
     std::unique_ptr<WebDataConsumerHandle> handle) {
   ALLOW_UNUSED_LOCAL(handle);
   DCHECK(!handle);
   NETWORK_DVLOG(1) << this << " didReceiveResponse(" << identifier << ")";
@@ skipping 196 matching lines @@
   visitor->traceWrappers(m_responseLegacyStream);
   visitor->traceWrappers(m_responseDocument);
   visitor->traceWrappers(m_responseArrayBuffer);
 }
 
 std::ostream& operator<<(std::ostream& ostream, const XMLHttpRequest* xhr) {
   return ostream << "XMLHttpRequest " << static_cast<const void*>(xhr);
 }
 
 }  // namespace blink