Stack exhaustion in [@ AppendText]

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: tsmith, Assigned: emilio)

Tracking

(Blocks 1 bug, {crash, testcase})

55 Branch
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?
qe-verify -

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox54 unaffected, firefox55 fixed, firefox56 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

2 years ago
Posted file test_case.html
#0  0x00007fffdcaf92d5 in AppendText () at src/dom/base/nsLineBreaker.cpp:330
#1  0x00007fffe0d4737b in SetupBreakSinksForTextRun () at src/layout/generic/nsTextFrame.cpp:2647
#2  0x00007fffe0d40874 in BuildTextRunForFrames () at src/layout/generic/nsTextFrame.cpp:2408
#3  0x00007fffe0d3a8f7 in FlushFrames () at src/layout/generic/nsTextFrame.cpp:1688
#4  0x00007fffe0d4ae4f in BuildTextRuns () at src/layout/generic/nsTextFrame.cpp:1614
#5  EnsureTextRun () at src/layout/generic/nsTextFrame.cpp:2861
#6  0x00007fffe0d82736 in AddInlineMinISizeForFlow () at src/layout/generic/nsTextFrame.cpp:8455
#7  0x00007fffe0d85464 in AddInlineMinISize () at src/layout/generic/nsTextFrame.cpp:8625
#8  0x00007fffe0b090cf in GetMinISize () at src/layout/generic/nsBlockFrame.cpp:770
#9  0x00007fffe0b7b356 in ShrinkWidthToFit () at src/layout/generic/nsFrame.cpp:5800
#10 ComputeAutoSize () at src/layout/generic/nsContainerFrame.cpp:845
#11 0x00007fffe0b81f97 in ComputeSize () at src/layout/generic/nsFrame.cpp:5061
#12 0x00007fffe0ac3b61 in InitConstraints () at src/layout/generic/ReflowInput.cpp:2492
#13 0x00007fffe0aba110 in Init () at src/layout/generic/ReflowInput.cpp:409
#14 0x00007fffe10e7cce in Reflow () at src/layout/mathml/nsMathMLTokenFrame.cpp:143
#15 0x00007fffe0af58a4 in ReflowChild () at src/layout/generic/nsContainerFrame.cpp:978
#16 0x00007fffe10d8d96 in ReflowChild () at src/layout/mathml/nsMathMLContainerFrame.cpp:839
#17 0x00007fffe10d96cd in Reflow () at src/layout/mathml/nsMathMLContainerFrame.cpp:892
#18 0x00007fffe0af58a4 in ReflowChild () at src/layout/generic/nsContainerFrame.cpp:978
#19 0x00007fffe10d8d96 in ReflowChild () at src/layout/mathml/nsMathMLContainerFrame.cpp:839
#20 0x00007fffe10d96cd in Reflow () at src/layout/mathml/nsMathMLContainerFrame.cpp:892
#21 0x00007fffe0cc71df in ReflowFrame () at src/layout/generic/nsLineLayout.cpp:921
#22 0x00007fffe0cc5153 in ReflowInlineFrame () at src/layout/generic/nsInlineFrame.cpp:798
#23 0x00007fffe0cc3597 in ReflowFrames () at src/layout/generic/nsInlineFrame.cpp:681
#24 0x00007fffe0cc26ba in Reflow () at src/layout/generic/nsInlineFrame.cpp:460
Flags: in-testsuite?
INFO: Last good revision: aab0dfdae32f14246e3bed8824a5f7ce309276cd
INFO: First bad revision: 3e6477d932037d6026ac13bd8c988dc0a51935d4
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=aab0dfdae32f14246e3bed8824a5f7ce309276cd&tochange=3e6477d932037d6026ac13bd8c988dc0a51935d4
Blocks: 1361766
Flags: needinfo?(emilio+bugs)
Version: Trunk → 55 Branch
(Assignee)

Comment 2

2 years ago
I don't know a lot about MathML, but I don't think that should be valid markup... Pretty much every change to the test case makes it invalid markup...

Here's a test-case with the balanced tags:

<math>
  <munderover>
    &gt;
    <munder accentunder="true">
      <mo>)</mo>
      <mscarry>
    </munder>
  </munderover>
</math>

I _think_ (reading https://www.w3.org/TR/MathML3/chapter3.html#presm.mscarries) that <mscarry> should only be allowed inside <mscarries>, but I'm not totally sure...

Loading this test-case in a debug build I get a bunch of:

WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004001: file /home/emilio/projects/moz/stylo/layout/generic/nsBlockFrame.cpp, line 946...
(Assignee)

Comment 3

2 years ago
That being said I think I know why the test-case regressed with my patch, which is kind of sad... MathML reflows are non-idempotent, which is something someone should really fix :(
Comment hidden (mozreview-request)

Comment 6

2 years ago
mozreview-review
Comment on attachment 8881163 [details]
Bug 1376158: Don't let non-primary frames affect the mIncrementScriptLevel of the content.

https://reviewboard.mozilla.org/r/152432/#review158550
Attachment #8881163 - Flags: review?(xidorn+moz) → review+

Comment 7

2 years ago
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5280f9ba46df
Don't let non-primary frames affect the mIncrementScriptLevel of the content. r=xidorn

Comment 8

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5280f9ba46df
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Should we uplift this change to beta?
Assignee: nobody → emilio+bugs
Flags: needinfo?(emilio+bugs)
(Assignee)

Comment 10

2 years ago
(In reply to Julien Cristau [:jcristau] from comment #9)
> Should we uplift this change to beta?

Seems harmless, yeah, will file a request.
Flags: needinfo?(emilio+bugs)
(Assignee)

Comment 11

2 years ago
Comment on attachment 8881163 [details]
Bug 1376158: Don't let non-primary frames affect the mIncrementScriptLevel of the content.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1361766
[User impact if declined]: Crashes on some obscure MathML content.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: Just this patch.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This just enforces an invariant that is assumed in the rest of the MathML code, and only affects already-invalid MathML content.
[String changes made/needed]: none
Attachment #8881163 - Flags: approval-mozilla-beta?
Comment on attachment 8881163 [details]
Bug 1376158: Don't let non-primary frames affect the mIncrementScriptLevel of the content.

avoid stack exhaustion, regression in beta55
Attachment #8881163 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #11)
> [Is this code covered by automated tests?]: Yes
> [Has the fix been verified in Nightly?]: Yes
> [Needs manual test from QE? If yes, steps to reproduce]: No

Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.