Closed Bug 1266341 Opened 5 years ago Closed 5 years ago
crash in mozilla::unicode::Get
Script Tag For Code
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.
jkew, can you please take a look?
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....
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: 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+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b41c3ceb94d42a39b31349e6e29b227521fc15b8 Bug 1266341 - Pass the right kind of enum constants for script codes. r=masayuki
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.
This crashes on a clean browser on common pages  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?  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.
You need to log in before you can comment on or make changes to this bug.