Move ThreadableLoader to Oilpan heap (3/3)

third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h

Index: third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
diff --git a/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h b/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
index bf6b6eac2f60f67f92f07d98ed5450d8711e55f3..bbed4205434a5bec5ffcadcdb17b9f0f58d05bb3 100644
--- a/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
+++ b/third_party/WebKit/Source/core/loader/WorkerThreadableLoader.h
@@ -55,9 +55,31 @@ class WorkerLoaderProxy;
 struct CrossThreadResourceRequestData;
 struct CrossThreadResourceTimingInfoData;
 
-// TODO(yhirano): Draw a diagram to illustrate the class relationship.
-// TODO(yhirano): Rename inner classes so that readers can see in which thread
-// they are living easily.
+// A WorkerThreadableLoader is a ThreadableLoader implementation intended to
+// be used in a WebWorker thread. Because Blink's ResourceFetcher and
+// ResourceLoader work only in the main thread, A WorkerThreadableLoader holds

[haraken, 2016/07/29 15:00:44]

A => a

[yhirano, 2016/08/01 08:52:38]

Done.

+// a ThreadableLoader in the main thread and delegates tasks asynchronously
+// to the loader.
+//
+// CTP: CrossThreadPersistent
+// CTWP: CrossThreadWeakPersistent
+//
+// ----------------------------------------------------------------
+//                 +------------------------+
+//       raw ptr   | ThreadableLoaderClient |
+//      +--------> | worker thread          |
+//      |          +------------------------+
+//      |
+// +----+------------------+    CTP  +------------------------+
+// + WorkerThreadableLoader|<--------+ MainThreadLoaderHolder |
+// | worker thread         +-------->| main thread            |
+// +-----------------------+   CTWP  +----------------------+-+

[haraken, 2016/07/29 15:00:44]

I asked this question before but let me confirm again: What happens if we use
CTP for the WorkerThreadableLoader => MainThreadLoaderHolder? It will create a
cycle temporarily but the cycle is explicitly broken when the worker thread
dies. So it won't cause memory leaks, right?

[yhirano, 2016/08/01 08:52:38]

It's a bit more complicated.

1. As WorkerThreadableLoader requires users to cancel it appropriately (see
ThreadableLoader's comments), WorkerThreadableLoader <=>
MainThreadableLoaderHelper persistent loop should be no harm (though I don't
feel good with the loop).

2. It is not planned now, but I think it's good to move ThreadableLoaderClient
to the heap. With on-heap ThreadableLoaderClient, WorkerThreadableLoader =>
ThreadableLoaderClient "raw ptr" will be replaced with a Member ptr. Also, it's
very common that a ThreadableLoaderClient has a  ThreadableLoader as Member.
That means there will be a persistent loop from ThreadableLoaderClient to
ThreadableLoaderClient itself. That's problematic. Assume that a
ThreadableLoader has a pre-finalizer that cancels its ThreadableLoader. That
pre-finalizer will not be called because there is a persistent loop.

3. As you say that loop will be broken when the thread is terminated but I think
that's not enough. We still see unbound memory consumption increase in a
long-running worker thread.

+//                                                          |
+//                                 +------------------+     | Member
+//                                 | ThreadableLoader | <---+
+//                                 |      main thread |
+//                                 +------------------+

[haraken, 2016/07/29 15:00:44]

This model looks amazing! It looks much simpler than before.

+//
 class WorkerThreadableLoader final : public ThreadableLoader {
 public:
     static void loadResourceSynchronously(WorkerGlobalScope&, const ResourceRequest&, ThreadableLoaderClient&, const ThreadableLoaderOptions&, const ResourceLoaderOptions&);
@@ -99,8 +121,8 @@ private:
     // ThreadableLoaderClient for a DocumentThreadableLoader and forward
     // notifications to the associated WorkerThreadableLoader living in the
     // worker thread.
-    class Peer final : public GarbageCollectedFinalized<Peer>, public ThreadableLoaderClient, public WorkerThreadLifecycleObserver {
-        USING_GARBAGE_COLLECTED_MIXIN(Peer);
+    class MainThreadLoaderHolder final : public GarbageCollectedFinalized<MainThreadLoaderHolder>, public ThreadableLoaderClient, public WorkerThreadLifecycleObserver {
+        USING_GARBAGE_COLLECTED_MIXIN(MainThreadLoaderHolder);
     public:
         static void createAndStart(
             WorkerThreadableLoader*,
@@ -111,7 +133,7 @@ private:
             const ResourceLoaderOptions&,
             PassRefPtr<WaitableEventWithTasks>,
             ExecutionContext*);
-        ~Peer() override;
+        ~MainThreadLoaderHolder() override;
 
         void overrideTimeout(unsigned long timeoutMillisecond);
         void cancel();
@@ -132,7 +154,7 @@ private:
         DECLARE_TRACE();
 
     private:
-        Peer(TaskForwarder*, WorkerThreadLifecycleContext*);
+        MainThreadLoaderHolder(TaskForwarder*, WorkerThreadLifecycleContext*);
         void start(Document&, std::unique_ptr<CrossThreadResourceRequestData>, const ThreadableLoaderOptions&, const ResourceLoaderOptions&);
 
         Member<TaskForwarder> m_forwarder;
@@ -143,7 +165,7 @@ private:
     };
 
     WorkerThreadableLoader(WorkerGlobalScope&, ThreadableLoaderClient*, const ThreadableLoaderOptions&, const ResourceLoaderOptions&, BlockingBehavior);
-    void didStart(Peer*);
+    void didStart(MainThreadLoaderHolder*);
 
     void didSendData(unsigned long long bytesSent, unsigned long long totalBytesToBeSent);
     void didReceiveResponse(unsigned long identifier, std::unique_ptr<CrossThreadResourceResponseData>, std::unique_ptr<WebDataConsumerHandle>);
@@ -164,8 +186,8 @@ private:
     ResourceLoaderOptions m_resourceLoaderOptions;
     BlockingBehavior m_blockingBehavior;
 
-    // |*m_peer| lives in the main thread.
-    CrossThreadPersistent<Peer> m_peer;
+    // |*m_mainThreadLoaderHolder| lives in the main thread.
+    CrossThreadPersistent<MainThreadLoaderHolder> m_mainThreadLoaderHolder;
 };
 
 } // namespace blink