Closed Bug 1343606 Opened 3 years ago Closed 3 years ago

Crash near null [@ nsSplittableFrame::GetNextInFlow]

Categories

(Core :: Layout, defect, critical)

52 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox-esr45 --- unaffected
firefox51 --- unaffected
firefox52 blocking fixed
firefox-esr52 52+ fixed
firefox53 + fixed
firefox54 + fixed

People

(Reporter: truber, Assigned: mats)

References

(Blocks 1 open bug)

Details

(5 keywords)

Crash Data

Attachments

(3 files)

The attached testcase crashes near null in mozilla-central rev 34c6c2f302e7.
The testcase is intermittent, crashing in about 4 of 5 runs.

==4768==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000058 (pc 0x7f7c5e23c538 bp 0x7ffd8922b490 sp 0x7ffd8922b490 T0)
    #0 0x7f7c5e23c537 in nsSplittableFrame::GetNextInFlow() const /home/worker/workspace/build/src/layout/generic/nsSplittableFrame.cpp:144:10
    #1 0x7f7c5e073ef5 in nsBlockInFlowLineIterator::FindValidLine() /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:5727:43
    #2 0x7f7c5df85708 in BidiParagraphData::AdvanceLineIteratorToFrame(nsIFrame*, nsBlockInFlowLineIterator*, nsIFrame*&) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:360:9
    #3 0x7f7c5de95244 in BidiParagraphData::AppendFrame(nsIFrame*, nsBlockInFlowLineIterator*, nsIContent*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:248:5
    #4 0x7f7c5de8fa3b in nsBidiPresUtils::TraverseFrames(nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:1072:7
    #5 0x7f7c5de8f9ef in nsBidiPresUtils::TraverseFrames(nsBlockInFlowLineIterator*, nsIFrame*, BidiParagraphData*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:1214:9
    #6 0x7f7c5de8e794 in nsBidiPresUtils::Resolve(nsBlockFrame*) /home/worker/workspace/build/src/layout/base/nsBidiPresUtils.cpp:714:7
    #7 0x7f7c5e03d96b in ResolveBidi /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:7454:10
    #8 0x7f7c5e03d96b in nsBlockFrame::GetMinISize(nsRenderingContext*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:736
    #9 0x7f7c5e0c1d37 in nsFrame::ComputeSize(nsRenderingContext*, mozilla::WritingMode, mozilla::LogicalSize const&, int, mozilla::LogicalSize const&, mozilla::LogicalSize const&, mozilla::LogicalSize const&, nsIFrame::ComputeSizeFlags) /
home/worker/workspace/build/src/layout/generic/nsFrame.cpp:4940:35
    #10 0x7f7c5dfff16e in mozilla::ReflowInput::InitConstraints(nsPresContext*, mozilla::LogicalSize const&, nsMargin const*, nsMargin const*, nsIAtom*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:2451:9
    #11 0x7f7c5dff5b54 in mozilla::ReflowInput::Init(nsPresContext*, mozilla::LogicalSize const*, nsMargin const*, nsMargin const*) /home/worker/workspace/build/src/layout/generic/ReflowInput.cpp:399:3
    #12 0x7f7c5e1a9256 in nsGridContainerFrame::ReflowInFlowChild(nsIFrame*, nsGridContainerFrame::GridItemInfo const*, nsSize, mozilla::Maybe<int> const&, nsGridContainerFrame::Fragmentainer const*, nsGridContainerFrame::GridReflowInput c
onst&, mozilla::LogicalRect const&, mozilla::ReflowOutput&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:5240:15
    #13 0x7f7c5e1ae258 in nsGridContainerFrame::ReflowRowsInFragmentainer(nsGridContainerFrame::GridReflowInput&, mozilla::LogicalRect const&, mozilla::ReflowOutput&, nsReflowStatus&, nsGridContainerFrame::Fragmentainer&, nsSize const&, ns
TArray<nsGridContainerFrame::GridItemInfo const*> const&, unsigned int, unsigned int, int, int) /home/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:5626:5
    #14 0x7f7c5e1acaf8 in nsGridContainerFrame::ReflowInFragmentainer(nsGridContainerFrame::GridReflowInput&, mozilla::LogicalRect const&, mozilla::ReflowOutput&, nsReflowStatus&, nsGridContainerFrame::Fragmentainer&, nsSize const&) /home/
worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:5528:10
    #15 0x7f7c5e1b17ec in nsGridContainerFrame::ReflowChildren(nsGridContainerFrame::GridReflowInput&, mozilla::LogicalRect const&, mozilla::ReflowOutput&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsGridContainerFra
me.cpp:5844:13
    #16 0x7f7c5e1b4460 in nsGridContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /home/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:6156:11
    #17 0x7f7c5e0654b4 in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /home/worker/workspace/build/src
/layout/generic/nsBlockReflowContext.cpp:306:3
Attached file testcase.html
Assignee: nobody → mats
Oh darn, this looks like a regression from bug 410857...
We should have passed the first child frame in overflowLines, not the first child
of the principal list here:
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/layout/base/nsBidiPresUtils.cpp#714

Jonathan, do you agree?

