[Merge-100][SH] Expect 'NotGenerated' failure when requesting selector.

Sometime browser side request a selector when it has never been generated. We know this is not due to recent changes as RecordSelectorStateUma recorded this case before, however, it was never reproduced. (cherry picked from commit 92e2d6a) Bug: 1301794 Change-Id: Ic05b27225ec0e2791a1d9c2174e21cca571e6685 Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3498437 Reviewed-by: David Bokan <bokan@chromium.org> Reviewed-by: Daniel Cheng <dcheng@chromium.org> Commit-Queue: Gayane Petrosyan <gayane@google.com> Cr-Original-Commit-Position: refs/heads/main@{#979810} Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3523141 Cr-Commit-Position: refs/branch-heads/4896@{#559} Cr-Branched-From: 1f63ff4-refs/heads/main@{#972766}
chromium · Mar 15, 2022 · 70b84e1 · 70b84e1
1 parent f2d6557
commit 70b84e1
Show file tree

Hide file tree

Showing 7 changed files with 56 additions and 11 deletions.
diff --git a/components/shared_highlighting/core/common/shared_highlighting_metrics.h b/components/shared_highlighting/core/common/shared_highlighting_metrics.h
@@ -73,7 +73,11 @@ enum class LinkGenerationError {
   // cannot be established. Android only.
   kNoRemoteConnection = 13,
 
-  kMaxValue = kNoRemoteConnection
+  // TODO(crbug.com/1301794): This shouldn't happen, but sometimes browser side
+  // requests link to text when generation was never started.
+  kNotGenerated = 14,
+
+  kMaxValue = kNotGenerated
 };
 
 // These values are persisted to logs. Entries should not be renumbered and

diff --git a/third_party/blink/common/link_to_text/link_to_text_mojom_traits.cc b/third_party/blink/common/link_to_text/link_to_text_mojom_traits.cc
@@ -41,6 +41,8 @@ EnumTraits<blink::mojom::LinkGenerationError,
       return blink::mojom::LinkGenerationError::kBlockList;
     case shared_highlighting::LinkGenerationError::kNoRemoteConnection:
       return blink::mojom::LinkGenerationError::kNoRemoteConnection;
+    case shared_highlighting::LinkGenerationError::kNotGenerated:
+      return blink::mojom::LinkGenerationError::kNotGenerated;
   }
 
   NOTREACHED();
@@ -97,6 +99,9 @@ bool EnumTraits<blink::mojom::LinkGenerationError,
     case blink::mojom::LinkGenerationError::kNoRemoteConnection:
       *output = shared_highlighting::LinkGenerationError::kNoRemoteConnection;
       return true;
+    case blink::mojom::LinkGenerationError::kNotGenerated:
+      *output = shared_highlighting::LinkGenerationError::kNotGenerated;
+      return true;
   }
 
   NOTREACHED();

diff --git a/third_party/blink/public/mojom/link_to_text/link_to_text.mojom b/third_party/blink/public/mojom/link_to_text/link_to_text.mojom
@@ -29,7 +29,8 @@ enum LinkGenerationError {
   kIFrame = 10,
   kTimeout = 11,
   kBlockList = 12,
-  kNoRemoteConnection = 13
+  kNoRemoteConnection = 13,
+  kNotGenerated = 14
 };
 
 // TextFragmentReceiver is used for requesting renderer to perform text fragment

