ContentCapture: fix a crash

The WebContentCaptureClient could be set to null when RenderFrameImpl::WasShown() is called, instead of, accessing content_capture_manager_ directly, GetContentCaptureManager() which check WebContentCaptureClient availability should be used. This patch also renames GetContentCaptureManager() to GetOrResetContentCaptureManager() (cherry picked from commit e5c09ff) Bug: 1312504 Change-Id: I855cb90ac11d3d13729b7e4d20bfc56af5bce5bf Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3572581 Reviewed-by: Xianzhu Wang <wangxianzhu@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Philip Rogers <pdr@chromium.org> Commit-Queue: Michael Bai <michaelbai@chromium.org> Cr-Original-Commit-Position: refs/heads/main@{#992764} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3591296 Auto-Submit: Michael Bai <michaelbai@chromium.org> Bot-Commit: Rubber Stamper <rubber-stamper@appspot.gserviceaccount.com> Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org> Cr-Commit-Position: refs/branch-heads/5005@{#25} Cr-Branched-From: 5b4d945-refs/heads/main@{#992738}
chromium · Apr 19, 2022 · 493c44d · 493c44d
1 parent 0cb27cb
commit 493c44d
Show file tree

Hide file tree

Showing 8 changed files with 50 additions and 40 deletions.
diff --git a/third_party/blink/renderer/core/content_capture/content_capture_task.cc b/third_party/blink/renderer/core/content_capture/content_capture_task.cc
@@ -50,6 +50,7 @@ ContentCaptureTask::ContentCaptureTask(LocalFrame& local_frame_root,
           local_frame_root_->GetTaskRunner(TaskType::kInternalContentCapture),
           this,
           &ContentCaptureTask::Run) {
+  DCHECK(local_frame_root.Client()->GetWebContentCaptureClient());
   task_delay_ = std::make_unique<TaskDelay>(local_frame_root.Client()
                                                 ->GetWebContentCaptureClient()
                                                 ->GetTaskInitialDelay());

diff --git a/third_party/blink/renderer/core/content_capture/content_capture_test.cc b/third_party/blink/renderer/core/content_capture/content_capture_test.cc
@@ -185,18 +185,18 @@ class ContentCaptureTest
     InitNodeHolders();
     // Setup captured content to ContentCaptureTask, it isn't necessary once
     // ContentCaptureManager is created by LocalFrame.
-    GetContentCaptureManager()
+    GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->SetCapturedContentForTesting(node_ids_);
     InitScrollingTestData();
   }
 
   void SimulateScrolling(wtf_size_t step) {
     CHECK_LT(step, 4u);
-    GetContentCaptureManager()
+    GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->SetCapturedContentForTesting(scrolling_node_ids_[step]);
-    GetContentCaptureManager()->OnScrollPositionChanged();
+    GetOrResetContentCaptureManager()->OnScrollPositionChanged();
   }
 
   void CreateTextNodeAndNotifyManager() {
@@ -207,18 +207,18 @@ class ContentCaptureTest
     Element* div_element = GetElementById("d1");
     div_element->appendChild(element);
     UpdateAllLifecyclePhasesForTest();
-    GetContentCaptureManager()->ScheduleTaskIfNeeded(*node);
+    GetOrResetContentCaptureManager()->ScheduleTaskIfNeeded(*node);
     created_node_id_ = DOMNodeIds::IdForNode(node);
     Vector<cc::NodeInfo> captured_content{
         cc::NodeInfo(created_node_id_, GetRect(node->GetLayoutObject()))};
-    GetContentCaptureManager()
+    GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->SetCapturedContentForTesting(captured_content);
   }
 
-  ContentCaptureManager* GetContentCaptureManager() {
+  ContentCaptureManager* GetOrResetContentCaptureManager() {
     if (content_capture_manager_ == nullptr)
-      content_capture_manager_ = GetFrame().GetContentCaptureManager();
+      content_capture_manager_ = GetFrame().GetOrResetContentCaptureManager();
     return content_capture_manager_;
   }
 
@@ -227,7 +227,7 @@ class ContentCaptureTest
   }
 
   ContentCaptureTask* GetContentCaptureTask() {
-    return GetContentCaptureManager()->GetContentCaptureTaskForTesting();
+    return GetOrResetContentCaptureManager()->GetContentCaptureTaskForTesting();
   }
 
   void RunContentCaptureTask() {
@@ -245,7 +245,7 @@ class ContentCaptureTest
   void RemoveNode(Node* node) {
     // Remove the node.
     node->remove();
-    GetContentCaptureManager()->OnLayoutTextWillBeDestroyed(*node);
+    GetOrResetContentCaptureManager()->OnLayoutTextWillBeDestroyed(*node);
   }
 
   void RemoveUnsentNode(const WebVector<WebContentHolder>& sent_nodes) {
@@ -300,7 +300,7 @@ class ContentCaptureTest
       CHECK(layout_object);
       CHECK(layout_object->IsText());
       nodes.push_back(node);
-      GetContentCaptureManager()->ScheduleTaskIfNeeded(*node);
+      GetOrResetContentCaptureManager()->ScheduleTaskIfNeeded(*node);
       node_ids.push_back(
           cc::NodeInfo(DOMNodeIds::IdForNode(node), GetRect(layout_object)));
     }
@@ -444,7 +444,7 @@ TEST_P(ContentCaptureTest, NodeOnlySendOnce) {
   EXPECT_EQ(GetExpectedSecondResultSize(),
             GetWebContentCaptureClient()->Data().size());
 
-  GetContentCaptureManager()->OnScrollPositionChanged();
+  GetOrResetContentCaptureManager()->OnScrollPositionChanged();
   RunContentCaptureTask();
   EXPECT_TRUE(GetWebContentCaptureClient()->Data().empty());
   EXPECT_TRUE(GetWebContentCaptureClient()->RemovedData().empty());
@@ -459,15 +459,16 @@ TEST_P(ContentCaptureTest, UnsentNode) {
 
   // Simulates the |invisible_node_| being changed, and verifies no content
   // change because |invisible_node_| wasn't captured.
-  GetContentCaptureManager()->OnNodeTextChanged(invisible_node());
+  GetOrResetContentCaptureManager()->OnNodeTextChanged(invisible_node());
   RunContentCaptureTask();
   EXPECT_TRUE(GetWebContentCaptureClient()->Data().empty());
   EXPECT_TRUE(GetWebContentCaptureClient()->UpdatedData().empty());
   EXPECT_TRUE(GetWebContentCaptureClient()->RemovedData().empty());
 
   // Simulates the |invisible_node_| being removed, and verifies no content
   // change because |invisible_node_| wasn't captured.
-  GetContentCaptureManager()->OnLayoutTextWillBeDestroyed(invisible_node());
+  GetOrResetContentCaptureManager()->OnLayoutTextWillBeDestroyed(
+      invisible_node());
   RunContentCaptureTask();
   EXPECT_TRUE(GetWebContentCaptureClient()->Data().empty());
   EXPECT_TRUE(GetWebContentCaptureClient()->UpdatedData().empty());
@@ -729,12 +730,12 @@ class ContentCaptureSimTest : public SimTest {
     GetDocument()
         .GetFrame()
         ->LocalFrameRoot()
-        .GetContentCaptureManager()
+        .GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->RunTaskForTestingUntil(state);
     // Cancels the scheduled task to simulate that the task is running by
     // scheduler.
-    GetContentCaptureManager()
+    GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->CancelTaskForTesting();
   }
@@ -788,31 +789,31 @@ class ContentCaptureSimTest : public SimTest {
                  main_frame_expected_text_.end(), old_text, new_text);
   }
 
-  ContentCaptureManager* GetContentCaptureManager() {
+  ContentCaptureManager* GetOrResetContentCaptureManager() {
     return DynamicTo<LocalFrame>(LocalFrameRoot().GetFrame())
-        ->GetContentCaptureManager();
+        ->GetOrResetContentCaptureManager();
   }
 
   void SimulateUserInputOnMainFrame() {
-    GetContentCaptureManager()->NotifyInputEvent(
+    GetOrResetContentCaptureManager()->NotifyInputEvent(
         WebInputEvent::Type::kMouseDown,
         *DynamicTo<LocalFrame>(MainFrame().GetFrame()));
   }
 
   void SimulateUserInputOnChildFrame() {
-    GetContentCaptureManager()->NotifyInputEvent(
+    GetOrResetContentCaptureManager()->NotifyInputEvent(
         WebInputEvent::Type::kMouseDown, *child_document_->GetFrame());
   }
 
   base::TimeDelta GetNextTaskDelay() {
-    return GetContentCaptureManager()
+    return GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->GetTaskDelayForTesting()
         .GetNextTaskDelay();
   }
 
   base::TimeDelta GetTaskNextFireInterval() {
-    return GetContentCaptureManager()
+    return GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->GetTaskNextFireIntervalForTesting();
   }
@@ -926,7 +927,7 @@ class ContentCaptureSimTest : public SimTest {
     GetDocument()
         .GetFrame()
         ->LocalFrameRoot()
-        .GetContentCaptureManager()
+        .GetOrResetContentCaptureManager()
         ->GetContentCaptureTaskForTesting()
         ->SetCapturedContentForTesting(captured_content);
   }

diff --git a/third_party/blink/renderer/core/frame/local_frame.cc b/third_party/blink/renderer/core/frame/local_frame.cc
@@ -1861,13 +1861,17 @@ bool LocalFrame::CanNavigate(const Frame& target_frame,
   return CanNavigateHelper(*this, *this, target_frame, destination_url);
 }
 
-ContentCaptureManager* LocalFrame::GetContentCaptureManager() {
+ContentCaptureManager* LocalFrame::GetOrResetContentCaptureManager() {
   DCHECK(Client());
   if (!IsLocalRoot())
     return nullptr;
 
-  // TODO(dcheng): Why does this function also Shutdown()? It seems rather
-  // surprising...
+  // WebContentCaptureClient is set on each navigation and it could become null
+  // because the url is in disallowed list, so ContentCaptureManager
+  // is created or released as needed to save the resources.
+  // It is a little bit odd that ContentCaptureManager is created or released on
+  // demand, and that this is something that could be improved with an explicit
+  // signal for creating / destroying content capture managers.
   if (auto* content_capture_client = Client()->GetWebContentCaptureClient()) {
     if (!content_capture_manager_) {
       content_capture_manager_ =
@@ -2003,8 +2007,8 @@ void LocalFrame::WasHidden() {
     return;
   hidden_ = true;
 
-  if (content_capture_manager_) {
-    content_capture_manager_->OnFrameWasHidden();
+  if (auto* content_capture_manager = GetOrResetContentCaptureManager()) {
+    content_capture_manager->OnFrameWasHidden();
   }
 
   // An iframe may get a "was hidden" notification before it has been attached
@@ -2045,8 +2049,8 @@ void LocalFrame::WasShown() {
   if (LocalFrameView* frame_view = View())
     frame_view->ScheduleAnimation();
 
-  if (content_capture_manager_) {
-    content_capture_manager_->OnFrameWasShown();
+  if (auto* content_capture_manager = GetOrResetContentCaptureManager()) {
+    content_capture_manager->OnFrameWasShown();
   }
 }
 

diff --git a/third_party/blink/renderer/core/frame/local_frame.h b/third_party/blink/renderer/core/frame/local_frame.h
@@ -276,8 +276,9 @@ class CORE_EXPORT LocalFrame final
   CoreProbeSink* GetProbeSink() { return probe_sink_.Get(); }
   scoped_refptr<InspectorTaskRunner> GetInspectorTaskRunner();
 
-  // Returns ContentCaptureManager in LocalFrameRoot.
-  ContentCaptureManager* GetContentCaptureManager();
+  // Returns ContentCaptureManager in LocalFrameRoot, create or destroy it as
+  // needed.
+  ContentCaptureManager* GetOrResetContentCaptureManager();
 
   // Returns the current state of caret browsing mode.
   bool IsCaretBrowsingEnabled() const;
@@ -878,6 +879,8 @@ class CORE_EXPORT LocalFrame final
   // SmoothScrollSequencer is only populated for local roots; all local frames
   // use the instance owned by their local root.
   Member<SmoothScrollSequencer> smooth_scroll_sequencer_;
+  // Access content_capture_manager_ through GetOrResetContentCaptureManager()
+  // because WebContentCaptureClient might already stop the capture.
   Member<ContentCaptureManager> content_capture_manager_;
 
   InterfaceRegistry* const interface_registry_;

diff --git a/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc b/third_party/blink/renderer/core/frame/web_frame_widget_impl.cc
@@ -2513,7 +2513,7 @@ WebInputEventResult WebFrameWidgetImpl::HandleInputEvent(
   Frame* frame = FocusedCoreFrame();
   if (auto* local_frame = DynamicTo<LocalFrame>(frame)) {
     if (auto* content_capture_manager =
-            local_frame->LocalFrameRoot().GetContentCaptureManager()) {
+            local_frame->LocalFrameRoot().GetOrResetContentCaptureManager()) {
       content_capture_manager->NotifyInputEvent(input_event.GetType(),
                                                 *local_frame);
     }

diff --git a/third_party/blink/renderer/core/layout/layout_text.cc b/third_party/blink/renderer/core/layout/layout_text.cc
@@ -303,7 +303,7 @@ void LayoutText::WillBeDestroyed() {
   GetSelectionDisplayItemClientMap().erase(this);
 
   if (node_id_ != kInvalidDOMNodeId) {
-    if (auto* manager = GetContentCaptureManager())
+    if (auto* manager = GetOrResetContentCaptureManager())
       manager->OnLayoutTextWillBeDestroyed(*GetNode());
     node_id_ = kInvalidDOMNodeId;
   }
@@ -2241,7 +2241,7 @@ void LayoutText::TextDidChangeWithoutInvalidation() {
     text_autosizer->Record(this);
 
   if (HasNodeId()) {
-    if (auto* content_capture_manager = GetContentCaptureManager())
+    if (auto* content_capture_manager = GetOrResetContentCaptureManager())
       content_capture_manager->OnNodeTextChanged(*GetNode());
   }
 
@@ -2871,7 +2871,7 @@ PhysicalRect LayoutText::DebugRect() const {
 DOMNodeId LayoutText::EnsureNodeId() {
   NOT_DESTROYED();
   if (node_id_ == kInvalidDOMNodeId) {
-    if (auto* content_capture_manager = GetContentCaptureManager()) {
+    if (auto* content_capture_manager = GetOrResetContentCaptureManager()) {
       if (auto* node = GetNode()) {
         content_capture_manager->ScheduleTaskIfNeeded(*node);
         node_id_ = DOMNodeIds::IdForNode(node);
@@ -2881,11 +2881,11 @@ DOMNodeId LayoutText::EnsureNodeId() {
   return node_id_;
 }
 
-ContentCaptureManager* LayoutText::GetContentCaptureManager() {
+ContentCaptureManager* LayoutText::GetOrResetContentCaptureManager() {
   NOT_DESTROYED();
   if (auto* node = GetNode()) {
     if (auto* frame = node->GetDocument().GetFrame()) {
-      return frame->LocalFrameRoot().GetContentCaptureManager();
+      return frame->LocalFrameRoot().GetOrResetContentCaptureManager();
     }
   }
   return nullptr;

diff --git a/third_party/blink/renderer/core/layout/layout_text.h b/third_party/blink/renderer/core/layout/layout_text.h
@@ -576,7 +576,7 @@ class CORE_EXPORT LayoutText : public LayoutObject {
   unsigned is_text_fragment_ : 1;
 
  private:
-  ContentCaptureManager* GetContentCaptureManager();
+  ContentCaptureManager* GetOrResetContentCaptureManager();
   void DetachAbstractInlineTextBoxes();
 
   // Used for LayoutNG with accessibility. True if inline fragments are

diff --git a/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc b/third_party/blink/renderer/core/paint/paint_layer_scrollable_area.cc
@@ -525,8 +525,9 @@ void PaintLayerScrollableArea::UpdateScrollOffset(
       ShowNonMacOverlayScrollbars();
     GetScrollAnchor()->Clear();
   }
-  if (ContentCaptureManager* manager =
-          frame_view->GetFrame().LocalFrameRoot().GetContentCaptureManager()) {
+  if (ContentCaptureManager* manager = frame_view->GetFrame()
+                                           .LocalFrameRoot()
+                                           .GetOrResetContentCaptureManager()) {
     manager->OnScrollPositionChanged();
   }
   if (AXObjectCache* cache =