Make ThreadableLoaderClient GarbageCollectedMixin

ThreadableLoaderClient has not been GarbageCollected mainly for
WorkerThreadableLoader. Now WorkerThreadableLoader is gone, so we can
make it an on-heap class. Doing so brings several benefits:

 - This is a blocker of removing ThreadableLoader after OOR-CORS is
   enabled, as ResourceClient is a GarbageCollectedMixin.
 - As ThreadableLoader holds a reference to its client as a raw
   pointer, each user needs to guarantee to cancel the loader properly
   before the client gets invalid. As most of clietns are already
   on-heap, this requires pre-finalizer for each client. Also, failing
   to keep the contract leads to crashes which are very hard to
   investigate.

Bug: 900506
Change-Id: I67b3e3dab7e0600f06da5222d458ff2d81c36c7b
Reviewed-on: https://chromium-review.googlesource.com/c/1335056Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Takashi Toyoshima <toyoshim@chromium.org>
Commit-Queue: Yutaka Hirano <yhirano@chromium.org>
Cr-Commit-Position: refs/heads/master@{#607970}

third_party/blink/renderer/core/exported/web_associated_url_loader_impl.cc

@@ -94,15 +94,17 @@ void HTTPRequestHeaderValidator::VisitHeader(const WebString& name,
 // It forwards its ThreadableLoaderClient notifications to a
 // WebAssociatedURLLoaderClient.
 class WebAssociatedURLLoaderImpl::ClientAdapter final
-    : public ThreadableLoaderClient {
+    : public GarbageCollectedFinalized<ClientAdapter>,
+      public ThreadableLoaderClient {
+  USING_GARBAGE_COLLECTED_MIXIN(ClientAdapter);
+
  public:
-  static std::unique_ptr<ClientAdapter> Create(
-      WebAssociatedURLLoaderImpl*,
-      WebAssociatedURLLoaderClient*,
-      const WebAssociatedURLLoaderOptions&,
-      network::mojom::FetchRequestMode,
-      network::mojom::FetchCredentialsMode,
-      scoped_refptr<base::SingleThreadTaskRunner>);
+  ClientAdapter(WebAssociatedURLLoaderImpl*,
+                WebAssociatedURLLoaderClient*,
+                const WebAssociatedURLLoaderOptions&,
+                network::mojom::FetchRequestMode,
+                network::mojom::FetchCredentialsMode,
+                scoped_refptr<base::SingleThreadTaskRunner>);
 
   // ThreadableLoaderClient
   void DidSendData(unsigned long long /*bytesSent*/,
@@ -140,13 +142,6 @@ class WebAssociatedURLLoaderImpl::ClientAdapter final
   }
 
  private:
-  ClientAdapter(WebAssociatedURLLoaderImpl*,
-                WebAssociatedURLLoaderClient*,
-                const WebAssociatedURLLoaderOptions&,
-                network::mojom::FetchRequestMode,
-                network::mojom::FetchCredentialsMode,
-                scoped_refptr<base::SingleThreadTaskRunner>);
-
   void NotifyError(TimerBase*);
 
   WebAssociatedURLLoaderImpl* loader_;
@@ -163,19 +158,6 @@ class WebAssociatedURLLoaderImpl::ClientAdapter final
   DISALLOW_COPY_AND_ASSIGN(ClientAdapter);
 };
 
-std::unique_ptr<WebAssociatedURLLoaderImpl::ClientAdapter>
-WebAssociatedURLLoaderImpl::ClientAdapter::Create(
-    WebAssociatedURLLoaderImpl* loader,
-    WebAssociatedURLLoaderClient* client,
-    const WebAssociatedURLLoaderOptions& options,
-    network::mojom::FetchRequestMode fetch_request_mode,
-    network::mojom::FetchCredentialsMode credentials_mode,
-    scoped_refptr<base::SingleThreadTaskRunner> task_runner) {
-  return base::WrapUnique(new ClientAdapter(loader, client, options,
-                                            fetch_request_mode,
-                                            credentials_mode, task_runner));
-}
-
 WebAssociatedURLLoaderImpl::ClientAdapter::ClientAdapter(
     WebAssociatedURLLoaderImpl* loader,
     WebAssociatedURLLoaderClient* client,
@@ -406,7 +388,7 @@ void WebAssociatedURLLoaderImpl::LoadAsynchronously(
     task_runner = Thread::Current()->GetTaskRunner();
   }
   client_ = client;
-  client_adapter_ = ClientAdapter::Create(
+  client_adapter_ = MakeGarbageCollected<ClientAdapter>(
       this, client, options_, request.GetFetchRequestMode(),
       request.GetFetchCredentialsMode(), std::move(task_runner));
 
@@ -442,7 +424,7 @@ void WebAssociatedURLLoaderImpl::LoadAsynchronously(
 
     if (observer_) {
       Document& document = To<Document>(*observer_->LifecycleContext());
-      loader_ = new ThreadableLoader(document, client_adapter_.get(),
+      loader_ = new ThreadableLoader(document, client_adapter_,
                                      resource_loader_options);
       loader_->Start(webcore_request);
     }
@@ -477,7 +459,7 @@ void WebAssociatedURLLoaderImpl::CancelLoader() {
     loader_->Cancel();
     loader_ = nullptr;
   }
-  client_adapter_.reset();
+  client_adapter_ = nullptr;
 }
 
 void WebAssociatedURLLoaderImpl::SetDefersLoading(bool defers_loading) {

third_party/blink/renderer/core/exported/web_associated_url_loader_impl.h

@@ -59,7 +59,7 @@ class CORE_EXPORT WebAssociatedURLLoaderImpl final
 
   // An adapter which converts the hreadableLoaderClient method
   // calls into the WebURLLoaderClient method calls.
-  std::unique_ptr<ClientAdapter> client_adapter_;
+  Persistent<ClientAdapter> client_adapter_;
   Persistent<ThreadableLoader> loader_;
 
   // A ContextLifecycleObserver for cancelling |m_loader| when the Document

third_party/blink/renderer/core/fetch/fetch_manager.cc

@@ -201,7 +201,7 @@ class SRIBytesConsumer final : public BytesConsumer {
 class FetchManager::Loader final
     : public GarbageCollectedFinalized<FetchManager::Loader>,
       public ThreadableLoaderClient {
-  USING_PRE_FINALIZER(FetchManager::Loader, Dispose);
+  USING_GARBAGE_COLLECTED_MIXIN(Loader);
 
  public:
   static Loader* Create(ExecutionContext* execution_context,
@@ -215,7 +215,7 @@ class FetchManager::Loader final
   }
 
   ~Loader() override;
-  virtual void Trace(blink::Visitor*);
+  void Trace(blink::Visitor*) override;
 
   bool WillFollowRedirect(const KURL&, const ResourceResponse&) override;
   void DidReceiveResponse(unsigned long,
@@ -413,6 +413,7 @@ void FetchManager::Loader::Trace(blink::Visitor* visitor) {
   visitor->Trace(integrity_verifier_);
   visitor->Trace(signal_);
   visitor->Trace(execution_context_);
+  ThreadableLoaderClient::Trace(visitor);
 }
 
 bool FetchManager::Loader::WillFollowRedirect(

third_party/blink/renderer/core/inspector/inspector_network_agent.cc

@@ -663,6 +663,7 @@ void InspectorNetworkAgent::Trace(blink::Visitor* visitor) {
   visitor->Trace(inspected_frames_);
   visitor->Trace(worker_global_scope_);
   visitor->Trace(resources_data_);
+  visitor->Trace(pending_request_);
   visitor->Trace(replay_xhrs_);
   visitor->Trace(replay_xhrs_to_be_deleted_);
   visitor->Trace(pending_xhr_replay_data_);

third_party/blink/renderer/core/inspector/inspector_network_agent.h

@@ -269,7 +269,7 @@ class CORE_EXPORT InspectorNetworkAgent final
   // Stores the pending ThreadableLoaderClient till an identifier for
   // the load is generated by the loader and passed to the inspector
   // via the WillSendRequest() method.
-  ThreadableLoaderClient* pending_request_;
+  Member<ThreadableLoaderClient> pending_request_;
   InspectorPageAgent::ResourceType pending_request_type_;
 
   Member<XHRReplayData> pending_xhr_replay_data_;

third_party/blink/renderer/core/loader/threadable_loader.cc

@@ -104,6 +104,8 @@ AtomicString CreateAccessControlRequestHeadersHeader(
 class ThreadableLoader::DetachedClient final
     : public GarbageCollectedFinalized<DetachedClient>,
       public ThreadableLoaderClient {
+  USING_GARBAGE_COLLECTED_MIXIN(DetachedClient);
+
  public:
   explicit DetachedClient(ThreadableLoader* loader)
       : self_keep_alive_(this), loader_(loader) {}
@@ -114,7 +116,10 @@ class ThreadableLoader::DetachedClient final
   }
   void DidFail(const ResourceError&) override { self_keep_alive_.Clear(); }
   void DidFailRedirectCheck() override { self_keep_alive_.Clear(); }
-  void Trace(Visitor* visitor) { visitor->Trace(loader_); }
+  void Trace(Visitor* visitor) override {
+    visitor->Trace(loader_);
+    ThreadableLoaderClient::Trace(visitor);
+  }
 
  private:
   SelfKeepAlive<DetachedClient> self_keep_alive_;
@@ -482,15 +487,7 @@ void ThreadableLoader::MakeCrossOriginAccessRequest(
   LoadRequest(cross_origin_request, cross_origin_options);
 }
 
-ThreadableLoader::~ThreadableLoader() {
-  // |client_| is a raw pointer and having a non-null |client_| here probably
-  // means UaF.
-  // In the detached case, |this| is held by DetachedClient defined above, but
-  // SelfKeepAlive in DetachedClient is forcibly cancelled on worker thread
-  // termination. We can safely ignore this case.
-  CHECK(!client_ || detached_);
-  DCHECK(!GetResource());
-}
+ThreadableLoader::~ThreadableLoader() {}
 
 void ThreadableLoader::SetTimeout(const TimeDelta& timeout) {
   timeout_ = timeout;
@@ -1105,6 +1102,7 @@ Document* ThreadableLoader::GetDocument() const {
 
 void ThreadableLoader::Trace(blink::Visitor* visitor) {
   visitor->Trace(execution_context_);
+  visitor->Trace(client_);
   RawResourceClient::Trace(visitor);
 }
 

third_party/blink/renderer/core/loader/threadable_loader.h

@@ -89,10 +89,6 @@ class CORE_EXPORT ThreadableLoader final
   // After any of these methods is called, the loader won't call any of the
   // ThreadableLoaderClient methods.
   //
-  // A user must guarantee that the loading completes before the attached
-  // client gets invalid. Also, a user must guarantee that the loading
-  // completes before the ThreadableLoader is destructed.
-  //
   // When ThreadableLoader::Cancel() is called,
   // ThreadableLoaderClient::DidFail() is called with a ResourceError
   // with IsCancellation() returning true, if any of DidFinishLoading()
@@ -100,7 +96,7 @@ class CORE_EXPORT ThreadableLoader final
   // called with a ResourceError with IsCancellation() returning true
   // also for cancellation happened inside the loader.)
   //
-  // ThreadableLoaderClient methods may call cancel().
+  // ThreadableLoaderClient methods may call Cancel().
   ThreadableLoader(ExecutionContext&,
                    ThreadableLoaderClient*,
                    const ResourceLoaderOptions&);
@@ -211,7 +207,7 @@ class CORE_EXPORT ThreadableLoader final
   // TODO(kinuko): Remove dependency to document.
   Document* GetDocument() const;
 
-  ThreadableLoaderClient* client_;
+  Member<ThreadableLoaderClient> client_;
   Member<ExecutionContext> execution_context_;
 
   TimeDelta timeout_;

third_party/blink/renderer/core/loader/threadable_loader_client.h

@@ -46,7 +46,7 @@ class ResourceError;
 class ResourceResponse;
 class ResourceTimingInfo;
 
-class CORE_EXPORT ThreadableLoaderClient {
+class CORE_EXPORT ThreadableLoaderClient : public GarbageCollectedMixin {
  public:
   virtual void DidSendData(unsigned long long /*bytesSent*/,
                            unsigned long long /*totalBytesToBeSent*/) {}

third_party/blink/renderer/core/loader/threadable_loader_test.cc

@@ -53,12 +53,13 @@ using Checkpoint = testing::StrictMock<testing::MockFunction<void(int)>>;
 
 constexpr char kFileName[] = "fox-null-terminated.html";
 
-class MockThreadableLoaderClient : public ThreadableLoaderClient {
+class MockThreadableLoaderClient final
+    : public GarbageCollectedFinalized<MockThreadableLoaderClient>,
+      public ThreadableLoaderClient {
+  USING_GARBAGE_COLLECTED_MIXIN(MockThreadableLoaderClient);
+
  public:
-  static std::unique_ptr<MockThreadableLoaderClient> Create() {
-    return base::WrapUnique(
-        new testing::StrictMock<MockThreadableLoaderClient>);
-  }
+  MockThreadableLoaderClient() = default;
   MOCK_METHOD2(DidSendData, void(unsigned long long, unsigned long long));
   MOCK_METHOD3(DidReceiveResponseMock,
                void(unsigned long,
@@ -77,9 +78,6 @@ class MockThreadableLoaderClient : public ThreadableLoaderClient {
   MOCK_METHOD0(DidFailRedirectCheck, void());
   MOCK_METHOD1(DidReceiveResourceTiming, void(const ResourceTimingInfo&));
   MOCK_METHOD1(DidDownloadData, void(int));
-
- protected:
-  MockThreadableLoaderClient() = default;
 };
 
 bool IsCancellation(const ResourceError& error) {
@@ -242,19 +240,21 @@ class ThreadableLoaderTest : public testing::Test {
 
   void CreateLoader() { helper_->CreateLoader(Client()); }
 
-  MockThreadableLoaderClient* Client() const { return client_.get(); }
+  MockThreadableLoaderClient* Client() const { return client_.Get(); }
 
  private:
   void SetUp() override {
-    client_ = MockThreadableLoaderClient::Create();
+    client_ = MakeGarbageCollected<MockThreadableLoaderClient>();
     helper_->OnSetUp();
   }
 
   void TearDown() override {
     helper_->OnTearDown();
-    client_.reset();
+    client_ = nullptr;
+    // We need GC here to avoid gmock flakiness.
+    ThreadState::Current()->CollectAllGarbage();
   }
-  std::unique_ptr<MockThreadableLoaderClient> client_;
+  Persistent<MockThreadableLoaderClient> client_;
   std::unique_ptr<ThreadableLoaderTestHelper> helper_;
 };
 

third_party/blink/renderer/core/workers/worker_classic_script_loader.cc

@@ -243,6 +243,7 @@ void WorkerClassicScriptLoader::Trace(Visitor* visitor) {
   visitor->Trace(threadable_loader_);
   visitor->Trace(content_security_policy_);
   visitor->Trace(execution_context_);
+  ThreadableLoaderClient::Trace(visitor);
 }
 
 void WorkerClassicScriptLoader::Cancel() {

third_party/blink/renderer/core/workers/worker_classic_script_loader.h

@@ -55,11 +55,7 @@ class TextResourceDecoder;
 class CORE_EXPORT WorkerClassicScriptLoader final
     : public GarbageCollectedFinalized<WorkerClassicScriptLoader>,
       public ThreadableLoaderClient {
-  // We have to cancel |threadable_loader_| before destruction. Otherwise
-  // DidFail() of the deleted |this| will be called from
-  // ThreadableLoader::NotifyFinished() when the associated context will be
-  // destroyed.
-  USING_PRE_FINALIZER(WorkerClassicScriptLoader, Cancel);
+  USING_GARBAGE_COLLECTED_MIXIN(WorkerClassicScriptLoader);
 
  public:
   WorkerClassicScriptLoader();
@@ -122,7 +118,7 @@ class CORE_EXPORT WorkerClassicScriptLoader final
   void DidFail(const ResourceError&) override;
   void DidFailRedirectCheck() override;
 
-  virtual void Trace(Visitor*);
+  void Trace(Visitor*) override;
 
  private:
   void NotifyError();

third_party/blink/renderer/core/xmlhttprequest/xml_http_request.cc

@@ -2014,6 +2014,7 @@ void XMLHttpRequest::Trace(blink::Visitor* visitor) {
   visitor->Trace(blob_loader_);
   visitor->Trace(response_text_);
   XMLHttpRequestEventTarget::Trace(visitor);
+  ThreadableLoaderClient::Trace(visitor);
   DocumentParserClient::Trace(visitor);
   PausableObject::Trace(visitor);
 }

third_party/blink/renderer/core/xmlhttprequest/xml_http_request.h

@@ -77,12 +77,6 @@ class XMLHttpRequest final : public XMLHttpRequestEventTarget,
   DEFINE_WRAPPERTYPEINFO();
   USING_GARBAGE_COLLECTED_MIXIN(XMLHttpRequest);
 
-  // In some cases hasPendingActivity doesn't work correctly, i.e.,
-  // 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);
-
  public:
   static XMLHttpRequest* Create(ScriptState*);
   static XMLHttpRequest* Create(ExecutionContext*);

third_party/blink/renderer/modules/background_fetch/background_fetch_icon_loader.h

@@ -23,6 +23,8 @@ struct WebSize;
 class MODULES_EXPORT BackgroundFetchIconLoader final
     : public GarbageCollectedFinalized<BackgroundFetchIconLoader>,
       public ThreadableLoaderClient {
+  USING_GARBAGE_COLLECTED_MIXIN(BackgroundFetchIconLoader);
+
  public:
   // The bitmap may be empty if the request failed or the image data could not
   // be decoded. The int64_t returned is the scale of the ideal to chosen icon,
@@ -50,9 +52,10 @@ class MODULES_EXPORT BackgroundFetchIconLoader final
   void DidFail(const ResourceError& error) override;
   void DidFailRedirectCheck() override;
 
-  void Trace(blink::Visitor* visitor) {
+  void Trace(blink::Visitor* visitor) override {
     visitor->Trace(threadable_loader_);
     visitor->Trace(icons_);
+    ThreadableLoaderClient::Trace(visitor);
   }
 
  private:

third_party/blink/renderer/modules/eventsource/event_source.cc

@@ -359,6 +359,7 @@ void EventSource::Trace(blink::Visitor* visitor) {
   visitor->Trace(parser_);
   visitor->Trace(loader_);
   EventTargetWithInlineData::Trace(visitor);
+  ThreadableLoaderClient::Trace(visitor);
   ContextLifecycleObserver::Trace(visitor);
   EventSourceParser::Client::Trace(visitor);
 }

third_party/blink/renderer/modules/notifications/notification_image_loader.h

@@ -27,6 +27,8 @@ class ResourceError;
 class MODULES_EXPORT NotificationImageLoader final
     : public GarbageCollectedFinalized<NotificationImageLoader>,
       public ThreadableLoaderClient {
+  USING_GARBAGE_COLLECTED_MIXIN(NotificationImageLoader);
+
  public:
   // Type names are used in UMAs, so do not rename.
   enum class Type { kImage, kIcon, kBadge, kActionIcon };
@@ -59,7 +61,10 @@ class MODULES_EXPORT NotificationImageLoader final
   void DidFail(const ResourceError& error) override;
   void DidFailRedirectCheck() override;
 
-  void Trace(blink::Visitor* visitor) { visitor->Trace(threadable_loader_); }
+  void Trace(blink::Visitor* visitor) override {
+    visitor->Trace(threadable_loader_);
+    ThreadableLoaderClient::Trace(visitor);
+  }
 
  private:
   void RunCallbackWithEmptyBitmap();