crash in mozilla::unicode::GetScriptTagForCode

RESOLVED FIXED in Firefox 48

Status

()

defect
--
critical
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: njn, Assigned: jfkthame)

Tracking

({crash})

Trunk
mozilla48
Unspecified
Linux
Points:
---

Firefox Tracking Flags

(firefox48 fixed)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

3 years ago
This bug was filed from the Socorro interface and is 
report bp-9022cada-cca4-4f75-8003-dec832160419.
=============================================================

Three crashes, all on Linux, in Nightly builds in the past week.

> 0 	libxul.so 	mozilla::unicode::GetScriptTagForCode 	intl/unicharutil/util/nsUnicodeProperties.cpp
> 1 	libxul.so 	gfxPangoFontGroup::FindFontForChar 	gfx/thebes/gfxFontconfigFonts.cpp
> 2 	libxul.so 	MathMLTextRunFactory::RebuildTextRun 	layout/generic/MathMLTextRunFactory.cpp
> 3 	libxul.so 	BuildTextRunsScanner::FlushLineBreaks 	layout/generic/nsTextRunTransformations.h
> 4 	libxul.so 	BuildTextRunsScanner::FlushFrames 	layout/generic/nsTextFrame.cpp
> 5 	libxul.so 	BuildTextRuns 	layout/generic/nsTextFrame.cpp
> 6 	libxul.so 	nsTextFrame::EnsureTextRun 	layout/generic/nsTextFrame.cpp
> 7 	libxul.so 	nsTextFrame::AddInlinePrefISizeForFlow 	layout/generic/nsTextFrame.cpp
> 8 	libxul.so 	nsTextFrame::AddInlinePrefISize 	layout/generic/nsTextFrame.cpp
> 9 	libxul.so 	nsBlockFrame::GetPrefISize 	layout/generic/nsBlockFrame.cpp
> 10 	libxul.so 	nsLayoutUtils::IntrinsicForAxis 	layout/base/nsLayoutUtils.cpp
> 11 	libxul.so 	nsMathMLContainerFrame::GetIntrinsicISizeMetrics 	layout/mathml/nsMathMLContainerFrame.cpp
> 12 	libxul.so 	nsMathMLContainerFrame::GetIntrinsicISizeMetrics 	layout/mathml/nsMathMLContainerFrame.cpp
> 13 	libxul.so 	nsMathMLContainerFrame::UpdateIntrinsicWidth 	layout/mathml/nsMathMLContainerFrame.cpp
> 14 	libxul.so 	nsMathMLContainerFrame::GetMinISize 	layout/mathml/nsMathMLContainerFrame.cpp
> 15 	libxul.so 	nsContainerFrame::ComputeAutoSize 	layout/generic/nsFrame.cpp
> 16 	libxul.so 	nsFrame::ComputeSize 	layout/generic/nsFrame.cpp
> 17 	libxul.so 	nsHTMLReflowState::InitConstraints 	layout/generic/nsHTMLReflowState.cpp
> 18 	libxul.so 	nsHTMLReflowState::Init 	layout/generic/nsHTMLReflowState.cpp

Looks like a straightforward null deref. I think uscript_getShortName() can return null but we don't check for that.
(Reporter)

Comment 1

3 years ago
jkew, can you please take a look?
Flags: needinfo?(jfkthame)
(Assignee)

Comment 2

3 years ago
Yes, uscript_getShortName() can return null, but only if the script code passed is invalid. Which should never happen; so the question is where that invalid script code came from.

And the answer is that MathMLTextRunFactory::RebuildTextRun passes a harfbuzz script code (HB_SCRIPT_COMMON), which is wrong; it should pass MOZ_SCRIPT_COMMON. Back when that line of code was written, the two were equivalent, but then harfbuzz changed its type to use tags, and we aligned our codes with ICU ... and now this is invalid, as we'd have discovered sooner if the MOZ_SCRIPT_* codes were an enum class.

(A quick grep reveals that there's also an abuse of an HB_SCRIPT_* constant in gfxFontGroup::FindFontForChar, though that one's relatively harmless -- it could result in us attempting font fallback when we didn't really want to, but that's all.)

The immediate bug-fix here, then, is a one-liner (well, two-liner if we fix the FindFontForChar error at the same time), so let's do it. But I also think we should update our script codes to use an enum class, instead of just passing around int32_t values; that would have prevented the bug here, because the compiler wouldn't have allowed us to use the wrong constant (without explicitly casting, which would have served as a huge red flag). I'll file a separate bug for that, as it'll turn into a pretty pervasive (though largely mechanical) patch....
Flags: needinfo?(jfkthame)
(Assignee)

Comment 3

3 years ago
Filed bug 1266391 for the type conversion.
See Also: → 1266391
(Assignee)

Comment 4

3 years ago
This fixes the specific error that's causing the crash. I'll post the big patch to introduce stronger typing separately in bug 1266391.
Attachment #8743822 - Flags: review?(masayuki)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Comment on attachment 8743822 [details] [diff] [review]
Pass the right kind of enum constants for script codes

I'm surprised at how did you find such mistake from the jungle!
Attachment #8743822 - Flags: review?(masayuki) → review+
(Assignee)

Comment 6

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b41c3ceb94d42a39b31349e6e29b227521fc15b8
Bug 1266341 - Pass the right kind of enum constants for script codes. r=masayuki
(Reporter)

Comment 7

3 years ago
jkew: I give you two gold stars -- one for the fast fix, and another for the follow-up to make this kind of problem less likely. Lovely work, thank you.

Comment 8

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b41c3ceb94d4
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
This crashes on a clean browser on common pages [1] after changing "gfx.font_rendering.fontconfig.fontlist.enabled" to "false".

If its importance really is "Critical", is there any chance to get it into Firefox 47, or even a point release of 46?

[1] https://en.wikipedia.org/wiki/Greek_alphabet
"Critical" just means it causes a crash. Looking at the crash reports link, I only see 4 crashes on any branch in the last 7 days, which does not meet the criteria for a point release for Firefox 47 (which just released today).
3 of those crash reports are me :)

I understand the rationale. It's a good metric.
(In reply to Andrew McCreight [:mccr8] from comment #10)
> "Critical" just means it causes a crash. Looking at the crash reports link,
> I only see 4 crashes on any branch in the last 7 days, which does not meet
> the criteria for a point release for Firefox 47 (which just released today).

I wouldn't expect this metric for Linux-only crashes makes much sense, because crash information from distro versions of Firefox doesn't go here, and the majority of our Linux users seems to actually use the version their distro provides rather than our official one.
(In reply to Xidorn Quan [:xidorn] (PTO June 6 ~ 10) from comment #12)
> I wouldn't expect this metric for Linux-only crashes makes much sense,
> because crash information from distro versions of Firefox doesn't go here,
> and the majority of our Linux users seems to actually use the version their
> distro provides rather than our official one.

Are you sure? The crashes I got were from Ubuntu's version of Firefox, and they appear in about:crashes, with links to Mozilla's crash-stats website.

Regardless, I believe the low number of crashes makes sense, as it requires changing an obscure config setting.

I asked because it's a pretty small and safe patch, and I'm affected by it. But I'm fine with waiting a few months if that's the decision.

Updated

3 years ago
Duplicate of this bug: 1280311
You need to log in before you can comment on or make changes to this bug.