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

Source/core/xml/XMLHttpRequest.cpp

Index: Source/core/xml/XMLHttpRequest.cpp
diff --git a/Source/core/xml/XMLHttpRequest.cpp b/Source/core/xml/XMLHttpRequest.cpp
index dc6d85947c97ebbf57cb8fd5ca7261bd4ee50da6..fc9941c5b327aa2ac58bbb72d6117baa73d5d375 100644
--- a/Source/core/xml/XMLHttpRequest.cpp
+++ b/Source/core/xml/XMLHttpRequest.cpp
@@ -179,7 +179,6 @@ XMLHttpRequest::XMLHttpRequest(ExecutionContext* context, PassRefPtr<SecurityOri
     , m_exceptionCode(0)
     , m_progressEventThrottle(this)
     , m_responseTypeCode(ResponseTypeDefault)
-    , m_dropProtectionRunner(this, &XMLHttpRequest::dropProtection)
     , m_securityOrigin(securityOrigin)
 {
     initializeXMLHttpRequestStaticData();
@@ -827,12 +826,6 @@ void XMLHttpRequest::createRequest(ExceptionState& exceptionState)
         // 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);
     }
@@ -847,9 +840,6 @@ 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.
@@ -880,7 +870,7 @@ void XMLHttpRequest::clearVariablesForLoading()
     m_responseEncoding = String();
 }
 
-bool XMLHttpRequest::internalAbort(DropProtection async)
+bool XMLHttpRequest::internalAbort()
 {
     m_error = true;
 
@@ -897,32 +887,23 @@ bool XMLHttpRequest::internalAbort(DropProtection async)
     // 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.
-    //
-    // 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.
+    // 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();
 
-    // Save to a local variable since we're going to drop protection.
-    bool newLoadStarted = m_loader;
+    // 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.
+    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;
-
-    if (async == DropProtectionAsync)
-        dropProtectionSoon();
-    else
-        dropProtection();
+    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.

 
-    return !newLoadStarted;
+    return true;
 }
 
 void XMLHttpRequest::clearResponse()
@@ -1037,16 +1018,6 @@ void XMLHttpRequest::handleRequestError(ExceptionCode exceptionCode, const Atomi
     dispatchEventAndLoadEnd(type, receivedLength, expectedLength);
 }
 
-void XMLHttpRequest::dropProtectionSoon()
-{
-    m_dropProtectionRunner.runAsync();
-}
-
-void XMLHttpRequest::dropProtection()
-{
-    unsetPendingActivity(this);
-}
-
 void XMLHttpRequest::overrideMimeType(const AtomicString& override)
 {
     m_mimeTypeOverride = override;
@@ -1239,13 +1210,8 @@ void XMLHttpRequest::didFinishLoading(unsigned long identifier, double)
 
     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);
 
@@ -1354,9 +1320,6 @@ 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;
@@ -1380,7 +1343,12 @@ void XMLHttpRequest::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()