Add a pre-finalizer for XHR to cancel the loader

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

Index: third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h
diff --git a/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h b/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h
index 516d17d5c1a2bf5ff13ce1c27fc0414d913487aa..0ee3744aa0a4946ae24edae987df70454d47dd46 100644
--- a/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h
+++ b/third_party/WebKit/Source/core/xmlhttprequest/XMLHttpRequest.h
@@ -73,6 +73,12 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
   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*);
@@ -130,6 +136,7 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
       const ArrayBufferOrArrayBufferViewOrBlobOrDocumentOrStringOrFormData&,
       ExceptionState&);
   void abort();
+  void dispose();
   void setRequestHeader(const AtomicString& name,
                         const AtomicString& value,
                         ExceptionState&);