Add a pre-finalizer for XHR to cancel the loader

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

 /*
  *  Copyright (C) 2003, 2006, 2008 Apple Inc. All rights reserved.
  *  Copyright (C) 2005, 2006 Alexey Proskuryakov <ap@nypop.com>
  *  Copyright (C) 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
  *  version 2 of the License, or (at your option) any later version.
@@ skipping 55 matching lines @@
 class XMLHttpRequestUpload;
 
 class XMLHttpRequest final : public XMLHttpRequestEventTarget,
                              private ThreadableLoaderClient,
                              public DocumentParserClient,
                              public ActiveScriptWrappable<XMLHttpRequest>,
                              public SuspendableObject {
   DEFINE_WRAPPERTYPEINFO();
   USING_GARBAGE_COLLECTED_MIXIN(XMLHttpRequest);
 
+  // In some cases hasPendingActivity doesn't work correctly, i.e.,

[Yuki, 2017/01/20 07:02:53]

IMHO, hasPendingActivity is not designed to keep the object alive forever or
unconditionally.  Even if an object has a pending task, we don't need to keep
the object or its wrapper if the context is discarded for example.

The spec says that we should discard queued tasks if the context is discarded,
but probably we're not doing it correctly, and it could be the root cause.

[yhirano, 2017/01/20 07:10:10]

We have contextDestroyed and it makes hasPendingActivity return false.

+  // doesn't keep |this| alive. We need to cancel the loader in such cases,
+  // which is why we need this pre-finalizer.
+  // TODO(yhirano): Remove this pre-finalizer when the bug is fixed.
+  USING_PRE_FINALIZER(XMLHttpRequest, dispose);

[sof, 2017/01/20 09:17:23]

This object is already eagerly finalized, so also adding a prefinalizer is a bit
strange.

Actually, I wonder if moving ThreadableLoader to the heap introduced an "issue"
here, as the eager finalization would have let go of the loader promptly before,
but it now doesn't.

Assuming it is a bug for the loader and the XHR object to quietly die during the
same sweep, we can either stick with eager finalization or a prefinalizer, but
not both. I'm fine with using a prefinalizer, as the number of live XHRs
wouldn't be excessively large. I hope cancel() won't touch other prefinalized
objects.

[yhirano, 2017/01/20 09:49:56]

No. As written in the ThreadableLoader comments, a ThreadableLoader must be
completed (including canceled) before its destruction. Quietly dying is a
problem and the sweeping order is not related. I thought that rule is kept via
hasPendingActivity and contextDestroyed but I heard otherwise today. I would
like to it a "bug" of ActiveScriptWrappable or ContextLifecycleObserver so I
added the TODO.
Thanks, I will stop XHR from EAGERLY_FINALIZing next week.
DocumentThreadableLoader::cancel calls Resource::removeClient which can be
called in the pre-finalization step. It also calls
ThreadableLoaderClient::didFail, but with m_error set it does nothing.

[yhirano, 2017/01/20 09:54:08]

What happens if we specify both?

 1) Something very bad happens (e.g., crash, leak)
 2) One of them is ignored
 3) They're working but wasteful

[sof, 2017/01/20 09:57:45]

3, it is confused and wasteful to do both. The destructor must run if eagerly
finalized, so you get to promptly let go of some resources, like some of the
buffers and decoder objects that XHR has.

+
  public:
   static XMLHttpRequest* create(ScriptState*);
   static XMLHttpRequest* create(ExecutionContext*);
   ~XMLHttpRequest() override;
 
   // These exact numeric values are important because JS expects them.
   enum State {
     kUnsent = 0,
     kOpened = 1,
     kHeadersReceived = 2,
@@ skipping 37 matching lines @@
             const String& password,
             ExceptionState&);
   void open(const AtomicString& method,
             const KURL&,
             bool async,
             ExceptionState&);
   void send(
       const ArrayBufferOrArrayBufferViewOrBlobOrDocumentOrStringOrFormData&,
       ExceptionState&);
   void abort();
+  void dispose();
   void setRequestHeader(const AtomicString& name,
                         const AtomicString& value,
                         ExceptionState&);
   void overrideMimeType(const AtomicString& override, ExceptionState&);
   String getAllResponseHeaders() const;
   const AtomicString& getResponseHeader(const AtomicString&) const;
   ScriptString responseText(ExceptionState&);
   ScriptString responseJSONSource();
   Document* responseXML(ExceptionState&);
   Blob* responseBlob();
@@ skipping 192 matching lines @@
   bool m_downloadingToFile;
   bool m_responseTextOverflow;
   bool m_sendFlag;
 };
 
 std::ostream& operator<<(std::ostream&, const XMLHttpRequest*);
 
 }  // namespace blink
 
 #endif  // XMLHttpRequest_h