Closed
Bug 1286889
Opened 8 years ago
Closed 8 years ago
Crash in nsBlockFrame::GetLineCursor when trying to style ruby text when bidi is enabled
Categories
(Core :: Layout: Block and Inline, defect)
Tracking
()
RESOLVED
FIXED
mozilla50
People
(Reporter: rory, Assigned: xidorn)
References
Details
(Keywords: crash, regression)
Crash Data
Attachments
(2 files)
109 bytes,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
jfkthame
:
review+
gchang
:
approval-mozilla-beta+
|
Details |
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:47.0) Gecko/20100101 Firefox/47.0 Build ID: 20160630070928 Steps to reproduce: Open the testcase. As far as I can tell (I'm no C++ debugging expert) this is a null pointer issue? The crash doesn't seem to be exploitable, so I haven't marked this as a security issue. The character that causes the issue (in case the upload goes wonky) is: ܛ Tested on FF47 and nightly. Actual results: Firefox crashes. Relevant stacktrace: #0 0x00007fffe33797c4 in nsIFrame::GetStateBits (this=0x0) at <..>/firefox-47.0.1/layout/generic/nsIFrame.h:1427 #1 0x00007fffe55b3723 in nsBlockFrame::GetLineCursor (this=0x0) at <..>/firefox-47.0.1/layout/generic/nsBlockFrame.h:370 #2 0x00007fffe55a68fd in nsBlockInFlowLineIterator::nsBlockInFlowLineIterator (this=0x7fffffff1140, aFrame=0x0, aFindFrame=0x7fffd47d2ee8, aFoundValidLine=0x7fffffff116f) at <..>/firefox-47.0.1/layout/generic/nsBlockFrame.cpp:5494 #3 0x00007fffe54ef10c in InlineBackgroundData::AreOnSameLine (this=0x7fffdad85970, aFrame1=0x7fffd47d2ee8, aFrame2=0x7fffd47d2020) at <..>/firefox-47.0.1/layout/base/nsCSSRendering.cpp:382 #4 0x00007fffe54eeed9 in InlineBackgroundData::Init (this=0x7fffdad85970, aFrame=0x7fffd47d2ee8) at <..>/firefox-47.0.1/layout/base/nsCSSRendering.cpp:354 #5 0x00007fffe54ee917 in InlineBackgroundData::SetFrame (this=0x7fffdad85970, aFrame=0x7fffd47d2ee8) Expected results: No crash!
Comment 1•8 years ago
|
||
bp-3fdd4ebc-4332-4958-b9d9-4a7212160714 bp-e42477d3-4552-4eef-a6c2-c9e8d2160714
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsBlockFrame::GetLineCursor]
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox-esr45:
--- → affected
Ever confirmed: true
OS: Linux → All
CR FF50: https://crash-stats.mozilla.com/report/index/6d1c4c9e-0a2a-4139-8757-cf0762160714 Old builds crash with "layout.css.ruby.enabled:true", so with this pref, I got this regression range: Last good revision: 3d846527576f (2015-01-13) First bad revision: 63006936ab99 (2015-01-14) https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3d846527576f&tochange=63006936ab99 It's probably regressed by bug 1055658.
Blocks: 1055658
Crash Signature: [@ nsBlockFrame::GetLineCursor] → [@ nsBlockFrame::GetLineCursor ]
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox47:
affected → ---
status-firefox48:
affected → ---
status-firefox49:
affected → ---
status-firefox50:
affected → ---
status-firefox-esr45:
affected → ---
Component: Untriaged → Layout: Block and Inline
Flags: needinfo?(xidorn+moz)
Keywords: reproducible,
testcase
OS: All → Linux
Product: Firefox → Core
Summary: Firefox crashes trying to style special UTF-8 character → Crash in nsBlockFrame::GetLineCursor when trying to style ruby text with special UTF-8 character
Version: 47 Branch → 38 Branch
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox49:
--- → affected
status-firefox50:
--- → affected
status-firefox-esr45:
--- → affected
OS: Linux → All
Hardware: x86_64 → All
Comment 3•8 years ago
|
||
Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=95c1fecef4f6&tochange=2f68cea9b4c9 Triggered by : 587cfd4385b7 Xidorn Quan — Bug 1039006 - Enable the preference for CSS Ruby by default. r=dbaron Regression window(force enable css ruby): https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0a0e57c4e420&tochange=a638073e104d
Assignee | ||
Comment 4•8 years ago
|
||
This happens when bidi is enabled, not only that specific character.
Assignee: nobody → xidorn+moz
Summary: Crash in nsBlockFrame::GetLineCursor when trying to style ruby text with special UTF-8 character → Crash in nsBlockFrame::GetLineCursor when trying to style ruby text when bidi is enabled
Assignee | ||
Comment 5•8 years ago
|
||
If looks like this code asserts that line container is always block frame which is no longer true since ruby support, because ruby text container could be line container as well.
Assignee | ||
Comment 6•8 years ago
|
||
Hmmm, there are actually two bugs... One is that the code doesn't handle ruby text container as line container very well, the second is that list-item is not computed to inline-list-item. Given we haven't implemented inline-list-item, fixing the first one is easier :)
Assignee | ||
Comment 7•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/64568/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64568/
Attachment #8771348 -
Flags: review?(dholbert)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(xidorn+moz)
Updated•8 years ago
|
Attachment #8771348 -
Flags: review?(dholbert) → review?(jfkthame)
Comment 8•8 years ago
|
||
Comment on attachment 8771348 [details] Bug 1286889 - Handle ruby text container as line container in InlineBackgroundData. https://reviewboard.mozilla.org/r/64568/#review61710 jfkthame would be a better reviewer here, if he has cycles for this. He's got 'hg blame' for some of this InlineBackgroundData code, whereas I've never really looked at it before. --> Transfering review to him. One nit that I noticed while skimming this, though: ::: layout/base/nsCSSRendering.cpp:328 (Diff revision 1) > void Init(nsIFrame* aFrame) > { > mPIStartBorderData.Reset(); > mBidiEnabled = aFrame->PresContext()->BidiEnabled(); > if (mBidiEnabled) { > // Find the containing block frame This comment needs an update. (This code isn't finding the "containing block frame" anymore -- now it's finding the "line container".) Might be nice to add a brief comment somewhere explaining in words (rather than code) what mLineContainer actually represents -- either here, or where mLineContainer is declared.
Attachment #8771348 -
Flags: review?(dholbert)
Updated•8 years ago
|
Attachment #8771348 -
Flags: review?(dholbert)
Comment 9•8 years ago
|
||
Comment on attachment 8771348 [details] Bug 1286889 - Handle ruby text container as line container in InlineBackgroundData. https://reviewboard.mozilla.org/r/64568/#review61748 This looks good to me. A couple of places, I wonder if we can add further assertions, just to sanity-check what we're doing (and give Jesse's fuzzers extra targets to aim at!).... do these make sense? ::: layout/base/nsCSSRendering.cpp:335 (Diff revision 1) > - frame = frame->GetParent(); > - mBlockFrame = do_QueryFrame(frame); > + mLineContainer->IsFrameOfType(nsIFrame::eLineParticipant)) { > + mLineContainer = mLineContainer->GetParent(); > } > - while (frame && frame->IsFrameOfType(nsIFrame::eLineParticipant)); > > - NS_ASSERTION(mBlockFrame, "Cannot find containing block."); > + MOZ_ASSERT(mLineContainer, "Cannot find line containing frame."); Should we also assert that mLineContainer != aFrame? ::: layout/base/nsCSSRendering.cpp:390 (Diff revision 1) > - it1.GetContainer() == it2.GetContainer() && > + it1.GetContainer() == it2.GetContainer() && > - // And on the same line in it > + // And on the same line in it > - it1.GetLine() == it2.GetLine(); > + it1.GetLine() == it2.GetLine(); > - } > + } > + if (nsRubyTextContainerFrame* rtcFrame = do_QueryFrame(mLineContainer)) { > + nsBlockFrame* block = nsLayoutUtils::FindNearestBlockAncestor(rtcFrame); Should we assert that this finds a valid block? ::: layout/base/nsCSSRendering.cpp:398 (Diff revision 1) > + bool isDescendent1 = > + nsLayoutUtils::IsProperAncestorFrame(frame, aFrame1, block); > + bool isDescendent2 = > + nsLayoutUtils::IsProperAncestorFrame(frame, aFrame2, block); > + if (isDescendent1 && isDescendent2) { > + return true; > + } > + if (isDescendent1 || isDescendent2) { > + return false; > + } > + } > + MOZ_ASSERT_UNREACHABLE("None of the frames is a descendent of this rtc?"); nit: the spelling throughout these lines should be 'descendant' (-ant ending, not -ent).
Attachment #8771348 -
Flags: review?(jfkthame) → review+
Assignee | ||
Comment 10•8 years ago
|
||
https://reviewboard.mozilla.org/r/64568/#review61748 > Should we assert that this finds a valid block? I don't think we need. I imagine it is possible that there is no ancestor block frame at all, if, for example, the body is set to something else. The block here serves as an optimization to avoid traversing too far. And if it is nullptr, the following lines would just traverse all the way to the root, and no harm would actually happen.
Comment 11•8 years ago
|
||
Pushed by xquan@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/50ef663047f3 Handle ruby text container as line container in InlineBackgroundData. r=jfkthame
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/50ef663047f3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 13•8 years ago
|
||
xidorn do you think we should uplift this fix to 49? If so please request approval.
Flags: needinfo?(xidorn+moz)
Assignee | ||
Comment 14•8 years ago
|
||
Comment on attachment 8771348 [details] Bug 1286889 - Handle ruby text container as line container in InlineBackgroundData. Approval Request Comment [Feature/regressing bug #]: CSS Ruby [User impact if declined]: may crash in some cases [Describe test coverage new/current, TreeHerder]: added new crashtest [Risks and why]: not risky, it basically just adds an additional branch to handle the ruby case [String/UUID change made/needed]: n/a
Flags: needinfo?(xidorn+moz)
Attachment #8771348 -
Flags: approval-mozilla-beta?
Comment 15•8 years ago
|
||
Comment on attachment 8771348 [details] Bug 1286889 - Handle ruby text container as line container in InlineBackgroundData. This patch fixes a crash for ruby. Take it in 49 beta and aurora.
Attachment #8771348 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/c18bbf3a9ef2
You need to log in
before you can comment on or make changes to this bug.
Description
•