diff --git a/third_party/blink/renderer/core/fragment_directive/text_fragment_handler.cc b/third_party/blink/renderer/core/fragment_directive/text_fragment_handler.cc
@@ -46,20 +46,29 @@ void TextFragmentHandler::RequestSelector(RequestSelectorCallback callback) {
   DCHECK(shared_highlighting::ShouldOfferLinkToText(
       GetFrame()->GetDocument()->Url()));
   DCHECK(!GetFrame()->Selection().SelectedText().IsEmpty());
-  DCHECK(GetTextFragmentSelectorGenerator());
-
-  GetTextFragmentSelectorGenerator()->RecordSelectorStateUma();
 
+  response_callback_ = std::move(callback);
   selector_ready_status_ =
       preemptive_generation_result_.has_value()
           ? shared_highlighting::LinkGenerationReadyStatus::kRequestedAfterReady
           : shared_highlighting::LinkGenerationReadyStatus::
                 kRequestedBeforeReady;
-  response_callback_ = std::move(callback);
 
-  // If generation finished simply
-  // respond with the result. Otherwise, the response callback is stored so
-  // that we reply on completion.
+  if (!GetTextFragmentSelectorGenerator()) {
+    // TODO(crbug.com/1303881): This shouldn't happen, but sometimes browser
+    // side requests link to text when generation was never started.
+    // See crash in crbug.com/1301794.
+    error_ = shared_highlighting::LinkGenerationError::kNotGenerated;
+    InvokeReplyCallback(
+        TextFragmentSelector(TextFragmentSelector::SelectorType::kInvalid),
+        error_);
+    return;
+  }
+
+  GetTextFragmentSelectorGenerator()->RecordSelectorStateUma();
+
+  // If generation finished simply respond with the result. Otherwise, the
+  // response callback is stored so that we reply on completion.
   if (selector_ready_status_.value() ==
       shared_highlighting::LinkGenerationReadyStatus::kRequestedAfterReady)
     InvokeReplyCallback(preemptive_generation_result_.value(), error_);
@@ -240,7 +249,8 @@ void TextFragmentHandler::InvokeReplyCallback(
       .Run(selector.ToString(), error, selector_ready_status_.value());
 
   // After reply is sent it is safe to reset the generator.
-  GetTextFragmentSelectorGenerator()->Reset();
+  if (GetTextFragmentSelectorGenerator())
+    GetTextFragmentSelectorGenerator()->Reset();
 }
 
 TextFragmentAnchor* TextFragmentHandler::GetTextFragmentAnchor() {

diff --git a/third_party/blink/renderer/core/fragment_directive/text_fragment_handler.h b/third_party/blink/renderer/core/fragment_directive/text_fragment_handler.h
@@ -60,7 +60,7 @@ class CORE_EXPORT TextFragmentHandler final
  private:
   FRIEND_TEST_ALL_PREFIXES(TextFragmentHandlerTest,
                            IfGeneratorResetShouldRecordCorrectError);
-
+  FRIEND_TEST_ALL_PREFIXES(TextFragmentHandlerTest, NotGenerated);
   // Returns whether preemptive generation should run for the given frame.
   static bool ShouldPreemptivelyGenerateFor(LocalFrame* frame);
 

diff --git a/third_party/blink/renderer/core/fragment_directive/text_fragment_handler_test.cc b/third_party/blink/renderer/core/fragment_directive/text_fragment_handler_test.cc
@@ -1145,4 +1145,28 @@ TEST_F(TextFragmentHandlerTest, IfGeneratorResetShouldRecordCorrectError) {
   EXPECT_EQ(expected_error, GetTextFragmentHandler().error_);
 }
 
+// crbug.com/1301794 If generation didn't start requesting selector shouldn't
+// crash.
+TEST_F(TextFragmentHandlerTest, NotGenerated) {
+  SimRequest request("https://example.com/test.html", "text/html");
+  LoadURL("https://example.com/test.html");
+  request.Complete(R"HTML(
+    <!DOCTYPE html>
+    <div>Test page</div>
+    <p id='first'>First paragraph text that is longer than 20 chars</p>
+    <p id='second'>Second paragraph text</p>
+  )HTML");
+
+  Node* first_paragraph = GetDocument().getElementById("first")->firstChild();
+  const auto& selected_start = Position(first_paragraph, 5);
+  const auto& selected_end = Position(first_paragraph, 6);
+  ASSERT_EQ(" ", PlainText(EphemeralRange(selected_start, selected_end)));
+
+  SetSelection(selected_start, selected_end);
+  EXPECT_EQ(RequestSelector(), "");
+
+  shared_highlighting::LinkGenerationError expected_error =
+      shared_highlighting::LinkGenerationError::kNotGenerated;
+  EXPECT_EQ(expected_error, GetTextFragmentHandler().error_);
+}
 }  // namespace blink
diff --git a/tools/metrics/histograms/enums.xml b/tools/metrics/histograms/enums.xml
@@ -50291,6 +50291,7 @@ Called by update_gpu_driver_bug_workaround_entries.py.-->
   <int value="11" label="Timeout"/>
   <int value="12" label="BlockList"/>
   <int value="13" label="NoRemoteConnection"/>
+  <int value="14" label="NotGenerated"/>
 </enum>
 
 <enum name="LinkGenerationSharedStatus">