Closed
Bug 1343606
Opened 7 years ago
Closed 7 years ago
Crash near null [@ nsSplittableFrame::GetNextInFlow]
Categories
(Core :: Layout, defect)
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: MatsPalmgren_bugz)
References
Details
(5 keywords)
Crash Data
Attachments
(3 files)
666 bytes,
text/html
|
Details | |
1.20 KB,
patch
|
jfkthame
:
review+
lizzard
:
approval-mozilla-aurora+
lizzard
:
approval-mozilla-beta+
lizzard
:
approval-mozilla-release+
lizzard
:
approval-mozilla-esr52+
dveditz
:
sec-approval+
|
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
Reporter | ||
Comment 1•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → mats
Assignee | ||
Comment 2•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
Yeah, commenting out the TraverseFrames call there makes it not crash...
Assignee | ||
Comment 5•7 years ago
|
||
I think this should fix it... https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac7552ffb105a5f418e44ce53bec6027bcf784dd
Comment 6•7 years ago
|
||
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.
Assignee | ||
Comment 7•7 years ago
|
||
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.
Assignee | ||
Comment 8•7 years ago
|
||
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...
Updated•7 years ago
|
Blocks: 410857
status-firefox51:
--- → unaffected
status-firefox52:
--- → affected
status-firefox53:
--- → affected
status-firefox-esr45:
--- → unaffected
status-firefox-esr52:
--- → affected
tracking-firefox52:
--- → ?
tracking-firefox53:
--- → ?
tracking-firefox54:
--- → ?
tracking-firefox-esr52:
--- → ?
Flags: in-testsuite?
Keywords: regression
Version: Trunk → 52 Branch
Updated•7 years ago
|
Crash Signature: [@ nsSplittableFrame::GetNextInFlow]
Comment 9•7 years ago
|
||
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?
Comment 10•7 years ago
|
||
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.
Assignee | ||
Comment 11•7 years ago
|
||
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.
Comment 12•7 years ago
|
||
(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.
Comment 13•7 years ago
|
||
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)
Comment 14•7 years ago
|
||
Does this also affect 52 fennec? do we need a new build there too?
Comment 15•7 years ago
|
||
Presumably so.
Assignee | ||
Comment 16•7 years ago
|
||
Flags: needinfo?(mats)
Assignee | ||
Comment 17•7 years ago
|
||
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 18•7 years ago
|
||
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+
Assignee | ||
Comment 19•7 years ago
|
||
(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.
Comment 20•7 years ago
|
||
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.
Comment 21•7 years ago
|
||
Marking this s-s, kind of speculatively based on conversations with Dan and jfkthame.
Group: core-security
Comment 22•7 years ago
|
||
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
Updated•7 years ago
|
Attachment #8842944 -
Flags: sec-approval+
Comment 23•7 years ago
|
||
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+
Comment 24•7 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/1c676fbbcdbf https://hg.mozilla.org/releases/mozilla-beta/rev/25ee9d2ee428 https://hg.mozilla.org/releases/mozilla-release/rev/25ee9d2ee428 https://hg.mozilla.org/releases/mozilla-esr52/rev/25ee9d2ee428
Flags: in-testsuite? → in-testsuite+
Updated•7 years ago
|
Comment 25•7 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/6b35c4986b0e https://hg.mozilla.org/mozilla-central/rev/e49c530bd068
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Updated•7 years ago
|
Group: core-security → core-security-release
Updated•7 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•