Closed Bug 1489153 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

VERIFIED FIXED
mozilla64
Tracking Status
firefox-esr60 --- wontfix
firefox62 --- wontfix
firefox63 --- verified
firefox64 --- verified

People

(Reporter: jkratzer, Assigned: mats)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [post-critsmash-triage][adv-main63-])

Attachments

(4 files, 1 obsolete file)

Attached file testcase.html
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?
Group: core-security → layout-core-security
Note that the two ColumnSet frames are grid items.
Assignee: nobody → mats
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.
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...
Keywords: sec-lowsec-other
OS: Unspecified → All
Priority: -- → P2
Hardware: Unspecified → All
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)
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-
(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.)
Blocks: 1490032
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+
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: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Is this worth backporting to Beta?
Flags: needinfo?(mats)
Sure, if we're still somewhat early in the cycle I think the risk/benefit
is probably worth it.
Flags: needinfo?(mats)
Can you please do the request? :)
Flags: needinfo?(mats)
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?
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+
Flags: qe-verify+
Whiteboard: [post-critsmash-triage]
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
Flags: qe-verify+
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main63-]
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.