Closed Bug 1286889 Opened 4 years ago Closed 4 years ago

Crash in nsBlockFrame::GetLineCursor when trying to style ruby text when bidi is enabled

Categories

(Core :: Layout: Block and Inline, defect, critical)

38 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox47 --- wontfix
firefox48 --- wontfix
firefox49 --- fixed
firefox-esr45 --- affected
firefox50 --- fixed

People

(Reporter: rory, Assigned: xidorn)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

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!
Keywords: crash
OS: Unspecified → Linux
Hardware: Unspecified → x86_64
bp-3fdd4ebc-4332-4958-b9d9-4a7212160714
bp-e42477d3-4552-4eef-a6c2-c9e8d2160714
Severity: normal → critical
Status: UNCONFIRMED → NEW
Crash Signature: [@ nsBlockFrame::GetLineCursor]
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
Component: Untriaged → Layout: Block and Inline
Flags: needinfo?(xidorn+moz)
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
OS: Linux → All
Hardware: x86_64 → All
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
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
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.
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 :)
Flags: needinfo?(xidorn+moz)
Attachment #8771348 - Flags: review?(dholbert) → review?(jfkthame)
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)
Attachment #8771348 - Flags: review?(dholbert)
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+
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.
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
https://hg.mozilla.org/mozilla-central/rev/50ef663047f3
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
xidorn do you think we should uplift this fix to 49? If so please request approval.
Flags: needinfo?(xidorn+moz)
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 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+
You need to log in before you can comment on or make changes to this bug.