Closed Bug 1343606 Opened 3 years ago Closed 3 years ago
Crash near null [@ ns
Splittable Frame::Get Next In Flow]
666 bytes, text/html
1.20 KB, patch
|Details | Diff | Splinter Review|
1.46 KB, patch
|Details | Diff | Splinter Review|
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
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...)
That sounds plausible at first glance, certainly.
Yeah, commenting out the TraverseFrames call there makes it not crash...
I think this should fix it... https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7552ffb105a5f418e44ce53bec6027bcf784dd
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...
Version: Trunk → 52 Branch
Crash Signature: [@ nsSplittableFrame::GetNextInFlow]
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!
Does this also affect 52 fennec? do we need a new build there too?
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 firstname.lastname@example.org: 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)
Comment on attachment 8842944 [details] [diff] [review] fix Last minute uplift for 52 release and 52esr.
You need to log in before you can comment on or make changes to this bug.