1489153 - AddressSanitizer: use-after-poison /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2795:38 in Type

Reporter

Description

•

6 years ago

Attached file testcase.html — Details

Testcase found while fuzzing mozilla-central rev 0c947d96e8f3.

=================================================================
==1311==ERROR: AddressSanitizer: use-after-poison on address 0x625000330efd at pc 0x7f00725d5c23 bp 0x7ffd23199550 sp 0x7ffd23199548
READ of size 1 at 0x625000330efd thread T0 (file:// Content)
    #0 0x7f00725d5c22 in Type /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2795:38
    #1 0x7f00725d5c22 in IsTableWrapperFrame /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/FrameTypeList.h:100
    #2 0x7f00725d5c22 in nsGridContainerFrame::GridItemInfo::ShouldApplyAutoMinSize(mozilla::WritingMode, mozilla::LogicalAxis, int) const /builds/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:615
    #3 0x7f00725d40d8 in nsGridContainerFrame::Tracks::ResolveIntrinsicSizeStep1(nsGridContainerFrame::GridReflowInput&, nsGridContainerFrame::TrackSizingFunctions const&, int, SizingConstraint, nsGridContainerFrame::LineRange const&, nsGridContainerFrame::GridItemInfo const&) /builds/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:3871:19
    #4 0x7f00725cc87e in nsGridContainerFrame::Tracks::ResolveIntrinsicSize(nsGridContainerFrame::GridReflowInput&, nsTArray<nsGridContainerFrame::GridItemInfo>&, nsGridContainerFrame::TrackSizingFunctions const&, nsGridContainerFrame::LineRange nsGridContainerFrame::GridArea::*, int, SizingConstraint) /builds/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:4312:11
    #5 0x7f00725b79ed in CalculateSizes /builds/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:3830:3
    #6 0x7f00725b79ed in nsGridContainerFrame::GridReflowInput::CalculateTrackSizes(nsGridContainerFrame::Grid const&, mozilla::LogicalSize const&, SizingConstraint) /builds/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:2425
    #7 0x7f00725f182f in nsGridContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsGridContainerFrame.cpp:6000:21
    #8 0x7f00723df15b in nsBlockReflowContext::ReflowBlock(mozilla::LogicalRect const&, bool, nsCollapsingMargin&, int, bool, nsLineBox*, mozilla::ReflowInput&, nsReflowStatus&, mozilla::BlockReflowInput&) /builds/worker/workspace/build/src/layout/generic/nsBlockReflowContext.cpp:309:11
    #9 0x7f00723f98da in nsBlockFrame::ReflowFloat(mozilla::BlockReflowInput&, mozilla::LogicalRect const&, nsIFrame*, mozilla::LogicalMargin&, mozilla::LogicalMargin&, bool, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:6449:9
    #10 0x7f0072328cdf in mozilla::BlockReflowInput::FlowAndPlaceFloat(nsIFrame*) /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:916:13
    #11 0x7f0072326856 in mozilla::BlockReflowInput::AddFloat(nsLineLayout*, nsIFrame*, int) /builds/worker/workspace/build/src/layout/generic/BlockReflowInput.cpp:627:14
    #12 0x7f00726477c9 in AddFloat /builds/worker/workspace/build/src/layout/generic/nsLineLayout.h:181:22
    #13 0x7f00726477c9 in TryToPlaceFloat /builds/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:1549
    #14 0x7f00726477c9 in nsLineLayout::ReflowFrame(nsIFrame*, nsReflowStatus&, mozilla::ReflowOutput*, bool&) /builds/worker/workspace/build/src/layout/generic/nsLineLayout.cpp:971
    #15 0x7f00723e47fe in nsBlockFrame::ReflowInlineFrame(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsIFrame*, LineReflowStatus*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4269:15
    #16 0x7f00723e287a in nsBlockFrame::DoReflowInlineFrames(mozilla::BlockReflowInput&, nsLineLayout&, nsLineList_iterator, nsFlowAreaRect&, int&, nsFloatManager::SavedState*, bool*, LineReflowStatus*, bool) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:4069:5
    #17 0x7f00723d6623 in nsBlockFrame::ReflowInlineFrames(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:3942:9
    #18 0x7f00723ccb3c in nsBlockFrame::ReflowLine(mozilla::BlockReflowInput&, nsLineList_iterator, bool*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2924:5
    #19 0x7f00723be967 in nsBlockFrame::ReflowDirtyLines(mozilla::BlockReflowInput&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:2458:7
    #20 0x7f00723b2249 in nsBlockFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:1292:3
    #21 0x7f007243394b in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:951:14
    #22 0x7f007243a08c in nsColumnSetFrame::ReflowChildren(mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&, nsColumnSetFrame::ReflowConfig const&, bool, nsCollapsingMargin*, nsColumnSetFrame::ColumnBalanceData&) /builds/worker/workspace/build/src/layout/generic/nsColumnSetFrame.cpp:783:7
    #23 0x7f0072441b77 in ReflowColumns /builds/worker/workspace/build/src/layout/generic/nsColumnSetFrame.cpp:473:19
    #24 0x7f0072441b77 in nsColumnSetFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsColumnSetFrame.cpp:1223
    #25 0x7f007243394b in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:951:14
    #26 0x7f0072431237 in nsCanvasFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:803:5
    #27 0x7f007243394b in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, nsSize const&, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:951:14
    #28 0x7f00725672bb in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:606:3
    #29 0x7f0072568e0c in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:730:3
    #30 0x7f007256e1f7 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1120:3
    #31 0x7f007238c5b8 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:995:14
    #32 0x7f007238acdb in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:338:7
    #33 0x7f00720e2591 in mozilla::PresShell::DoReflow(nsIFrame*, bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:9020:11
    #34 0x7f00720fd188 in mozilla::PresShell::ProcessReflowCommands(bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:9193:24
    #35 0x7f00720fb301 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4351:11
    #36 0x7f007206ef0a in FlushPendingNotifications /builds/worker/workspace/build/src/layout/base/nsIPresShell.h:577:5
    #37 0x7f007206ef0a in nsRefreshDriver::Tick(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1927
    #38 0x7f0072081862 in TickDriver /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:324:13
    #39 0x7f0072081862 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:299
    #40 0x7f0072081391 in mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:317:5
    #41 0x7f0072084601 in RunRefreshDrivers /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:755:5
    #42 0x7f0072084601 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:671
    #43 0x7f00720840db in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:571:9
    #44 0x7f0072b402b6 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /builds/worker/workspace/build/src/layout/ipc/VsyncChild.cpp:78:16
    #45 0x7f006997d00d in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20
    #46 0x7f006970afb8 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:2280:28
    #47 0x7f0068f620fe in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2248:25
    #48 0x7f0068f5da2e in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2175:17
    #49 0x7f0068f5fe8d in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2012:5
    #50 0x7f0068f60be7 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2045:15
    #51 0x7f0067d6d720 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1161:14
    #52 0x7f0067d763f5 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #53 0x7f0068f6c1ce in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #54 0x7f0068e6d88c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #55 0x7f0068e6d88c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #56 0x7f0068e6d88c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #57 0x7f007199e8c6 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #58 0x7f0075d2867e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:944:22
    #59 0x7f0068e6d88c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #60 0x7f0068e6d88c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #61 0x7f0068e6d88c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #62 0x7f0075d27735 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:770:34
    #63 0x4f6b61 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #64 0x4f6b61 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:287
    #65 0x7f00896e2b96 in __libc_start_main /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
    #66 0x425f3c in _start (/home/forb1dden/builds/mc-asan/firefox+0x425f3c)

0x625000330efd is located 7677 bytes inside of 8192-byte region [0x62500032f100,0x625000331100)
allocated by thread T0 (file:// Content) here:
    #0 0x4c6683 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f0067d00be3 in AllocateChunk /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f0067d00be3 in InternalAllocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228
    #3 0x7f0067d00be3 in Allocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f0067d00be3 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f007237e95a in AllocateByFrameID /builds/worker/workspace/build/src/layout/base/nsPresArena.h:39:12
    #6 0x7f007237e95a in AllocateFrame /builds/worker/workspace/build/src/layout/base/nsIPresShell.h:206
    #7 0x7f007237e95a in operator new /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:34
    #8 0x7f007237e95a in NS_NewViewportFrame(nsIPresShell*, mozilla::ComputedStyle*) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:31
    #9 0x7f00721ab67e in nsCSSFrameConstructor::ConstructRootFrame() /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2661:5
    #10 0x7f00720da5f1 in mozilla::PresShell::Initialize() /builds/worker/workspace/build/src/layout/base/PresShell.cpp:1799:36
    #11 0x7f006bd3ff67 in nsContentSink::StartLayout(bool) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:1274:26
    #12 0x7f006a744052 in nsHtml5TreeOpExecutor::StartLayout(bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:673:18
    #13 0x7f006a73f49e in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**, bool*, bool*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOperation.cpp:1204:17
    #14 0x7f006a73c3f6 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:489:17
    #15 0x7f006a74889b in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:121:18
    #16 0x7f0067d30305 in mozilla::SchedulerGroup::Runnable::Run() /builds/worker/workspace/build/src/xpcom/threads/SchedulerGroup.cpp:337:32
    #17 0x7f0067d6d720 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1161:14
    #18 0x7f0067d763f5 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #19 0x7f0068f6c1ce in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #20 0x7f0068e6d88c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #21 0x7f0068e6d88c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #22 0x7f0068e6d88c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #23 0x7f007199e8c6 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #24 0x7f0075d2867e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:944:22
    #25 0x7f0068e6d88c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #26 0x7f0068e6d88c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #27 0x7f0068e6d88c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #28 0x7f0075d27735 in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:770:34
    #29 0x4f6b61 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #30 0x4f6b61 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:287

SUMMARY: AddressSanitizer: use-after-poison /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2795:38 in Type
Shadow bytes around the buggy address:
  0x0c4a8005e180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8005e190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8005e1a0: 00 00 00 00 00 f7 f7 00 00 00 00 00 00 00 00 00
  0x0c4a8005e1b0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a8005e1c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c4a8005e1d0: 00 00 00 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7[f7]
  0x0c4a8005e1e0: f7 f7 f7 f7 f7 f7 f7 00 00 00 00 00 00 00 00 00
  0x0c4a8005e1f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 f7 f7 f7
  0x0c4a8005e200: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a8005e210: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a8005e220: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==1311==ABORTING

Flags: in-testsuite?

Daniel Veditz [:dveditz]

Updated

•

6 years ago

Group: core-security → layout-core-security

Keywords: csectype-framepoisoning, sec-low

Mats Palmgren (inactive)

Assignee

Comment 1

•

6 years ago

Attached file Frame tree at the start of nsGridContainerFrame::Reflow — Details

Note that the two ColumnSet frames are grid items.

Assignee: nobody → mats

Mats Palmgren (inactive)

Assignee

Comment 2

•

6 years ago

Attached file stack for destroying 0x625000f579d0 (red in the frame dump) — Details

At some point during track sizing we need to know the size
contribution of the first item (blue) so we call MeasuringReflow
on it.  This leads to its next-in-flow, our second grid item
frame, to be destroyed.

Mats Palmgren (inactive)

Assignee

Comment 3

•

6 years ago

Initially we reflow this grid container:

GridContainer(html)(3)@625000c79b28 <
  ColumnSet(body)(1)@625000c79ce8
>

the item is incomplete so we create a next-in-flow on our OverflowList:

GridContainer(html)(3)@625000c79b28 <
  ColumnSet(body)(1)@625000c79ce8  next-in-flow=625000d409d0
OverflowList 625000d40a60 <
  ColumnSet(body)(1)@625000d409d0 prev-in-flow=625000c79ce8
>

then some script inserts a DOM node and while doing that we need to
figure out where its frame should be inserted and to do that we
call DrainSelfOverflowList on the grid container above:

(rr) bt
#0  0x00007f61d84e9e5a in nsFrameList::InsertFrames(... aPrevSibling=0x625000c79ce8
#1  0x00007f61d8316faf in nsFrameList::AppendFrames
#2  0x00007f61d859f337 in MergeSortedFrameLists
#3  0x00007f61d85a2bdd in nsGridContainerFrame::DrainSelfOverflowList()
#4  0x00007f61d81434a2 in FindAppendPrevSibling(nsIFrame*, nsIFrame*)
#5  0x00007f61d8147bf3 in nsCSSFrameConstructor::ContentAppended

So now we have:

GridContainer(html)(3)@625000c79b28 <
  ColumnSet(body)(1)@625000c79ce8 next-in-flow=625000d409d0
  ColumnSet(body)(1)@625000d409d0 prev-in-flow=625000c79ce8
>

Then we Reflow that grid container and find that the first child
is complete...

Mats Palmgren (inactive)

Assignee

Comment 4

•

6 years ago

https://treeherder.mozilla.org/#/jobs?repo=try&revision=22b472df3ed0c05d6c9f9af5c11882656941f722

Mats Palmgren (inactive)

Assignee

Updated

•

6 years ago

Keywords: sec-low → sec-other

OS: Unspecified → All

Priority: -- → P2

Hardware: Unspecified → All

Mats Palmgren (inactive)

Assignee

Comment 5

•

6 years ago

Attached patch Push any child next-in-flows in our principal list to OverflowList before starting our reflow. (obsolete) — Details — Splinter Review

The NS_FRAME_NO_DELETE_NEXT_IN_FLOW_CHILD addition in
MeasuringReflow avoids a virtual GetNextInFlow() call
in ReflowChild, so it's a small optimization.
It also feels like MeasuringReflow should be idempotent
and not mutate the frame tree by deleting next-in-flows
as happened here.

Attachment #9007668 - Flags: review?(dholbert)

Daniel Holbert [:dholbert]

Comment 6

•

6 years ago

Comment on attachment 9007668 [details] [diff] [review]
Push any child next-in-flows in our principal list to OverflowList before starting our reflow.

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

::: layout/generic/nsGridContainerFrame.cpp
@@ +5885,5 @@
> +  // Push any child next-in-flows in our principal list to OverflowList.
> +  if (HasAnyStateBits(NS_STATE_GRID_HAS_CHILD_NIFS)) {
> +    nsIFrame* firstChild = GetChildList(kPrincipalList).FirstChild();
> +    nsFrameList framesToPush;
> +    for (auto child = firstChild; child; child = child->GetNextSibling()) {

I was going to suggest that we simplify this to use a range-based for loop, i.e..:
  for (auto child : GetChildList(kPrincipalList)) {
... but then I realized that we mutate the list while we're iterating, which I think means we can't use range-based for loops.

Perhaps worth adding a comment here to document that we're explicitly/intentionally *not* using range-based for loop & why, to reduce the likelihood that someone comes along and inadvertantly "modernizes" this without noticing the mutation-during-iteration?

@@ +5891,5 @@
> +        if (childNIF->GetParent() == this) {
> +          for (auto c = child->GetNextSibling(); c; c = c->GetNextSibling()) {
> +            if (c == childNIF) {
> +              // child's next-in-flow is in our principal child list, push it.
> +              StealFrame(childNIF);

Is StealFrame the right thing to be calling here? (Fair warning, I don't recall the precise semantics around when/how to call it -- I'm just recalling / reading up on what it seems to do.)

Observations:
- It looks like nsContainerFrame::StealFrame mostly/primarily checks the OverflowflowContainer frame-lists, and *then* checks the principal child list as an afterthought, to handle a special case for nsColumnSetFrame and nsCanvasFrame :
https://dxr.mozilla.org/mozilla-central/rev/9bb0f7fc73ad2109e4ea777e9ba65a16a6acc9cd/layout/generic/nsContainerFrame.cpp#1334-1339

- So: since we *already know* childNIF is in our principal child list, then I think the only line from StealFrame that will run is this one:
   mFrames.StartRemoveFrame(aChild);

- Plus: even that function (StartRemoveFrame) seems to be tentative -- it's for scenarios where we're not sure which frame list (of several) a frame might be in, per its documentation here:
https://dxr.mozilla.org/mozilla-central/rev/9bb0f7fc73ad2109e4ea777e9ba65a16a6acc9cd/layout/generic/nsFrameList.h#174-193

So. Bottom-line, maybe we should replace this StealFrame call with something more direct like:
  GetChildList(kPrincipalList).RemoveFrame(childNIF)
...or something like that?  (I'm not sure, just a guess from my reading so far. Given that we *know* childNIF is in our principal list, it seems like we can skip the tentative stuff...)

@@ +6562,5 @@
>    AutoFrameListPtr overflowFrames(PresContext(), StealOverflowFrames());
>    if (overflowFrames) {
>      ::MergeSortedFrameLists(mFrames, *overflowFrames, GetContent());
> +    // We set a frame bit to push them again in Reflow() to avoid creating
> +    // multiple grid items per container for the same content.

"per container" is kinda vague here.  Maybe say "per container frame", or even "per grid container frame"?  Or "per grid container fragment"?

(Nice to clarify this language, because we *can and do* create multiple grid items per grid container *element*, if the grid container element and the item are both fragmented.  And without the clarified language, this comment kinda sounds like it's saying that scenario would be forbidden/unexpected.)

Attachment #9007668 - Flags: review?(dholbert) → review-

Mats Palmgren (inactive)

Assignee

Comment 7

•

6 years ago

(In reply to Daniel Holbert [:dholbert] from comment #6)
> So. Bottom-line, maybe we should replace this StealFrame call with something
> more direct like:
>   GetChildList(kPrincipalList).RemoveFrame(childNIF)
> ...or something like that?  (I'm not sure, just a guess from my reading so
> far. Given that we *know* childNIF is in our principal list, it seems like
> we can skip the tentative stuff...)

Generally, StealFrame is better since it avoids making assumptions
about which child list the given child frame lives on, and that's why
using mFrames directly is generally frowned upon because people tend to
forget about the various overflow lists.  But yes, we can use
mFrames.RemoveFrame directly here since we're operating on 'this' and
we already know that childNIF is on that list.
(GetChildList() can't be used since it returns a const ref)

(StartRemoveFrame/ContinueRemoveFrame can be used when you know it's on
a specific subset of lists, in cases where performance matters.)

Mats Palmgren (inactive)

Assignee

Comment 8

•

6 years ago

Attached patch Push any child next-in-flows in our principal list to OverflowList before starting our reflow. — Details — Splinter Review

Attachment #9007668 - Attachment is obsolete: true

Attachment #9007943 - Flags: review?(dholbert)

Mats Palmgren (inactive)

Assignee

Updated

•

6 years ago

Blocks: 1490032

Daniel Holbert [:dholbert]

Comment 9

•

6 years ago

Comment on attachment 9007943 [details] [diff] [review]
Push any child next-in-flows in our principal list to OverflowList before starting our reflow.

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

r=me

Attachment #9007943 - Flags: review?(dholbert) → review+

Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)

Comment 10

•

6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/1719e0b4140df4657af0c05d18665f0af0f3e6dd
https://hg.mozilla.org/mozilla-central/rev/1719e0b4140d

Group: layout-core-security → core-security-release

Status: NEW → RESOLVED

Closed: 6 years ago

status-firefox64: affected → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla64

Ryan VanderMeulen [:RyanVM]

Comment 11

•

6 years ago

Is this worth backporting to Beta?

status-firefox62: --- → wontfix

status-firefox63: --- → affected

status-firefox-esr60: --- → wontfix

Flags: needinfo?(mats)

Mats Palmgren (inactive)

Assignee

Comment 12

•

6 years ago

Sure, if we're still somewhat early in the cycle I think the risk/benefit
is probably worth it.

Flags: needinfo?(mats)

Ryan VanderMeulen [:RyanVM]

Comment 13

•

6 years ago

Can you please do the request? :)

Flags: needinfo?(mats)

Mats Palmgren (inactive)

Assignee

Comment 14

•

6 years ago

Comment on attachment 9007943 [details] [diff] [review]
Push any child next-in-flows in our principal list to OverflowList before starting our reflow.

Approval Request Comment
[Feature/Bug causing the regression]:grid fragmentation feature
[User impact if declined]:crash
[Is this code covered by automated tests?]:not landed yet
[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]:no
[Is the change risky?]:no
[Why is the change risky/not risky?]:it's an edge that isn't triggered on normal web pages, and it's a fairly trivial fix
[String changes made/needed]:no

Flags: needinfo?(mats)

Attachment #9007943 - Flags: approval-mozilla-beta?

Pascal Chevrel:pascalc (PTO until April 26)

Comment 15

•

6 years ago

Comment on attachment 9007943 [details] [diff] [review]
Push any child next-in-flows in our principal list to OverflowList before starting our reflow.

Approved for 63 beta 7, thanks.

Attachment #9007943 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Ryan VanderMeulen [:RyanVM]

Comment 16

•

6 years ago

uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/57de52c17585

status-firefox63: affected → fixed

Bogdan Maris, Desktop QA

Updated

•

6 years ago

Flags: qe-verify+

Whiteboard: [post-critsmash-triage]

Cristian Baica [:cbaica], Release Desktop QA

Comment 17

•

6 years ago

I have managed to reproduce the issue using Fx64.0a1 on Ubuntu 16.04 x64.

The issue is verified as fixed using Fx63.0b8(asan build) on Ubuntu and Fx63.0b7 and latest Fx64.0a1 on Windows 10 x64, macOS 10.14 and Ubuntu 16.04. The console no longer displays the error when loading the testcase.html file in the browser.

Status: RESOLVED → VERIFIED

status-firefox63: fixed → verified

status-firefox64: fixed → verified

Flags: qe-verify+

Al Billings [:abillings - ex-MoCo]

Updated

•

6 years ago

Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63-]

Daniel Veditz [:dveditz]

Updated

•

4 years ago

Group: core-security-release

Sylvestre Ledru [:Sylvestre]

Updated

•

4 years ago

Blocks: asan-maintenance

testcase.html 6 years ago Jason Kratzer [:jkratzer] 1.43 KB, text/html		Details
Frame tree at the start of nsGridContainerFrame::Reflow 6 years ago Mats Palmgren (inactive) 6.86 KB, text/html		Details
stack for destroying 0x625000f579d0 (red in the frame dump) 6 years ago Mats Palmgren (inactive) 5.60 KB, text/plain		Details
Push any child next-in-flows in our principal list to OverflowList before starting our reflow. 6 years ago Mats Palmgren (inactive) 4.86 KB, patch	dholbert : review-	Details \| Diff \| Splinter Review
Push any child next-in-flows in our principal list to OverflowList before starting our reflow. 6 years ago Mats Palmgren (inactive) 4.93 KB, patch	dholbert : review+ pascalc : approval-mozilla-beta+	Details \| Diff \| Splinter Review