Firefox crashes whenever I navigate to a URL with a text fragment (with intl.icu4x.segmenter.enabled = False)
Categories
(Core :: Internationalization, defect)
Tracking
()
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)
43.59 KB,
text/plain
|
Details | |
48 bytes,
text/x-phabricator-request
|
RyanVM
:
approval-mozilla-beta+
|
Details | Review |
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.
Reporter | ||
Comment 1•2 months ago
|
||
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.
Comment 2•2 months ago
|
||
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.
Reporter | ||
Comment 3•2 months ago
|
||
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.
Reporter | ||
Comment 4•2 months ago
|
||
Comment 5•2 months ago
|
||
Do you see any crash signature in about:crashes? It would be great if you could share one.
Updated•2 months ago
|
Reporter | ||
Comment 6•2 months ago
|
||
Comment 7•2 months ago
•
|
||
Not very familiar with crash reports, but this might belong to Core::Internationalization, given the mention of icu_segmenter
Updated•2 months ago
|
Updated•2 months ago
|
Comment 8•2 months ago
•
|
||
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)
Comment 9•2 months ago
|
||
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.
Comment 10•2 months ago
|
||
Thanks for reporting! I’ll look into this later today.
Comment 11•2 months ago
|
||
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)
Comment 12•2 months ago
|
||
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.
Updated•2 months ago
|
Comment 13•2 months ago
|
||
(not marking as disabled as there is no plan to revert the default setting of intl.icu4x.segmenter.enabled
, IIUC)
Assignee | ||
Comment 14•2 months ago
•
|
||
This seems to be simple bug when this pref is false by bug 1928931. Due to bug 1871754, we keep an option for this.
Reporter | ||
Comment 15•2 months ago
|
||
FWIW I have the ICU4X segmenter disabled because of bug 1851131.
Assignee | ||
Comment 17•2 months ago
|
||
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.
Comment 18•2 months ago
|
||
Thank you Jan and Makoto for looking into this bug.
Comment 19•2 months ago
|
||
Comment 20•2 months ago
|
||
bugherder |
Comment 21•2 months ago
|
||
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
towontfix
.
For more information, please visit BugBot documentation.
Assignee | ||
Comment 22•2 months ago
|
||
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
Updated•2 months ago
|
Comment 23•2 months ago
|
||
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.
Updated•2 months ago
|
Comment 24•2 months ago
|
||
uplift |
Description
•