Skip to content

Commit

Permalink
shared-highlighting: Fix null LayoutObject crash
Browse files Browse the repository at this point in the history 
This crash occurs in NextVisibleTextNodeWithinBlock when the
LayoutObject associated with start_node is removed between
TextFragmentSelectorGenerator runs.

Bug: 1313253
Change-Id: Ia93fccf72beaa80b35ea633572fc923c3fe28318
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3774038
Reviewed-by: Koji Ishii <kojii@chromium.org>
Commit-Queue: David Bokan <bokan@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1026211}
  • Loading branch information
@bokand
bokand authored and Chromium LUCI CQ committed Jul 20, 2022
1 parent 39c7e1a commit 136a29e
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 57 deletions.
3 changes: 3 additions & 0 deletions third_party/blink/renderer/core/fragment_directive/same_block_word_iterator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,9 @@ bool SameBlockWordIterator<Direction>::NextNode() {
template <typename Direction>
Node* SameBlockWordIterator<Direction>::NextVisibleTextNodeWithinBlock(
Node& start_node) {
if (!start_node.GetLayoutObject())
return nullptr;

// Move forward/backward until no next/previous node is available within same
// |block_ancestor|.
Node* node = &start_node;
Expand Down
57 changes: 36 additions & 21 deletions third_party/blink/renderer/core/fragment_directive/text_fragment_finder_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
#include "third_party/blink/renderer/core/testing/sim/sim_request.h"
#include "third_party/blink/renderer/core/testing/sim/sim_test.h"

using testing::_;
using testing::Mock;

namespace blink {

class MockTextFragmentFinder : public TextFragmentFinder {
Expand All @@ -20,21 +23,21 @@ class MockTextFragmentFinder : public TextFragmentFinder {
void GoToStep(SelectorMatchStep step) override { step_ = step; }
};

class TextFragmentFinderTest : public SimTest,
public TextFragmentFinder::Client {
class MockTextFragmentFinderClient : public TextFragmentFinder::Client {
public:
MOCK_METHOD(void,
DidFindMatch,
(const RangeInFlatTree& match, bool is_unique),
(override));
MOCK_METHOD(void, NoMatchFound, (), (override));
};

class TextFragmentFinderTest : public SimTest {
public:
void SetUp() override {
SimTest::SetUp();
WebView().MainFrameViewWidget()->Resize(gfx::Size(800, 600));
}

void NoMatchFound() override { no_match_called_ = true; }

void DidFindMatch(const RangeInFlatTree& match, bool is_unique) override {}
bool IsNoMatchFoundCalled() { return no_match_called_; }

private:
bool no_match_called_ = false;
};

// Tests that Find tasks will fail gracefully when DOM mutations invalidate the
Expand All @@ -52,23 +55,35 @@ TEST_F(TextFragmentFinderTest, DOMMutation) {
"First paragraph", "", "button text",
"prefix to unique");

MockTextFragmentFinderClient client;

MockTextFragmentFinder* finder = MakeGarbageCollected<MockTextFragmentFinder>(
*this, selector, &GetDocument(),
client, selector, &GetDocument(),
TextFragmentFinder::FindBufferRunnerType::kSynchronous);
finder->FindMatch();
EXPECT_CALL(client, DidFindMatch(_, _)).Times(0);

finder->FindPrefix();
EXPECT_EQ(false, IsNoMatchFoundCalled());
{
EXPECT_CALL(client, NoMatchFound()).Times(0);
finder->FindMatch();
finder->FindPrefix();
Mock::VerifyAndClearExpectations(&client);
}

finder->FindTextStart();
EXPECT_EQ(false, IsNoMatchFoundCalled());
{
EXPECT_CALL(client, NoMatchFound()).Times(0);
finder->FindTextStart();
Mock::VerifyAndClearExpectations(&client);
}

Node* input = GetDocument().getElementById("input");
input->remove();
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kTest);
{
EXPECT_CALL(client, NoMatchFound()).Times(1);
Node* input = GetDocument().getElementById("input");
input->remove();
GetDocument().UpdateStyleAndLayout(DocumentUpdateReason::kTest);

finder->FindSuffix();
EXPECT_EQ(true, IsNoMatchFoundCalled());
finder->FindSuffix();
Mock::VerifyAndClearExpectations(&client);
}
}

} // namespace blink
3 changes: 3 additions & 0 deletions third_party/blink/renderer/core/fragment_directive/text_fragment_selector_generator.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -193,6 +193,9 @@ String TextFragmentSelectorGenerator::GetSelectorTargetText() const {

void TextFragmentSelectorGenerator::DidFindMatch(const RangeInFlatTree& match,
bool is_unique) {
if (did_find_match_callback_for_testing_)
std::move(did_find_match_callback_for_testing_).Run(is_unique);

if (is_unique &&
PlainText(match.ToEphemeralRange()).StripWhiteSpace().length() ==
PlainText(range_->ToEphemeralRange()).StripWhiteSpace().length()) {
Expand Down
9 changes: 7 additions & 2 deletions third_party/blink/renderer/core/fragment_directive/text_fragment_selector_generator.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -103,17 +103,18 @@ class CORE_EXPORT TextFragmentSelectorGenerator final
GetNextTextStartPosition_NoNextNode);
FRIEND_TEST_ALL_PREFIXES(TextFragmentSelectorGeneratorTest,
ExactTextSelector_Long);

FRIEND_TEST_ALL_PREFIXES(
TextFragmentSelectorGeneratorTest,
GetPreviousTextEndPosition_ShouldSkipNodesWithNoLayoutObject);
FRIEND_TEST_ALL_PREFIXES(TextFragmentSelectorGeneratorTest,
RemoveLayoutObjectAsync);

// Used for determining the next step of selector generation.
enum GenerationStep { kExact, kRange, kContext };

// Used for determining the current state of |selector_|.
enum SelectorState {
// Sreach for candidate selector didn't start.
// Search for candidate selector didn't start.
kNotStarted,

// Candidate selector should be generated or extended.
Expand Down Expand Up @@ -181,6 +182,10 @@ class CORE_EXPORT TextFragmentSelectorGenerator final

GenerateCallback pending_generate_selector_callback_;

// Callback invoked each time DidFindMatch is called; for testing only.
// Allows inserting code to run between asynchronous generation steps.
base::OnceCallback<void(bool is_unique)> did_find_match_callback_for_testing_;

GenerationStep step_ = kExact;
SelectorState state_ = kNeedsNewCandidate;

Expand Down
134 changes: 100 additions & 34 deletions third_party/blink/renderer/core/fragment_directive/text_fragment_selector_generator_test.cc
View file
Original file line number Diff line number Diff line change
Expand Up @@ -98,18 +98,17 @@ class TextFragmentSelectorGeneratorTest : public SimTest {
};
auto callback =
WTF::Bind(lambda, std::ref(callback_called), std::ref(selector));
GetTextFragmentSelectorGenerator()->Generate(
*MakeGarbageCollected<RangeInFlatTree>(
ToPositionInFlatTree(selected_start),
ToPositionInFlatTree(selected_end)),
std::move(callback));
CreateGenerator()->Generate(*MakeGarbageCollected<RangeInFlatTree>(
ToPositionInFlatTree(selected_start),
ToPositionInFlatTree(selected_end)),
std::move(callback));
base::RunLoop().RunUntilIdle();

EXPECT_TRUE(callback_called);
return selector;
}

TextFragmentSelectorGenerator* GetTextFragmentSelectorGenerator() {
TextFragmentSelectorGenerator* CreateGenerator() {
return MakeGarbageCollected<TextFragmentSelectorGenerator>(
GetDocument().GetFrame());
}
Expand Down Expand Up @@ -1443,9 +1442,8 @@ TEST_F(TextFragmentSelectorGeneratorTest, GetPreviousTextEndPosition_PrevNode) {
Node* first_paragraph = GetDocument().getElementById("first")->firstChild();
const auto& expected_position =
ToPositionInFlatTree(Position::LastPositionInNode(*first_paragraph));
EXPECT_EQ(
expected_position,
GetTextFragmentSelectorGenerator()->GetPreviousTextEndPosition(start));
EXPECT_EQ(expected_position,
CreateGenerator()->GetPreviousTextEndPosition(start));
}

// Check the case when available prefix is a text node outside of selection
Expand All @@ -1466,9 +1464,8 @@ TEST_F(TextFragmentSelectorGeneratorTest,
Node* node = GetDocument().getElementById("first")->previousSibling();
const auto& expected_position =
ToPositionInFlatTree(Position::LastPositionInNode(*node));
EXPECT_EQ(
expected_position,
GetTextFragmentSelectorGenerator()->GetPreviousTextEndPosition(start));
EXPECT_EQ(expected_position,
CreateGenerator()->GetPreviousTextEndPosition(start));
}

// Check the case when available prefix is a parent node text content outside of
Expand All @@ -1489,9 +1486,8 @@ TEST_F(TextFragmentSelectorGeneratorTest,
Node* node = GetDocument().getElementById("div")->firstChild();
const auto& expected_position =
ToPositionInFlatTree(Position::LastPositionInNode(*node));
EXPECT_EQ(
expected_position,
GetTextFragmentSelectorGenerator()->GetPreviousTextEndPosition(start));
EXPECT_EQ(expected_position,
CreateGenerator()->GetPreviousTextEndPosition(start));
}

// Check the case when previous node is used for available prefix when selection
Expand All @@ -1515,9 +1511,8 @@ TEST_F(TextFragmentSelectorGeneratorTest,
Node* node = GetDocument().getElementById("first")->firstChild();
const auto& expected_position =
ToPositionInFlatTree(Position::LastPositionInNode(*node));
EXPECT_EQ(
expected_position,
GetTextFragmentSelectorGenerator()->GetPreviousTextEndPosition(start));
EXPECT_EQ(expected_position,
CreateGenerator()->GetPreviousTextEndPosition(start));
}

// Check the case when previous node is used for available prefix when selection
Expand Down Expand Up @@ -1545,9 +1540,8 @@ TEST_F(TextFragmentSelectorGeneratorTest,
Node* node = GetDocument().getElementById("first")->firstChild();
const auto& expected_position =
ToPositionInFlatTree(Position::LastPositionInNode(*node));
EXPECT_EQ(
expected_position,
GetTextFragmentSelectorGenerator()->GetPreviousTextEndPosition(start));
EXPECT_EQ(expected_position,
CreateGenerator()->GetPreviousTextEndPosition(start));
}

// Check the case when available prefix complete text content of the previous
Expand All @@ -1566,9 +1560,8 @@ TEST_F(TextFragmentSelectorGeneratorTest,
ASSERT_EQ("First", PlainText(EphemeralRangeInFlatTree(start, end)));

PositionInFlatTree expected_position;
EXPECT_EQ(
expected_position,
GetTextFragmentSelectorGenerator()->GetPreviousTextEndPosition(start));
EXPECT_EQ(expected_position,
CreateGenerator()->GetPreviousTextEndPosition(start));
}

// Similar test for suffix.
Expand All @@ -1593,7 +1586,7 @@ TEST_F(TextFragmentSelectorGeneratorTest, GetNextTextStartPosition_NextNode) {
const auto& expected_position =
ToPositionInFlatTree(Position::FirstPositionInNode(*node));
EXPECT_EQ(expected_position,
GetTextFragmentSelectorGenerator()->GetNextTextStartPosition(end));
CreateGenerator()->GetNextTextStartPosition(end));
}

// Check the case when there is a commented block between selection and the
Expand All @@ -1620,7 +1613,7 @@ TEST_F(TextFragmentSelectorGeneratorTest,
const auto& expected_position =
ToPositionInFlatTree(Position::FirstPositionInNode(*node));
EXPECT_EQ(expected_position,
GetTextFragmentSelectorGenerator()->GetNextTextStartPosition(end));
CreateGenerator()->GetNextTextStartPosition(end));
}

// Check the case when available suffix is a text node outside of selection
Expand All @@ -1643,7 +1636,7 @@ TEST_F(TextFragmentSelectorGeneratorTest,
const auto& expected_position =
ToPositionInFlatTree(Position::FirstPositionInNode(*node));
EXPECT_EQ(expected_position,
GetTextFragmentSelectorGenerator()->GetNextTextStartPosition(end));
CreateGenerator()->GetNextTextStartPosition(end));
}

// Check the case when available suffix is a parent node text content outside of
Expand All @@ -1665,7 +1658,7 @@ TEST_F(TextFragmentSelectorGeneratorTest, GetNextTextStartPosition_ParentNode) {
const auto& expected_position =
ToPositionInFlatTree(Position::FirstPositionInNode(*node));
EXPECT_EQ(expected_position,
GetTextFragmentSelectorGenerator()->GetNextTextStartPosition(end));
CreateGenerator()->GetNextTextStartPosition(end));
}

// Check the case when next node is used for available suffix when selection is
Expand All @@ -1690,7 +1683,7 @@ TEST_F(TextFragmentSelectorGeneratorTest,
const auto& expected_position =
ToPositionInFlatTree(Position::FirstPositionInNode(*node));
EXPECT_EQ(expected_position,
GetTextFragmentSelectorGenerator()->GetNextTextStartPosition(end));
CreateGenerator()->GetNextTextStartPosition(end));
}

