[XHR] Use hasPendingActivity() to manage XHR's life time.

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 161 matching lines @@
     , m_createdDocument(false)
     , m_error(false)
     , m_uploadEventsAllowed(true)
     , m_uploadComplete(false)
     , m_sameOriginRequest(true)
     , m_receivedLength(0)
     , m_lastSendLineNumber(0)
     , m_exceptionCode(0)
     , m_progressEventThrottle(this)
     , m_responseTypeCode(ResponseTypeDefault)
-    , m_dropProtectionRunner(this, &XMLHttpRequest::dropProtection)
     , m_securityOrigin(securityOrigin)
 {
     initializeXMLHttpRequestStaticData();
 #ifndef NDEBUG
     xmlHttpRequestCounter.increment();
 #endif
     ScriptWrappable::init(this);
 }
 
 XMLHttpRequest::~XMLHttpRequest()
@@ skipping 627 matching lines @@
     if (m_async) {
         if (m_upload)
             request.setReportUploadProgress(true);
 
         // ThreadableLoader::create can return null here, for example if we're no longer attached to a page.
         // This is true while running onunload handlers.
         // FIXME: Maybe we need to be able to send XMLHttpRequests from onunload, <http://bugs.webkit.org/show_bug.cgi?id=10904>.
         // FIXME: Maybe create() can return null for other reasons too?
         ASSERT(!m_loader);
         m_loader = ThreadableLoader::create(executionContext(), this, request, options);
-        if (m_loader) {
-            // 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.
-            setPendingActivity(this);
-        }
     } else {
         ThreadableLoader::loadResourceSynchronously(executionContext(), request, *this, options);
     }
 
     if (!m_exceptionCode && m_error)
         m_exceptionCode = NetworkError;
     if (m_exceptionCode)
         exceptionState.throwUninformativeAndGenericDOMException(m_exceptionCode);
 }
 
 void XMLHttpRequest::abort()
 {
     WTF_LOG(Network, "XMLHttpRequest %p abort()", this);
 
-    // internalAbort() calls dropProtection(), which may release the last reference.
-    RefPtr<XMLHttpRequest> protect(this);

[abarth-chromium, 2013/12/05 04:17:24]

Don't we still need this?  Won't internalAbort remove m_loader and make use
eligible for GC?  If a GC occurs during this function, we'll be destroyed while
on the stack.

[tyoshino (SeeGerritForStatus), 2013/12/05 04:32:10]

oh right! I still didn't understand correctly when GC may happen. kouhei
educated me. thanks. I'll put it back.

-
     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();
@@ skipping 10 matching lines @@
 
 void XMLHttpRequest::clearVariablesForLoading()
 {
     // FIXME: when we add the support for multi-part XHR, we will have to think be careful with this initialization.
     m_receivedLength = 0;
     m_decoder.clear();
 
     m_responseEncoding = String();
 }
 
-bool XMLHttpRequest::internalAbort(DropProtection async)
+bool XMLHttpRequest::internalAbort()
 {
     m_error = true;
 
     clearVariablesForLoading();
 
     InspectorInstrumentation::didFailXHRLoading(executionContext(), this, this);
 
     if (m_responseStream && m_state != DONE)
         m_responseStream->abort();
 
     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 m_loader
-    // is left to be non 0, internalAbort() call for the inner open() makes
-    // an extra dropProtection() call (when we're back to the outer open(),
-    // we'll call dropProtection()). To avoid that, clears m_loader before
-    // calling cancel.
-    //
+    // is left to be non 0, internalAbort() for the inner open() repeats the
+    // the same work. To avoid that, clears m_loader before calling cancel().
+    RefPtr<ThreadableLoader> loader = m_loader.release();
+    loader->cancel();
+
     // 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.
-    RefPtr<ThreadableLoader> loader = m_loader.release();
-    loader->cancel();
-
-    // Save to a local variable since we're going to drop protection.
-    bool newLoadStarted = m_loader;
+    if (m_loader)
+        return false;
 
     // If abort() called internalAbort() and a nested open() ended up
     // clearing the error flag, but didn't send(), make sure the error
     // flag is still set.
-    if (!newLoadStarted)
-        m_error = true;
+    m_error = true;

[abarth-chromium, 2013/12/05 04:17:24]

For example, on this line of code, m_loader is 0, which means we could be GCed.

 
-    if (async == DropProtectionAsync)
-        dropProtectionSoon();
-    else
-        dropProtection();
-
-    return !newLoadStarted;
+    return true;
 }
 
 void XMLHttpRequest::clearResponse()
 {
     m_response = ResourceResponse();
 
     m_responseText.clear();
 
     m_createdDocument = false;
     m_responseDocument = 0;
@@ skipping 94 matching lines @@
     if (!m_uploadComplete) {
         m_uploadComplete = true;
         if (m_upload && m_uploadEventsAllowed)
             m_upload->handleRequestError(type);
     }
 
     dispatchThrottledProgressEvent(EventTypeNames::progress, receivedLength, expectedLength);
     dispatchEventAndLoadEnd(type, receivedLength, expectedLength);
 }
 
-void XMLHttpRequest::dropProtectionSoon()
-{
-    m_dropProtectionRunner.runAsync();
-}
-
-void XMLHttpRequest::dropProtection()
-{
-    unsetPendingActivity(this);
-}
-
 void XMLHttpRequest::overrideMimeType(const AtomicString& override)
 {
     m_mimeTypeOverride = override;
 }
 
 void XMLHttpRequest::setRequestHeader(const AtomicString& name, const AtomicString& value, ExceptionState& exceptionState)
 {
     if (m_state != OPENED || m_loader) {
         exceptionState.throwDOMException(InvalidStateError, ExceptionMessages::failedToExecute("setRequestHeader", "XMLHttpRequest", "The object's state must be OPENED."));
         return;
@@ skipping 172 matching lines @@
         changeState(HEADERS_RECEIVED);
 
     if (m_decoder)
         m_responseText = m_responseText.concatenateWith(m_decoder->flush());
 
     if (m_responseStream)
         m_responseStream->finalize();
 
     InspectorInstrumentation::didFinishXHRLoading(executionContext(), this, this, identifier, m_responseText, m_url, m_lastSendURL, m_lastSendLineNumber);
 
-    // Prevent dropProtection releasing the last reference, and retain |this| until the end of this method.
-    RefPtr<XMLHttpRequest> protect(this);
-
-    if (m_loader) {
+    if (m_loader)
         m_loader = 0;
-        dropProtection();
-    }
 
     changeState(DONE);
 
     clearVariablesForLoading();
 }
 
 void XMLHttpRequest::didSendData(unsigned long long bytesSent, unsigned long long totalBytesToBeSent)
 {
     WTF_LOG(Network, "XMLHttpRequest %p didSendData(%llu, %llu)", this, bytesSent, totalBytesToBeSent);
 
@@ skipping 88 matching lines @@
         // FIXME: Make our implementation and the spec consistent. This
         // behavior was needed when the progress event was not available.
         dispatchReadyStateChangeEvent();
     }
 }
 
 void XMLHttpRequest::handleDidTimeout()
 {
     WTF_LOG(Network, "XMLHttpRequest %p handleDidTimeout()", this);
 
-    // internalAbort() calls dropProtection(), which may release the last reference.
-    RefPtr<XMLHttpRequest> protect(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()
 {
     m_progressEventThrottle.resume();
 }
 
 void XMLHttpRequest::stop()
 {
-    internalAbort(DropProtectionAsync);
+    internalAbort();

[abarth-chromium, 2013/12/05 04:14:03]

Don't we need to set some sort of flag here to prevent us from starting a new
request?

[tyoshino (SeeGerritForStatus), 2013/12/05 04:26:20]

Good catch! We don't want it to start a new loading here.

When I was fixing uaf issue due to reentrance, I asked japhet@ for comments.
There were two choices
a) suppress new loading by inner open() (via window.onload) and continue outer
open()
b) continue inner and abort outer abort

We chose (b). Maybe it's time to rethink. What do you think? We can do (a)
exceptionally for stop() case, but if it makes sense for other cases, we can do
(a) for all.

+}
+
+bool XMLHttpRequest::hasPendingActivity() const
+{
+    return m_loader;

[abarth-chromium, 2013/12/05 04:14:03]

Is there something that stops m_loader from being created if we're already in
the stopped state?

[tyoshino (SeeGerritForStatus), 2013/12/05 04:26:20]

I once tried to add a boolean member, say "m_abortOngoing", to prevent new
loading being started and set it in internalAbort() so that any XHR method will
be noop while we're in internalAbort(). Maybe checking it just in open() is
enough.

 }
 
 void XMLHttpRequest::contextDestroyed()
 {
     ASSERT(!m_loader);
     ActiveDOMObject::contextDestroyed();
 }
 
 const AtomicString& XMLHttpRequest::interfaceName() const
 {
     return EventTargetNames::XMLHttpRequest;
 }
 
 ExecutionContext* XMLHttpRequest::executionContext() const
 {
     return ActiveDOMObject::executionContext();
 }
 
 } // namespace WebCore