1989891 - Crash in [@ mozilla::dom::TextDirectiveUtil::ComputeCommonSubstringLength<T>]

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P3)

Description

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Similar to bug 1987845 after bug 1979588 relanded but with reduced crash volume.

Crash report: https://crash-stats.mozilla.org/report/index/85998ad3-d168-407b-b2bb-453450250910

MOZ_CRASH Reason:

MOZ_DIAGNOSTIC_ASSERT(!nsContentUtils::IsHTMLWhitespace(aReferenceString.First()))

Top 10 frames:

0  xul.dll  mozilla::dom::TextDirectiveUtil::ComputeCommonSubstringLength<-1>(nsTSubstrin...  dom/base/TextDirectiveUtil.h:554
1  xul.dll  mozilla::dom::RangeBasedTextDirectiveCreator::FindEndMatchCommonSubstringLeng...  dom/base/TextDirectiveCreator.cpp:624
2  xul.dll  mozilla::dom::RangeBasedTextDirectiveCreator::FindAllMatchingCandidates()  dom/base/TextDirectiveCreator.cpp:575
3  xul.dll  mozilla::dom::TextDirectiveCreator::CreateTextDirectiveFromRange(mozilla::dom...  dom/base/TextDirectiveCreator.cpp:57
4  xul.dll  mozilla::dom::FragmentDirective::CreateTextDirectiveForRanges(mozilla::dom::S...  dom/base/FragmentDirective.cpp:465
5  xul.dll  mozilla::dom::FragmentDirective_Binding::createTextDirectiveForRanges(JSConte...  dom/bindings/FragmentDirectiveBinding.cpp:194
5  xul.dll  mozilla::dom::FragmentDirective_Binding::createTextDirectiveForRanges_promise...  dom/bindings/FragmentDirectiveBinding.cpp:205
6  xul.dll  mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::Nor...  dom/bindings/BindingUtils.cpp:3308
7  xul.dll  CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::...  js/src/vm/Interpreter.cpp:501
7  xul.dll  js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstru...  js/src/vm/Interpreter.cpp:597

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

This is diagnostic assertion only.

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

The bug is marked as tracked for firefox145 (nightly). However, the bug still isn't assigned, has low priority and has low severity.

:hsinyi, could you please find an assignee, increase the priority and increase the severity for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit BugBot documentation.

Comment 3

[Jan Jaeschke [:jjaschke]]

This is a diagnostic assert. I'm still trying to reproduce. The "regressor" changed the asserts from MOZ_ASSERT to MOZ_DIAGNOSTIC_ASSERT. So it's not really a regression.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

:smaug, could you consider increasing the severity of this top-crash bug?

For more information, please visit BugBot documentation.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Yes. This is still a very common issue.
jjaschke, I think the assertion should be changed to MOZ_ASSERT or something rather soon.

Comment 6

[Jan Jaeschke [:jjaschke]]

I can reproduce the crash with a page that has this content:

test 1 1111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111
test

When selecting the "test" at the end and then right-clicking.

The algorithm collects surrounding prefix and suffix text around the target range. Then it collects all other matches for the target range in the document, and compares the surrounding text of these matches to the prefix and suffix, to determine the length of the common substrings.
These prefix and suffix terms are cut off arbitrarily at a certain length (1024 chars) to avoid doing too much work -- if the first 1024 chars of a prefix and suffix are equal between the target range and a match, we give up (the URLs would become much too long in that case as well).

The code here cuts off at 1024 chars, but doesn't account for whitespace - if it happens that at that exact cutoff position there is whitespace, we run into the assert. The test case above does exactly that. After 1024 characters (reading from the right, because this is prefix) there is a whitespace character, so that the cutoff prefix consists of a whitespace followed by 1023 "1"; the same happens for the suffix code.

The fix is trivial -- don't cutoff at 1024 chars, but instead at a word boundary.

Comment 7

[Jan Jaeschke [:jjaschke]]

Attachment: Bug 1989891 - Text Fragments: Ensure context terms don't start/end with whitespace when reaching maximum length. r=smaug!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/c1168e11ed85
https://hg.mozilla.org/integration/autoland/rev/5fe5a185035e
Text Fragments: Ensure context terms don't start/end with whitespace when reaching maximum length. r=smaug

Comment 9

[Iulian Moraru]

https://hg.mozilla.org/mozilla-central/rev/5fe5a185035e