2058916 - IllegalArgumentException in HighlightedDomainUrlKt.computeDomainEndScrollValue

Status: RESOLVED FIXED

(Firefox for Android :: Toolbar, defect)

Description

[Polly [:polly]]

Crash observed in play store since firefox v153

Exception java.lang.IllegalArgumentException:
  at androidx.compose.ui.text.internal.InlineClassHelperKt.throwIllegalArgumentException (InlineClassHelper.kt:53)
  at androidx.compose.ui.text.MultiParagraph.getPathForRange (MultiParagraph.java:1262)
  at androidx.compose.ui.text.TextLayoutResult.getPathForRange (TextLayoutResult.kt:533)
  at mozilla.components.compose.browser.toolbar.ui.HighlightedDomainUrlKt.computeDomainEndScrollValue (HighlightedDomainUrl.kt:222)
  at mozilla.components.compose.browser.toolbar.ui.HighlightedDomainUrlKt$focusTextIndexRange$2$1$1.invokeSuspend (HighlightedDomainUrl.kt:151)
  at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith (ContinuationImpl.kt:34)
  at kotlinx.coroutines.DispatchedTask.run (DispatchedTask.kt:100)
  at androidx.compose.ui.platform.AndroidUiDispatcher.performTrampolineDispatch (AndroidUiDispatcher.android.kt:79)
  at androidx.compose.ui.platform.AndroidUiDispatcher.access$performTrampolineDispatch (AndroidUiDispatcher.android.kt:41)
  at androidx.compose.ui.platform.AndroidUiDispatcher$dispatchCallback$1.run (AndroidUiDispatcher.android.kt:57)
  at android.os.Handler.handleCallback (Handler.java:938)
  at android.os.Handler.dispatchMessage (Handler.java:99)
  at android.os.Looper.loopOnce (Looper.java:210)
  at android.os.Looper.loop (Looper.java:299)
  at android.app.ActivityThread.main (ActivityThread.java:8280)
  at java.lang.reflect.Method.invoke
  at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run (RuntimeInit.java:576)
  at com.android.internal.os.ZygoteInit.main (ZygoteInit.java:1073)

Comment 1

[Polly [:polly]]

looks like this code is throwing, because the index is out of bounds
maybe we need to be more defensive in this calculation

Comment 2

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 2058916 - part 1 - Skip LinkText links not found in the full text r=#android-reviewers

buildUrlAnnotatedString logged but still added a LinkAnnotation when the link
text was blank or not found in the full text (indexOf returned -1), producing a
span with a negative start that crashed later in the text layout pass.
Skip such links instead so the text still renders, just without the broken
clickable span. This mainly guards against localized strings whose link
substring drifts out of the full text.

Comment 3

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 2058916 - part 2 - Clamp the domain range before measuring the highlighted URL r=#android-reviewers

computeDomainEndScrollValue passed the registrable domain indices straight to
getPathForRange, which throws if they are out of range or start > end. Coerce
them so 0 <= startIndex <= endIndex <= text.length always holds, guarding
against a caller supplying a domain span inconsistent with the displayed URL.

This is mostly hardening the changes done for bug 2056836.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/dee06f5188a1
https://hg.mozilla.org/integration/autoland/rev/ed69de72aa79
part 1 - Skip LinkText links not found in the full text r=android-reviewers,sfamisa
https://github.com/mozilla-firefox/firefox/commit/e5022c041c99
https://hg.mozilla.org/integration/autoland/rev/de36f9398a2e
part 2 - Clamp the domain range before measuring the highlighted URL r=android-reviewers,sfamisa

Comment 5

[rperta]

https://hg.mozilla.org/mozilla-central/rev/ed69de72aa79
https://hg.mozilla.org/mozilla-central/rev/de36f9398a2e

Comment 6

[BugBot [:suhaib / :marco]]

The patch landed in nightly and beta is affected.
:petru, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
  • See https://wiki.mozilla.org/Release_Management/Requesting_an_Uplift for documentation on how to request an uplift.
• If no, please set status-firefox154 to wontfix.

For more information, please visit BugBot documentation.

Comment 7

[Petru-Mugurel Lingurar [:petru]]