(I'll debug this more...)
Flags: needinfo?(jfkthame)
That sounds plausible at first glance, certainly.
Flags: needinfo?(jfkthame)
Yeah, commenting out the TraverseFrames call there makes it not crash...
In my local debug build, that does indeed seem to prevent the crash.

I still hit an assertion:

###!!! ASSERTION: conflicting overflow containers lists: '!(overflowContainers && GetPrevInFlow() && static_cast<nsContainerFrame*>(GetPrevInFlow()) ->GetPropTableFrames(ExcessOverflowContainersProperty()))', file /Users/jkew/mozdev/mozilla-central/layout/generic/nsContainerFrame.cpp, line 1674

when loading the testcase, but previously that was followed by a second and third (fatal) assertion:

###!!! ASSERTION: Can't find frame in lines!: 'hasNext', file /Users/jkew/mozdev/mozilla-central/layout/base/nsBidiPresUtils.cpp, line 361
Assertion failure: mCurrent != mListLink (running past end), at /Users/jkew/mozdev/mozilla-central/layout/generic/nsLineBox.h:800

whereas with the fix from comment 5, we seem to continue safely.
Comment on attachment 8842944 [details] [diff] [review]
fix

I think the child frame change it obvious, so I won't go into that,
but the "bpd.mPrevFrame = nullptr" part is not obvious.

Apparently, there's an invariant in this code that it should be
reset to null before starting to process a new line.
This comes from:
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/layout/base/nsBidiPresUtils.cpp#248
note that the last param there is a nsIFrame*&
http://searchfox.org/mozilla-central/rev/31b6089ce26fa76459642765115605d50a6c67b4/layout/base/nsBidiPresUtils.cpp#346
If we don't reset it before the TraverseFrames call in this patch,
then IsFrameInCurrentLine gets very confused and AdvanceLineIteratorToFrame
can't find the frame (i.e. the first child frame in the overflow line
list in this case) and it asserts on line 361 and eventually crashes.
Yes, I see that assertion too.  I think it's unrelated to the bidi crash here.
I'll spawn that off as a follow-up bug...
Blocks: 410857
Flags: in-testsuite?
Keywords: regression
Version: Trunk → 52 Branch
Crash Signature: [@ nsSplittableFrame::GetNextInFlow]
Blocks: 1343948
A see a couple crashes with this top frame on crash-stats, but most are from older versions with a different root cause presumably. Is this something we're worried about hitting in the wild more often once 52 goes to release?
Yes, I think so; even if there are other ways to crash with the same top frame, there's a definite newly-introduced flaw here that we really don't want to ship.
Yeah, it's hard to know how common this would be but given the uncertainty
I would strongly recommend that we don't ship without this fix if possible.
(In reply to Mats Palmgren (:mats) from comment #8)
> Yes, I see that assertion too.  I think it's unrelated to the bidi crash
> here.

FWIW, I confirmed that assertion occurs with a build prior to the landing of bug 410857, so it's definitely unrelated to the change we made there.
OK let's get this into our 52 build 2 today since mats and jfkthame agree it's worse than it looks at first glancde and we shouldn't ship without the fix. Can you request uplift asap please? don't forget esr!
Flags: needinfo?(mats)
Does this also affect 52 fennec? do we need a new build there too?
Presumably so.
Attached patch crashtestSplinter Review
Flags: needinfo?(mats)
Comment on attachment 8842944 [details] [diff] [review]
fix

Try run looks green, except there were a few unexpected-pass from the assert(1)
I pushed with.  I've adjusted that to assert(0-1) in the crashtest patch now.
Attachment #8842944 - Flags: review?(jfkthame)
Comment on attachment 8842944 [details] [diff] [review]
fix

Patch looks good to me, and works as expected when tested. Thanks!
Attachment #8842944 - Flags: review?(jfkthame) → review+
(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #14)
> Does this also affect 52 fennec? do we need a new build there too?

This affects all products built from branches where bug 410857 landed.
Pushed by mpalmgren@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b35c4986b0e
Pass the correct first child frame for the lines we're going to traverse, and reset bpd.mPrevFrame since we're starting at a new line.  r=jfkthame
https://hg.mozilla.org/integration/mozilla-inbound/rev/e49c530bd068
Crashtest.
Marking this s-s, kind of speculatively based on conversations with Dan and jfkthame.
Dan suggested marking this sec-high. Sounds like we think it might be both easily findable with fuzzing and easily exploitable.  I don't want to have to explain that individually to 10 more people on IRC, so I'd like to hide the bug (also, in case we need to slip the release date for any reason)
Keywords: sec-high
Attachment #8842944 - Flags: sec-approval+
Comment on attachment 8842944 [details] [diff] [review]
fix

Last minute uplift for 52 release and 52esr.
Attachment #8842944 - Flags: approval-mozilla-release+
Attachment #8842944 - Flags: approval-mozilla-esr52+
Attachment #8842944 - Flags: approval-mozilla-beta+
Attachment #8842944 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/6b35c4986b0e
https://hg.mozilla.org/mozilla-central/rev/e49c530bd068
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Group: core-security → core-security-release
Group: core-security-release
See Also: → 1549867
You need to log in before you can comment on or make changes to this bug.