Closed Bug 1362423 Opened 4 years ago Closed 4 years ago

Crash @ [NewPerFrameData /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:666]

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox-esr52 --- unaffected
firefox53 --- unaffected
firefox54 --- unaffected
firefox55 --- fixed

People

(Reporter: jkratzer, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

(4 keywords)

Attachments

(4 files, 1 obsolete file)

Attached file Testcase
Testcase found while fuzzing mozilla-central rev 20170504-0b255199db9d.

ASAN:DEADLYSIGNAL
=================================================================
==27669==ERROR: AddressSanitizer: SEGV on unknown address 0x00000000004c (pc 0x7f5d26ab425e bp 0x7ffe43d85f30 sp 0x7ffe43d85a60 T0)
==27669==The signal is caused by a READ memory access.
==27669==Hint: address points to the zero page.
    #0 0x7f5d26ab425d in GetWritingMode /home/worker/workspace/build/src/layout/generic/nsIFrame.h:850:56
    #1 0x7f5d26ab425d in NewPerFrameData /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:666
    #2 0x7f5d26ab425d in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:792
    #3 0x7f5d2698f461 in nsFirstLetterFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsFirstLetterFrame.cpp:245:9
    #4 0x7f5d26ab5905 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:921:13
    #5 0x7f5d26ab39f2 in nsInlineFrame::ReflowInlineFrame(nsPresContext*, mozilla::ReflowInput const&, nsInlineFrame::InlineReflowInput&, nsIFrame*, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsInlineFrame.cpp:798:15
    #6 0x7f5d26ab1fb6 in nsInlineFrame::ReflowFrames(nsPresContext*, mozilla::ReflowInput const&, nsInlineFrame::InlineReflowInput&, mozilla::ReflowOutput&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsInlineFrame.cpp:681:7
    #7 0x7f5d26ab1239 in nsInlineFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsInlineFrame.cpp:460:3
    #8 0x7f5d26ab5905 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /home/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:921:13
    #9 0x7f5d2693ea64 in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4215:15
    #10 0x7f5d2693d84c in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4016:5
    #11 0x7f5d26934fa6 in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3890:9
    #12 0x7f5d2692ea18 in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2876:5
    #13 0x7f5d26924780 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2412:7
    #14 0x7f5d2691b26a in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1232:3
    #15 0x7f5d26976d9a in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /home/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:898:14
Flags: in-testsuite?
Is this a trunk regression?  I can crash a recent Nightly but not a local Opt build that's
a few weeks old, on Linux64.
Priority: -- → P2
Flags: needinfo?(jfkthame)
Blocks: 1358275
Flags: needinfo?(jfkthame)
OK, I see what I broke here. In bug 1358275, I moved the clearing of the NEEDS_BIDI_RESOLUTION into the new pre-scanning loop, so that it would happen even if we take the no-bidi-needed early return; but unfortunately that doesn't work for the case where bidi-override is in effect and so we skip running the pre-scan loop. So we should keep this in the main Resolve loop as well. This means that in some cases, we'll end up clearing the flag twice, but it's a cheap operation (just clearing a bit in a flags word) so I think trying to avoid that duplication by introducing additional tests here would be counterproductive; just clearing it unconditionally in both places is the simple solution.
Attachment #8865137 - Flags: review?(dholbert)
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
This is just adding jkratzer's testcase to the tree as a crashtest.
Attachment #8865138 - Flags: review?(dholbert)
Comment on attachment 8865137 [details] [diff] [review]
Ensure nsBidiPresUtils::Resolve always clears the NEEDS_BIDI_RESOLUTION flags for the block and continuations, whichever code-path it takes when looking for possible short-circuits

Review of attachment 8865137 [details] [diff] [review]:
-----------------------------------------------------------------

r=me. Thanks for the clear explanation in comment 3.
Attachment #8865137 - Flags: review?(dholbert) → review+
Comment on attachment 8865138 [details] [diff] [review]
Testcase from the bug report

Review of attachment 8865138 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a different testcase as noted below.

::: layout/base/crashtests/1362423-1.html
@@ +10,5 @@
> +  try { document.head.appendChild(o3); } catch(e) { };
> +  try { document.styleSheets[1].insertRule("area { stroke: none; }", 0); } catch(e) { };
> +  try { document.styleSheets[1].insertRule("* { unicode-bidi: bidi-override; }", 0); } catch(e) { };
> +  try { document.styleSheets[1].insertRule("*::first-letter { unicode-bidi: auto; }", 0); } catch(e) { };
> +  try { document.styleSheets[1].insertRule("* { flex: -134217729 6101880.270428436 -8589934592; column-width: calc(-15px); height: 15%; }", 0); } catch(e) { };

The testcase crashes my debug build if I load it directly, but it doesn't seem to crash in the crashtest harness.  So I don't think this is sufficient to regression-test this.

After some investigation, I believe it's due to the dependence on window-height from the "height: 15%" on this ^^ line of the testcase.  If the window height is tall enough, then 15% is large enough that we don't exercise the multicol "column-width" property, and we get past this testcase without testing.  (And note that the crashtest harness uses a taller window than a normal fresh firefox window does.)

I've got a simplified & more reliable testcase which I'll post shortly. Please use that (or something that crashes more reliably in the crashtest harness) instead of this testcase.
Attachment #8865138 - Flags: review?(dholbert) → review+
Attachment #8866107 - Attachment description: more reliable testcase (warning: may crash your Firefox session when loaded) → more reliable testcase, for use as crashtest (warning: may crash your Firefox session when loaded)
[simplified the testcase a bit more. still crashes for me.]
Attachment #8866107 - Attachment is obsolete: true
Thanks for the improved testcase, I'll switch the crashtest to that version for landing. (Confirmed it still crashes nicely for me, too.)
https://hg.mozilla.org/integration/mozilla-inbound/rev/75a137315cd3af0b75d44ad728eb5d1bfbfc5049
Bug 1362423 - Ensure nsBidiPresUtils::Resolve always clears the NEEDS_BIDI_RESOLUTION flags for the block and continuations, whichever code-path it takes when looking for possible short-circuits. r=dholbert

https://hg.mozilla.org/integration/mozilla-inbound/rev/3bca83e4c7589f60ed53cad5420b018ff5636928
Bug 1362423 - Simplified testcase from the bug report. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/75a137315cd3
https://hg.mozilla.org/mozilla-central/rev/3bca83e4c758
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.