// Check the case when next node is used for available suffix when selection is
Expand Down Expand Up @@ -1718,7 +1711,7 @@ TEST_F(TextFragmentSelectorGeneratorTest,
const auto& expected_position =
ToPositionInFlatTree(Position::FirstPositionInNode(*node));
EXPECT_EQ(expected_position,
GetTextFragmentSelectorGenerator()->GetNextTextStartPosition(end));
CreateGenerator()->GetNextTextStartPosition(end));
}

// Check the case when available suffix is a text node outside of selection
Expand All @@ -1738,7 +1731,7 @@ TEST_F(TextFragmentSelectorGeneratorTest, GetNextTextStartPosition_NoNextNode) {

PositionInFlatTree expected_position;
EXPECT_EQ(expected_position,
GetTextFragmentSelectorGenerator()->GetNextTextStartPosition(end));
CreateGenerator()->GetNextTextStartPosition(end));
}

TEST_F(TextFragmentSelectorGeneratorTest, BeforeAndAfterAnchor) {
Expand Down Expand Up @@ -1785,9 +1778,82 @@ TEST_F(TextFragmentSelectorGeneratorTest,
Node* node = shadow1.getElementById("p")->firstChild();
const auto& expected_position =
ToPositionInFlatTree(Position::LastPositionInNode(*node));
EXPECT_EQ(
expected_position,
GetTextFragmentSelectorGenerator()->GetPreviousTextEndPosition(start));
EXPECT_EQ(expected_position,
CreateGenerator()->GetPreviousTextEndPosition(start));
}

// Tests that the generator fails gracefully if the layout subtree is removed
// while we're operating on it. Reproduction for https://crbug.com/1313253.
TEST_F(TextFragmentSelectorGeneratorTest, RemoveLayoutObjectAsync) {
SimRequest request("https://example.com/test.html", "text/html");
LoadURL("https://example.com/test.html");

// This test case relies on the initial try of 'p p p-,Foo,-s s s' being
// non-unique. This forces the generator to try expanding the context after
// the initial asynchronous search finishes. Before that happens, the current
// node's LayoutObject is removed.
request.Complete(R"HTML(
<!DOCTYPE html>
<p>p p p p</p>
<p id='target'>Foo s s s p p p Foo s s s</p>
<p>More text to so context expansion doesn't abort due to reaching end</p>
)HTML");

// Select the first instance of "Foo"
Element* target = GetDocument().getElementById("target");

Node* text = target->firstChild();
const auto& selected_start = Position(text, 0);
const auto& selected_end = Position(text, 3);
ASSERT_EQ("Foo", PlainText(EphemeralRange(selected_start, selected_end)));

String selector;
bool finished = false;
auto lambda = [](bool& callback_called, String& selector,
const TextFragmentSelector& generated_selector,
shared_highlighting::LinkGenerationError error) {
selector = generated_selector.ToString();
callback_called = true;
};
auto callback = WTF::Bind(lambda, std::ref(finished), std::ref(selector));

TextFragmentSelectorGenerator* generator = CreateGenerator();
generator->Generate(*MakeGarbageCollected<RangeInFlatTree>(
ToPositionInFlatTree(selected_start),
ToPositionInFlatTree(selected_end)),
std::move(callback));

// This test intends to test what happens when the layout tree is mutated
// while the generator is waiting for the next asynchronous step. Thus, the
// generator must still be running at this point for this test to be valid.
ASSERT_FALSE(finished);

// The TestCandidate state is the async break point in the generator, this
// will post a task in AsyncFindBuffer to perform the text search on the
// document. We'll mutate the layout tree while this task isn't run yet so
// that when it run and returns to the generator we ensure neither touches
// the removed layout tree.
EXPECT_EQ(generator->state_,
TextFragmentSelectorGenerator::SelectorState::kTestCandidate);

generator->did_find_match_callback_for_testing_ = WTF::Bind(
[](Element* target, bool is_unique) {
EXPECT_FALSE(is_unique);

// Set display:none should remove the layout object associated with the
// range the generator is currently targeting.
EXPECT_TRUE(target->GetLayoutObject());
target->setAttribute(html_names::kStyleAttr,
AtomicString("display:none"));
target->GetDocument().UpdateStyleAndLayoutTree();
target->GetDocument().View()->UpdateStyleAndLayout();
EXPECT_FALSE(target->GetLayoutObject());
},
WrapWeakPersistent(target));

// Pump tasks to continue generator operation now.
base::RunLoop().RunUntilIdle();
EXPECT_TRUE(finished);
}

} // namespace blink

0 comments on commit 136a29e

Please sign in to comment.