Checking what happened over the weekend I see that we haven't had any more of these crash in Nightly while in total we had a few hundreds crashes in beta and a few thousands of crashes in release so with the fix for this being small we should look into uplifting it.

Comment 8

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 2058916 - part 1 - Skip LinkText links not found in the full text

buildUrlAnnotatedString logged but still added a LinkAnnotation when the link
text was blank or not found in the full text (indexOf returned -1), producing a
span with a negative start that crashed later in the text layout pass.
Skip such links instead so the text still renders, just without the broken
clickable span. This mainly guards against localized strings whose link
substring drifts out of the full text.

Original Revision: https://phabricator.services.mozilla.com/D315282

Comment 9

[Phabricator Automation]

firefox-beta Uplift Approval Request

• User impact if declined/Reason for urgency: Potential crashes in the toolbar from trying to highlight the URL domain concurrently with the URL being changed.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: Issue depends on a race condition - hard to reproduce. We can check the crash reports to assess the impact of the fix.
• Risk associated with taking this patch: low
• Explanation of risk level: Small patch which avoids the original crash as per the Google Play statistics without causing other problems.
• String changes made/needed?: --
• Is Android affected?: yes

Comment 10

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 2058916 - part 2 - Clamp the domain range before measuring the highlighted URL

computeDomainEndScrollValue passed the registrable domain indices straight to
getPathForRange, which throws if they are out of range or start > end. Coerce
them so 0 <= startIndex <= endIndex <= text.length always holds, guarding
against a caller supplying a domain span inconsistent with the displayed URL.

This is mostly hardening the changes done for bug 2056836.

Original Revision: https://phabricator.services.mozilla.com/D315283

Comment 11

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 2058916 - part 1 - Skip LinkText links not found in the full text

buildUrlAnnotatedString logged but still added a LinkAnnotation when the link
text was blank or not found in the full text (indexOf returned -1), producing a
span with a negative start that crashed later in the text layout pass.
Skip such links instead so the text still renders, just without the broken
clickable span. This mainly guards against localized strings whose link
substring drifts out of the full text.

Original Revision: https://phabricator.services.mozilla.com/D315282

Comment 12

[Phabricator Automation]

firefox-release Uplift Approval Request

• User impact if declined/Reason for urgency: Potential crashes in the toolbar from trying to highlight the URL domain concurrently with the URL being changed.
• Code covered by automated testing?: yes
• Fix verified in Nightly?: no
• Needs manual QE testing?: no
• Steps to reproduce for manual QE testing: Issue depends on a race condition - hard to reproduce. We can check the crash reports to assess the impact of the fix.
• Risk associated with taking this patch: low
• Explanation of risk level: Small patch which avoids the original crash as per the Google Play statistics without causing other problems.
• String changes made/needed?: --
• Is Android affected?: yes

Comment 13

[Petru-Mugurel Lingurar [:petru]]

Attachment: Bug 2058916 - part 2 - Clamp the domain range before measuring the highlighted URL

computeDomainEndScrollValue passed the registrable domain indices straight to
getPathForRange, which throws if they are out of range or start > end. Coerce
them so 0 <= startIndex <= endIndex <= text.length always holds, guarding
against a caller supplying a domain span inconsistent with the displayed URL.

This is mostly hardening the changes done for bug 2056836.

Original Revision: https://phabricator.services.mozilla.com/D315283

Comment 14

[Pascal Chevrel (relman team) -> :pascalc|PTO until Sept 2]

Let's assess the crash data in beta first and if it's good, we can uplift in next week dot release.

Comment 15

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/87f13d1350a8
https://hg.mozilla.org/releases/mozilla-beta/rev/5e62e2d14fcd
https://github.com/mozilla-firefox/firefox/commit/e32fc2cb4369
https://hg.mozilla.org/releases/mozilla-beta/rev/4cde9a2542e3

Comment 16

[Pulsebot]

https://github.com/mozilla-firefox/firefox/commit/224959f4ec0f
https://hg.mozilla.org/releases/mozilla-release/rev/6b1425a871db
https://github.com/mozilla-firefox/firefox/commit/5fe6aeb95a8e
https://hg.mozilla.org/releases/mozilla-release/rev/46af0d8485b3