Closed Bug 1937366 Opened 2 months ago Closed 2 months ago

Firefox crashes whenever I navigate to a URL with a text fragment (with intl.icu4x.segmenter.enabled = False)

Categories

(Core :: Internationalization, defect)

Firefox 135
defect

Tracking

()

RESOLVED FIXED
135 Branch
Tracking Status
firefox-esr128 --- unaffected
firefox133 --- unaffected
firefox134 --- fixed
firefox135 --- fixed

People

(Reporter: cpplearner, Assigned: m_kato)

References

(Regression)

Details

(Keywords: regression)

Crash Data

Attachments

(2 files)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:135.0) Gecko/20100101 Firefox/135.0

Steps to reproduce:

Simply visit https://example.com/#:~:text=1

Actual results:

The tab crashes.

Expected results:

The page gets displayed normally.

The problem disappears if I reset the intl.icu4x.segmenter.enabled pref to true (I previously set it to false), so this must be the cause.

Does it happen on a clean profile? I can't reproduce with Nightly and macOS.

Please attach to the bug the raw content of your about:support:

  • Type about:support in the address bar, press enter.
  • Click Copy raw data to clipboard.
  • Come back to this bug, click the Attach new file button above your first comment.
  • Paste the content from your clipboard, add a description and hit Submit at the bottom.
Flags: needinfo?(cpplearner)

It doesn't happen on a clean profile, unless and until I set the intl.icu4x.segmenter.enabled to false in about:config. I can attach the content of about:support, but I doubt if it will be interesting.

Attached file about:support
Flags: needinfo?(cpplearner)

Do you see any crash signature in about:crashes? It would be great if you could share one.

Component: Untriaged → DOM: Navigation
Product: Firefox → Core

Not very familiar with crash reports, but this might belong to Core::Internationalization, given the mention of icu_segmenter

Component: DOM: Navigation → Internationalization
Status: UNCONFIRMED → NEW
Crash Signature: [@ icu_provider::response::DataPayload<T>::get ]
Ever confirmed: true
Summary: Firefox crashes whenever I navigate to a URL with a text fragment → Firefox crashes whenever I navigate to a URL with a text fragment (with intl.icu4x.segmenter.enabled = False)

Bisection:
Bug 1928931: Speed up nsFind by reusing the word segmenter. r=emilio
Differential Revision: https://phabricator.services.mozilla.com/D227804

(This bug needs a non-standard configuration of disabling icu4x segmentor, so the priority to fix this bug may be low)

Keywords: regression
Regressed by: 1928931

Set release status flags based on info from the regressing bug 1928931

:jjaschke, since you are the author of the regressor, bug 1928931, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Thanks for reporting! I’ll look into this later today.

Flags: needinfo?(jjaschke)

The fix for this specific issue is trivial, it's missing a pref check in WordBreakIteratorUtf16::Reset().

However, I don't think this is enough. After changing this, I got other crashes, because nsFind::BreakInbetween() expects the results of the word breaker to be Some. But the non-icu4x implementation does seem to return Nothing{} in some cases (through this condition).

I tried changing the code in nsFind::BreakInbetween() to account for Nothing:

  auto seekResult = mWordBreakIter.Seek(x16Len-1);
  return seekResult && *seekResult == x16Len;

That fixes the crash, but it seems that text fragments isn't working anymore.

I'll investigate more tomorrow, it's Sunday after all :)

(ni? TYLin for visibility since you did the initial implementation, maybe you have an idea)

Flags: needinfo?(aethanyc)

Do we really need the intl.icu4x.segmenter.enabled pref anymore? It seems to me we should use the ICU4X segmenter unconditionally and remove the pref.

(not marking as disabled as there is no plan to revert the default setting of intl.icu4x.segmenter.enabled, IIUC)

This seems to be simple bug when this pref is false by bug 1928931. Due to bug 1871754, we keep an option for this.

FWIW I have the ICU4X segmenter disabled because of bug 1851131.

Also, Bug 1928931 breaks Seek method.

Assignee: nobody → m_kato

This issue is regression by bug 1928931. It doesn't consider ICU4X
preference is off.

Also, Added Reset() methods is broken. After calling Reset(), Seek() method
doesn't return correct position.

Thank you Jan and Makoto for looking into this bug.

Flags: needinfo?(aethanyc)
Pushed by m_kato@ga2.so-net.ne.jp: https://hg.mozilla.org/integration/autoland/rev/4d9ad8621740 Reset of word segmenter iterator should consider ICU4X preference is off. r=TYLin
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → 135 Branch

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

  • If yes, please nominate the patch for beta approval.
  • If no, please set status-firefox134 to wontfix.

For more information, please visit BugBot documentation.

Flags: needinfo?(m_kato)

Comment on attachment 9444330 [details]
Bug 1937366 - Reset of word segmenter iterator should consider ICU4X preference is off. r=TYLin

Beta/Release Uplift Approval Request

  • User impact if declined/Reason for urgency: When browsing a URL that has text fragment parameter with intl.icu4x.segmenter.enabled=false, Firefox will crash.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This change is for intl.icu4x.segmenter.enabled=false only (default is true).
  • String changes made/needed:
  • Is Android affected?: Yes
Flags: needinfo?(m_kato)
Attachment #9444330 - Flags: approval-mozilla-beta?
Flags: in-testsuite+

Comment on attachment 9444330 [details]
Bug 1937366 - Reset of word segmenter iterator should consider ICU4X preference is off. r=TYLin

Fixes a new regression in 134 that causes crashes for some users. Patch is scoped to only affect the case of a pref being set to a non-default value and includes automated test coverage. Approved for the Fx134 RC.

Attachment #9444330 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: