Define DetachableConsoleLogger

Define DetachableConsoleLogger in console_logger.h, rather than in resource_fetcher.cc, in order to make it possible to know whether it is detachable from the type name. Bug: 924024 Change-Id: I07e2f227c34050c515f973b46f167004e7c855f3 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1609400 Commit-Queue: Yutaka Hirano <yhirano@chromium.org> Reviewed-by: Hiroshige Hayashizaki <hiroshige@chromium.org> Cr-Commit-Position: refs/heads/master@{#659767}
Denger-Network · May 15, 2019 · 59b07ab · 59b07ab
1 parent 28b732c
commit 59b07ab
Show file tree

Hide file tree

Showing 9 changed files with 53 additions and 57 deletions.
diff --git a/third_party/blink/renderer/core/loader/frame_fetch_context.cc b/third_party/blink/renderer/core/loader/frame_fetch_context.cc
@@ -224,7 +224,9 @@ ResourceFetcher* FrameFetchContext::CreateFetcherForCommittedDocument(
       MakeGarbageCollected<FrameFetchContext>(*frame_or_imported_document),
       frame.GetTaskRunner(TaskType::kNetworking),
       MakeGarbageCollected<LoaderFactoryForFrame>(*frame_or_imported_document));
-  init.console_logger = &document;
+  auto* console_logger =
+      MakeGarbageCollected<DetachableConsoleLogger>(&document);
+  init.console_logger = console_logger;
   // Frame loading should normally start with |kTight| throttling, as the
   // frame will be in layout-blocking state until the <body> tag is inserted
   init.initial_throttling_policy =
@@ -255,7 +257,9 @@ ResourceFetcher* FrameFetchContext::CreateFetcherForImportedDocument(
       MakeGarbageCollected<FrameFetchContext>(frame_or_imported_document),
       document->GetTaskRunner(blink::TaskType::kNetworking),
       MakeGarbageCollected<LoaderFactoryForFrame>(frame_or_imported_document));
-  init.console_logger = document;
+  auto* console_logger =
+      MakeGarbageCollected<DetachableConsoleLogger>(document);
+  init.console_logger = console_logger;
   init.frame_scheduler = frame.GetFrameScheduler();
   auto* fetcher = MakeGarbageCollected<ResourceFetcher>(init);
   fetcher->SetResourceLoadObserver(

diff --git a/third_party/blink/renderer/core/loader/resource/image_resource_test.cc b/third_party/blink/renderer/core/loader/resource/image_resource_test.cc
@@ -1877,7 +1877,7 @@ TEST(ImageResourceTest, PeriodicFlushTest) {
   auto frame_scheduler = std::make_unique<scheduler::FakeFrameScheduler>();
   auto* scheduler = MakeGarbageCollected<ResourceLoadScheduler>(
       ResourceLoadScheduler::ThrottlingPolicy::kNormal, *properties,
-      frame_scheduler.get(), *MakeGarbageCollected<NullConsoleLogger>());
+      frame_scheduler.get(), *MakeGarbageCollected<DetachableConsoleLogger>());
   ImageResource* image_resource = ImageResource::CreateForTest(test_url);
 
   // Ensure that |image_resource| has a loader.

diff --git a/third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc b/third_party/blink/renderer/core/workers/worker_or_worklet_global_scope.cc
@@ -300,7 +300,8 @@ ResourceFetcher* WorkerOrWorkletGlobalScope::CreateFetcherInternal(
         GetTaskRunner(TaskType::kNetworking),
         MakeGarbageCollected<LoaderFactoryForWorker>(
             *this, web_worker_fetch_context_));
-    init.console_logger = this;
+    auto* console_logger = MakeGarbageCollected<DetachableConsoleLogger>(this);
+    init.console_logger = console_logger;
     fetcher = MakeGarbageCollected<ResourceFetcher>(init);
     fetcher->SetResourceLoadObserver(
         MakeGarbageCollected<ResourceLoadObserverForWorker>(

diff --git a/third_party/blink/renderer/platform/loader/fetch/console_logger.h b/third_party/blink/renderer/platform/loader/fetch/console_logger.h
@@ -13,6 +13,9 @@
 namespace blink {
 
 // A pure virtual interface for console logging.
+// Retaining an instance of ConsoleLogger may be dangerous because after the
+// associated fetcher is detached it leads to a leak. Use
+// DetachableConsoleLogger in such a case.
 class PLATFORM_EXPORT ConsoleLogger : public GarbageCollectedMixin {
  public:
   ConsoleLogger() = default;
@@ -23,18 +26,37 @@ class PLATFORM_EXPORT ConsoleLogger : public GarbageCollectedMixin {
                                  const String& message) = 0;
 };
 
-class PLATFORM_EXPORT NullConsoleLogger final
-    : public GarbageCollected<NullConsoleLogger>,
+// A ConsoleLogger subclass which has Detach() method. An instance of
+// DetachableConsoleLogger can be kept even when the associated fetcher is
+// detached.
+class PLATFORM_EXPORT DetachableConsoleLogger final
+    : public GarbageCollected<DetachableConsoleLogger>,
       public ConsoleLogger {
-  USING_GARBAGE_COLLECTED_MIXIN(NullConsoleLogger);
+  USING_GARBAGE_COLLECTED_MIXIN(DetachableConsoleLogger);
 
  public:
-  NullConsoleLogger() = default;
-  ~NullConsoleLogger() override = default;
+  DetachableConsoleLogger() : DetachableConsoleLogger(nullptr) {}
+  DetachableConsoleLogger(ConsoleLogger* logger) : logger_(logger) {}
 
-  void AddConsoleMessage(mojom::ConsoleMessageSource,
-                         mojom::ConsoleMessageLevel,
-                         const String&) override {}
+  // Detaches |logger_|. After this function is called AddConsoleMessage will
+  // be no-op.
+  void Detach() { logger_ = nullptr; }
+
+  void AddConsoleMessage(mojom::ConsoleMessageSource source,
+                         mojom::ConsoleMessageLevel level,
+                         const String& message) override {
+    if (!logger_) {
+      return;
+    }
+    logger_->AddConsoleMessage(source, level, message);
+  }
+
+  void Trace(Visitor* visitor) override {
+    visitor->Trace(logger_);
+    ConsoleLogger::Trace(visitor);
+  }
+
+  Member<ConsoleLogger> logger_;
 };
 
 }  // namespace blink

diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.cc
@@ -401,33 +401,6 @@ mojom::RequestContextType ResourceFetcher::DetermineRequestContext(
   return mojom::RequestContextType::SUBRESOURCE;
 }
 
-class ResourceFetcher::DetachableConsoleLogger final
-    : public GarbageCollectedFinalized<DetachableConsoleLogger>,
-      public ConsoleLogger {
-  USING_GARBAGE_COLLECTED_MIXIN(DetachableConsoleLogger);
-
- public:
-  DetachableConsoleLogger(ConsoleLogger& logger) : logger_(logger) {}
-
-  void Detach() { logger_ = nullptr; }
-
-  // ConsoleLogger implementation.
-  void AddConsoleMessage(mojom::ConsoleMessageSource source,
-                         mojom::ConsoleMessageLevel level,
-                         const String& message) override {
-    if (logger_) {
-      logger_->AddConsoleMessage(source, level, message);
-    }
-  }
-  void Trace(Visitor* visitor) override {
-    visitor->Trace(logger_);
-    ConsoleLogger::Trace(visitor);
-  }
-
- private:
-  Member<ConsoleLogger> logger_;
-};
-
 // A delegating ResourceFetcherProperties subclass which can be from the
 // original ResourceFetcherProperties.
 class ResourceFetcher::DetachableProperties final
@@ -618,9 +591,9 @@ ResourceFetcher::ResourceFetcher(const ResourceFetcherInit& init)
           *MakeGarbageCollected<DetachableProperties>(*init.properties)),
       context_(init.context),
       task_runner_(init.task_runner),
-      console_logger_(MakeGarbageCollected<DetachableConsoleLogger>(
-          init.console_logger ? *init.console_logger
-                              : *MakeGarbageCollected<NullConsoleLogger>())),
+      console_logger_(init.console_logger
+                          ? init.console_logger.Get()
+                          : MakeGarbageCollected<DetachableConsoleLogger>()),
       loader_factory_(init.loader_factory),
       scheduler_(MakeGarbageCollected<ResourceLoadScheduler>(
           init.initial_throttling_policy,
@@ -1777,10 +1750,6 @@ void ResourceFetcher::ClearContext() {
   }
 }
 
-ConsoleLogger& ResourceFetcher::GetConsoleLogger() {
-  return *console_logger_;
-}
-
 int ResourceFetcher::BlockingRequestCount() const {
   return loaders_.size();
 }

diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h b/third_party/blink/renderer/platform/loader/fetch/resource_fetcher.h
@@ -50,7 +50,7 @@ namespace blink {
 
 enum class ResourceType : uint8_t;
 class CodeCacheLoader;
-class ConsoleLogger;
+class DetachableConsoleLogger;
 class FetchContext;
 class FrameScheduler;
 class MHTMLArchive;
@@ -170,7 +170,7 @@ class PLATFORM_EXPORT ResourceFetcher
 
   FetchContext& Context() const;
   void ClearContext();
-  ConsoleLogger& GetConsoleLogger();
+  DetachableConsoleLogger& GetConsoleLogger() { return *console_logger_; }
 
   int BlockingRequestCount() const;
   int NonblockingRequestCount() const;
@@ -268,7 +268,6 @@ class PLATFORM_EXPORT ResourceFetcher
 
  private:
   friend class ResourceCacheValidationSuppressor;
-  class DetachableConsoleLogger;
   class DetachableProperties;
   enum class StopFetchingTarget {
     kExcludingKeepaliveLoaders,
@@ -465,7 +464,7 @@ struct PLATFORM_EXPORT ResourceFetcherInit final {
   const Member<FetchContext> context;
   const scoped_refptr<base::SingleThreadTaskRunner> task_runner;
   const Member<ResourceFetcher::LoaderFactory> loader_factory;
-  Member<ConsoleLogger> console_logger;
+  Member<DetachableConsoleLogger> console_logger;
   ResourceLoadScheduler::ThrottlingPolicy initial_throttling_policy =
       ResourceLoadScheduler::ThrottlingPolicy::kNormal;
   Member<MHTMLArchive> archive;

diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_load_scheduler.cc b/third_party/blink/renderer/platform/loader/fetch/resource_load_scheduler.cc
@@ -353,7 +353,7 @@ ResourceLoadScheduler::ResourceLoadScheduler(
     ThrottlingPolicy initial_throttling_policy,
     const ResourceFetcherProperties& resource_fetcher_properties,
     FrameScheduler* frame_scheduler,
-    ConsoleLogger& console_logger)
+    DetachableConsoleLogger& console_logger)
     : resource_fetcher_properties_(resource_fetcher_properties),
       policy_(initial_throttling_policy),
       outstanding_limit_for_throttled_frame_scheduler_(

diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_load_scheduler.h b/third_party/blink/renderer/platform/loader/fetch/resource_load_scheduler.h
@@ -17,7 +17,7 @@
 
 namespace blink {
 
-class ConsoleLogger;
+class DetachableConsoleLogger;
 class ResourceFetcherProperties;
 
 // Client interface to use the throttling/scheduling functionality that
@@ -149,7 +149,7 @@ class PLATFORM_EXPORT ResourceLoadScheduler final
   ResourceLoadScheduler(ThrottlingPolicy initial_throttling_poilcy,
                         const ResourceFetcherProperties&,
                         FrameScheduler*,
-                        ConsoleLogger& console_logger);
+                        DetachableConsoleLogger& console_logger);
   ~ResourceLoadScheduler() override;
 
   void Trace(blink::Visitor*);
@@ -339,7 +339,7 @@ class PLATFORM_EXPORT ResourceLoadScheduler final
   std::unique_ptr<FrameScheduler::LifecycleObserverHandle>
       scheduler_observer_handle_;
 
-  const Member<ConsoleLogger> console_logger_;
+  const Member<DetachableConsoleLogger> console_logger_;
 
   DISALLOW_COPY_AND_ASSIGN(ResourceLoadScheduler);
 };

diff --git a/third_party/blink/renderer/platform/loader/fetch/resource_load_scheduler_test.cc b/third_party/blink/renderer/platform/loader/fetch/resource_load_scheduler_test.cc
@@ -58,8 +58,8 @@ class MockClient final : public GarbageCollectedFinalized<MockClient>,
   }
 
  private:
-  Member<ConsoleLogger> console_logger_ =
-      MakeGarbageCollected<NullConsoleLogger>();
+  Member<DetachableConsoleLogger> console_logger_ =
+      MakeGarbageCollected<DetachableConsoleLogger>();
   MockClientDelegate* delegate_;
   bool was_run_ = false;
 };
@@ -92,7 +92,8 @@ class ResourceLoadSchedulerTest : public testing::Test {
     console_logger_ = MakeGarbageCollected<MockConsoleLogger>();
     scheduler_ = MakeGarbageCollected<ResourceLoadScheduler>(
         ResourceLoadScheduler::ThrottlingPolicy::kTight, *properties,
-        frame_scheduler.get(), *console_logger_);
+        frame_scheduler.get(),
+        *MakeGarbageCollected<DetachableConsoleLogger>(console_logger_));
     Scheduler()->SetOutstandingLimitForTesting(1);
   }
   void TearDown() override { Scheduler()->Shutdown(); }