1159127 - Negative-size-param in nsTableFrame::InsertCol, with "writing-mode: vertical-rl"

Status: VERIFIED FIXED

(Core :: Layout: Tables, defect)

Description

[Abhishek Arya]

Attachment: Testcase

==4114==ERROR: AddressSanitizer: negative-size-param: (size=-16)
    #0 0x4a0fd0 in __asan_memmove _asan_rtl_
    #1 0x7f09dd574dfd in MoveElements /build/firefox/src/objdir-ff-asan/layout/tables/../../dist/include/nsTArray.h:588:5
    #2 0x7f09dd574dfd in ShiftData /build/firefox/src/objdir-ff-asan/layout/tables/../../dist/include/nsTArray-inl.h:268
    #3 0x7f09dd574dfd in InsertElementAt<nsTableColFrame *> /build/firefox/src/objdir-ff-asan/layout/tables/../../dist/include/nsTArray.h:1295
    #4 0x7f09dd574dfd in nsTableFrame::InsertCol(nsTableColFrame&, int) /build/firefox/src/layout/tables/nsTableFrame.cpp:578
    #5 0x7f09dd574933 in nsTableColGroupFrame::AddColsToTable(int, bool, nsFrameList::Slice const&) /build/firefox/src/layout/tables/nsTableColGroupFrame.cpp:88:5
    #6 0x7f09dd5a0974 in InsertColGroups /build/firefox/src/layout/tables/nsTableFrame.cpp:562:5
    #7 0x7f09dd5a0974 in nsTableFrame::AppendFrames(mozilla::layout::FrameChildListID, nsFrameList&) /build/firefox/src/layout/tables/nsTableFrame.cpp:2256
    #8 0x7f09dd5a170e in nsTableFrame::InsertFrames(mozilla::layout::FrameChildListID, nsIFrame*, nsFrameList&) /build/firefox/src/layout/tables/nsTableFrame.cpp:2304:5
    #9 0x7f09dd04a75c in InsertFrames /build/firefox/src/layout/base/nsFrameManager.cpp:486:5
    #10 0x7f09dd04a75c in nsCSSFrameConstructor::AppendFramesToParent(nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&, nsIFrame*, bool) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:6288
    #11 0x7f09dd0565e1 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, bool) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:7290:5
    #12 0x7f09dd04fd3c in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:6888:5
    #13 0x7f09dd04fdf5 in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:6895:7
    #14 0x7f09dd04fdf5 in nsCSSFrameConstructor::CreateNeededFrames(nsIContent*) /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:6895:7
    #15 0x7f09dd056f2a in nsCSSFrameConstructor::CreateNeededFrames() /build/firefox/src/layout/base/nsCSSFrameConstructor.cpp:6910:5
    #16 0x7f09dcfc46bf in mozilla::RestyleManager::ProcessPendingRestyles() /build/firefox/src/layout/base/RestyleManager.cpp:1627:3
    #17 0x7f09dd1ee5a1 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /build/firefox/src/layout/base/nsPresShell.cpp:4273:9
    #18 0x7f09dcf54100 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:1679:11
    #19 0x7f09dcf5e723 in TickDriver /build/firefox/src/layout/base/nsRefreshDriver.cpp:198:5
    #20 0x7f09dcf5e723 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:189
    #21 0x7f09dcf605e9 in RunRefreshDrivers /build/firefox/src/layout/base/nsRefreshDriver.cpp:440:5
    #22 0x7f09dcf605e9 in TickRefreshDriver /build/firefox/src/layout/base/nsRefreshDriver.cpp:374
    #23 0x7f09dcf605e9 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:345
    #24 0x7f09dd7ecdb4 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /build/firefox/src/layout/ipc/VsyncChild.cpp:63:5
    #25 0x7f09d7fd0b8b in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /build/firefox/src/objdir-ff-asan/ipc/ipdl/./PVsyncChild.cpp:224:20
    #26 0x7f09d7b67eb7 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /build/firefox/src/objdir-ff-asan/ipc/ipdl/./PBackgroundChild.cpp:1082:16
    #27 0x7f09d7af28fb in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /build/firefox/src/ipc/glue/MessageChannel.cpp:1248:14
    #28 0x7f09d7aefdaa in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) /build/firefox/src/ipc/glue/MessageChannel.cpp:1167:9
    #29 0x7f09d7ae2db0 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() /build/firefox/src/ipc/glue/MessageChannel.cpp:1151:9
    #30 0x7f09d7a84c1d in RunTask /build/firefox/src/ipc/chromium/src/base/message_loop.cc:361:3
    #31 0x7f09d7a84c1d in MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) /build/firefox/src/ipc/chromium/src/base/message_loop.cc:369
    #32 0x7f09d7a858ea in MessageLoop::DoWork() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:456:13
    #33 0x7f09d7af9f02 in mozilla::ipc::DoWorkRunnable::Run() /build/firefox/src/ipc/glue/MessagePump.cpp:233:3
    #34 0x7f09d71e67e5 in nsThread::ProcessNextEvent(bool, bool*) /build/firefox/src/xpcom/threads/nsThread.cpp:866:7
    #35 0x7f09d7245b5c in NS_ProcessNextEvent(nsIThread*, bool) /build/firefox/src/xpcom/glue/nsThreadUtils.cpp:265:10
    #36 0x7f09d71e593a in nsThread::Shutdown() /build/firefox/src/xpcom/threads/nsThread.cpp:667:5
    #37 0x7f09d71f9620 in apply<nsIThread, nsresult (nsIThread::*)()> /build/firefox/src/objdir-ff-asan/xpcom/threads/../../dist/include/nsThreadUtils.h:602:5
    #38 0x7f09d71f9620 in nsRunnableMethodImpl<nsresult (nsIThread::*)(), true>::Run() /build/firefox/src/objdir-ff-asan/xpcom/threads/../../dist/include/nsThreadUtils.h:809
    #39 0x7f09d71e67e5 in nsThread::ProcessNextEvent(bool, bool*) /build/firefox/src/xpcom/threads/nsThread.cpp:866:7
    #40 0x7f09d7245b5c in NS_ProcessNextEvent(nsIThread*, bool) /build/firefox/src/xpcom/glue/nsThreadUtils.cpp:265:10
    #41 0x7f09d7af96ae in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /build/firefox/src/ipc/glue/MessagePump.cpp:99:21
    #42 0x7f09d7a83891 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #43 0x7f09d7a83891 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #44 0x7f09d7a83891 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #45 0x7f09dc90087f in nsBaseAppShell::Run() /build/firefox/src/widget/nsBaseAppShell.cpp:165:3
    #46 0x7f09de589263 in XRE_RunAppShell /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:738:12
    #47 0x7f09d7a83891 in RunInternal /build/firefox/src/ipc/chromium/src/base/message_loop.cc:233:3
    #48 0x7f09d7a83891 in RunHandler /build/firefox/src/ipc/chromium/src/base/message_loop.cc:226
    #49 0x7f09d7a83891 in MessageLoop::Run() /build/firefox/src/ipc/chromium/src/base/message_loop.cc:200
    #50 0x7f09de5887db in XRE_InitChildProcess /build/firefox/src/toolkit/xre/nsEmbedFunctions.cpp:575:7
    #51 0x4db762 in content_process_main(int, char**) /build/firefox/src/ipc/app/../contentproc/plugin-container.cpp:236:19
    #52 0x7f09d46c6ec4 in __libc_start_main
    #53 0x41cfb1 in _start

0x6250001f87a8 is located 7848 bytes inside of 8192-byte region [0x6250001f6900,0x6250001f8900)
allocated by thread T0 (Web Content) here:
    #0 0x4b5ea8 in __interceptor_malloc _asan_rtl_
    #1 0x7f09e4775ed8 in PL_ArenaAllocate /build/firefox/src/nsprpub/lib/ds/plarena.c:203:27
    #2 0x7f09dcf4a171 in nsPresArena::Allocate(unsigned int, unsigned long) /build/firefox/src/layout/base/nsPresArena.cpp:99:3
    #3 0x7f09dce99072 in AllocateByObjectID /build/firefox/src/layout/style/../base/nsPresArena.h:93:12
    #4 0x7f09dce99072 in AllocateByObjectID /build/firefox/src/layout/style/../base/nsIPresShell.h:253
    #5 0x7f09dce99072 in operator new /build/firefox/src/layout/style/nsStyleStruct.h:1327
    #6 0x7f09dce99072 in nsRuleNode::ComputePositionData(void*, nsRuleData const*, nsStyleContext*, nsRuleNode*, nsRuleNode::RuleDetail, bool) /build/firefox/src/layout/style/nsRuleNode.cpp:7579
    #7 0x7f09dce7c1ac in nsRuleNode::WalkRuleTree(nsStyleStructID, nsStyleContext*) /build/firefox/src/objdir-ff-asan/layout/style/./nsStyleStructList.h:127:1
    #8 0x7f09dcedccf4 in nsRuleNode::GetStylePosition(nsStyleContext*, bool) /build/firefox/src/objdir-ff-asan/layout/style/./nsStyleStructList.h:127:1
    #9 0x7f09dd2fa3fe in DoGetStylePosition /build/firefox/src/objdir-ff-asan/layout/generic/../../dist/include/nsStyleStructList.h:127:1
    #10 0x7f09dd2fa3fe in StylePosition /build/firefox/src/objdir-ff-asan/layout/generic/../../dist/include/nsStyleStructList.h:127
    #11 0x7f09dd2fa3fe in StylePosition /build/firefox/src/objdir-ff-asan/layout/generic/../../dist/include/nsStyleStructList.h:127
    #12 0x7f09dd2fa3fe in nsHTMLReflowState::Init(nsPresContext*, int, int, nsMargin const*, nsMargin const*) /build/firefox/src/layout/generic/nsHTMLReflowState.cpp:362
    #13 0x7f09dd298a92 in nsBlockReflowContext::ComputeCollapsedBStartMargin(nsHTMLReflowState const&, nsCollapsingMargin*, nsIFrame*, bool*, bool*) /build/firefox/src/layout/generic/nsBlockReflowContext.cpp:162:31
    #14 0x7f09dd291b1d in nsBlockFrame::ReflowBlockFrame(nsBlockReflowState&, nsLineList_iterator, bool*) /build/firefox/src/layout/generic/nsBlockFrame.cpp:3209:7
    #15 0x7f09dd2860b4 in ReflowLine /build/firefox/src/layout/generic/nsBlockFrame.cpp:2710:5
    #16 0x7f09dd2860b4 in nsBlockFrame::ReflowDirtyLines(nsBlockReflowState&) /build/firefox/src/layout/generic/nsBlockFrame.cpp:2248
    #17 0x7f09dd27f53c in nsBlockFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /build/firefox/src/layout/generic/nsBlockFrame.cpp:1160:3
    #18 0x7f09dd2e6e98 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, mozilla::WritingMode const&, mozilla::LogicalPoint const&, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /build/firefox/src/layout/generic/nsContainerFrame.cpp:965:3
    #19 0x7f09dd2cc74d in nsCanvasFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /build/firefox/src/layout/generic/nsCanvasFrame.cpp:676:5
    #20 0x7f09dd375b16 in ReflowChild /build/firefox/src/layout/generic/nsContainerFrame.cpp:965:3
    #21 0x7f09dd375b16 in nsHTMLScrollFrame::ReflowScrolledFrame(ScrollReflowState*, bool, bool, nsHTMLReflowMetrics*, bool) /build/firefox/src/layout/generic/nsGfxScrollFrame.cpp:510
    #22 0x7f09dd377129 in nsHTMLScrollFrame::ReflowContents(ScrollReflowState*, nsHTMLReflowMetrics const&) /build/firefox/src/layout/generic/nsGfxScrollFrame.cpp:621:3
    #23 0x7f09dd3795e9 in nsHTMLScrollFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /build/firefox/src/layout/generic/nsGfxScrollFrame.cpp:856:3
    #24 0x7f09dd2e7294 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, int, int, unsigned int, unsigned int&, nsOverflowContinuationTracker*) /build/firefox/src/layout/generic/nsContainerFrame.cpp:1007:3
    #25 0x7f09dd4d22df in ViewportFrame::Reflow(nsPresContext*, nsHTMLReflowMetrics&, nsHTMLReflowState const&, unsigned int&) /build/firefox/src/layout/generic/nsViewportFrame.cpp:217:7
    #26 0x7f09dd1da4e3 in PresShell::DoReflow(nsIFrame*, bool) /build/firefox/src/layout/base/nsPresShell.cpp:9216:3
    #27 0x7f09dd1ef6bd in PresShell::ProcessReflowCommands(bool) /build/firefox/src/layout/base/nsPresShell.cpp:9376:24
    #28 0x7f09dd1ee844 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /build/firefox/src/layout/base/nsPresShell.cpp:4314:11
    #29 0x7f09dcf53aa1 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:1729:11
    #30 0x7f09dcf5e723 in TickDriver /build/firefox/src/layout/base/nsRefreshDriver.cpp:198:5
    #31 0x7f09dcf5e723 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:189
    #32 0x7f09dcf605e9 in RunRefreshDrivers /build/firefox/src/layout/base/nsRefreshDriver.cpp:440:5
    #33 0x7f09dcf605e9 in TickRefreshDriver /build/firefox/src/layout/base/nsRefreshDriver.cpp:374
    #34 0x7f09dcf605e9 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) /build/firefox/src/layout/base/nsRefreshDriver.cpp:345
    #35 0x7f09dd7ecdb4 in mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) /build/firefox/src/layout/ipc/VsyncChild.cpp:63:5
    #36 0x7f09d7fd0b8b in mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) /build/firefox/src/objdir-ff-asan/ipc/ipdl/./PVsyncChild.cpp:224:20
    #37 0x7f09d7b67eb7 in mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) /build/firefox/src/objdir-ff-asan/ipc/ipdl/./PBackgroundChild.cpp:1082:16
    #38 0x7f09d7af28fb in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /build/firefox/src/ipc/glue/MessageChannel.cpp:1248:14
    #39 0x7f09d7aefdaa in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) /build/firefox/src/ipc/glue/MessageChannel.cpp:1167:9
    #40 0x7f09d7ae2db0 in mozilla::ipc::MessageChannel::OnMaybeDequeueOne() /build/firefox/src/ipc/glue/MessageChannel.cpp:1151:9

Comment 1

[Daniel Holbert [:dholbert]]

A few notes:
 (1) I can only reproduce this if my browser window is pretty short (vertically). (This affects the flow of the table.)

 (2) I can only reproduce this if "layout.css.vertical-text.enabled" is enabled. (It's disabled by default in release builds (RELEASE_BUILD), for now, and enabled by default in nightlies.)


 (3) We're basically inserting after the end of a nsTArray. Specifically, when we hit this part of the stack...
(In reply to Abhishek Arya from comment #0)
>     #2 0x7f09dd574dfd in ShiftData [...]nsTArray-inl.h:268
>     #3 0x7f09dd574dfd in InsertElementAt<nsTableColFrame *> [...]nsTArray.h:1295
>     #4 0x7f09dd574dfd in nsTableFrame::InsertCol(nsTableColFrame&, int) nsTableFrame.cpp:578
... we're calling nsTArray::InsertElementAt with aIndex == 2, in an array with Length() == 0.  Unsurprisingly, this causes bogus behavior. (We grow the array by 1 (to a length of 1), and then we try to shift over all of the elements after the insertion-position, to make room for our inserted elements. But there's a negative number of elements after the insertion position, hence the asan error.)

Comment 2

[Daniel Holbert [:dholbert]]

Also, in a non-ASAN build, I hit a MOZ_CRASH() at jemalloc.c:1610 instead of hitting the asan error. So, no need to bother with ASAN builds when debugging/poking at this.

Comment 3

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (reduced) (may need to be saved locally to repro)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 2a (with nonzero setTimeout)

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8598845 [details]
testcase 2a (with nonzero setTimeout)

Note: for some reason, the Bugzilla-hosted version of testcase 2 doesn't reproduce the issue (even with a slightly longer setTimeout to fight against race conditions).  But it does crash my debug build when I save it locally & load that local version.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

1. With short window, there is no need of "body { height: 30px; }".
2. The "* {...}" rule can be restrict to "body,tr {...}".

The second item indicates that this problem could be seemingly fixed by bug 1159101.

Comment 7

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> 1. With short window, there is no need of "body { height: 30px; }".

(Yup, I added that to the testcase so you don't need to bother making your window short.)

> 2. The "* {...}" rule can be restrict to "body,tr {...}".
> 
> The second item indicates that this problem could be seemingly fixed by bug 1159101.

IIUC, bug 1159101 will make <tr> defer to its <table> for the writing-mode -- so wouldn't we still behave the same if we made the rule "body, table { ...}"? (or just "*" as in the current testcases)

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #6)
> > 1. With short window, there is no need of "body { height: 30px; }".
> 
> (Yup, I added that to the testcase so you don't need to bother making your
> window short.)

Hmm, it's weird that I could not reproduce the crash without shrinking the window, even there was such rule.

> > 2. The "* {...}" rule can be restrict to "body,tr {...}".
> > 
> > The second item indicates that this problem could be seemingly fixed by bug 1159101.
> 
> IIUC, bug 1159101 will make <tr> defer to its <table> for the writing-mode
> -- so wouldn't we still behave the same if we made the rule "body, table {
> ...}"? (or just "*" as in the current testcases)

If I change the rule to "body, table {...}", it doesn't crash for me. ((body || html) && tr) seems to be the exact condition.

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8)
> Hmm, it's weird that I could not reproduce the crash without shrinking the
> window, even there was such rule.

(Gotcha. Yeah, 30px was an arbitrary size I picked which reliably worked for me -- but the exact crashy layout might depend on fonts / system properties, so 30px may not get you into that layout.)

> If I change the rule to "body, table {...}", it doesn't crash for me. ((body
> || html) && tr) seems to be the exact condition.

I'd still expect that the original testcases shouldn't be affected by bug 1159101, since they use a "*" selector. (I can confirm that bug 1159101 *does* seem to prevent the crash on the testcases we've got so far, though, and that confuses me. :))

Comment 10

[Mats Palmgren (inactive)]

Daniel / Xidorn, do either of you have time to fix this one?

Comment 11

[Xidorn Quan [:xidorn] UTC+10]

Is this bug still reproducible? It seems to me that my patch in bug 1159101 apparently fixed this problem, although I have no idea about the real reason.

Comment 12

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Daniel Holbert [:dholbert] from comment #9)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #8)
> > If I change the rule to "body, table {...}", it doesn't crash for me. ((body
> > || html) && tr) seems to be the exact condition.
> 
> I'd still expect that the original testcases shouldn't be affected by bug
> 1159101, since they use a "*" selector. (I can confirm that bug 1159101
> *does* seem to prevent the crash on the testcases we've got so far, though,
> and that confuses me. :))

That confuses me, too. Even if we do not specify "*", without explicit rule, <tr> should also inherit the writing-mode from its ancestors, hence if <body>/<html> has writing-mode set, the <tr> rule should be useless.

I always suspect there is a more dangerous security bug behind this bug.

Comment 13

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #11)
> Is this bug still reproducible? It seems to me that my patch in bug 1159101
> apparently fixed this problem, although I have no idea about the real reason.

That's kind of what I'm asking - do you have time to do that analysis?
Updating your tree to the revision before bug 1159101 landed is probably
the first step, and then debugging it to figure out exactly how and why
the crash occurs.

We need to do that analysis so we know for sure that it's fixed in all
situations.

Comment 14

[Xidorn Quan [:xidorn] UTC+10]

OK, leave a ni? myself for now.

Comment 15

[Xidorn Quan [:xidorn] UTC+10]

Now I know why patch of bug 1159101 hides this problem: we have a rule in html.css says

table { writing-mode: horizontal-tb !important; }

which means, with bug 1159101, all the inner table frames would be forced to have horizontal writing mode, while this problem happens only when the row element has a vertical writing mode. So, with that rule, bug 1159101 does fix this problem.

The direct reason seems to be that, we try to insert the column group frame into a continuation table frame, while we check the index according to the first-in-flow. It is interesting that we create a continuation table frame when it is not in paginated context. I suppose it shouldn't happen, or we should have had a lot of trouble, shouldn't we?

The initial frame split seems to happen in the block frame inside the table cell frame. It reports the incomplete reflow status at nsBlockFrame::PlaceLine() -> PushTruncatedLine() [1]. The condition before the call leads us to the final assertion that, nsBlockReflowState.mBEndEdge can be a value other than NS_UNCONSTRAINEDSIZE only in paginated situation. This assertion is recorded in comment in nsBlockReflowState::nsBlockReflowState [2], the only place mBEndEdge can be set to those values. However, this assertion seems to have been broken in vertical writing mode.

I suspect block frame being split in non-paginated context could potentially cause other problem. Not sure how dangerous it is, though.

[1] http://hg.mozilla.org/mozilla-central/file/62d9b117c688/layout/generic/nsBlockFrame.cpp#l4397
[2] http://hg.mozilla.org/mozilla-central/file/62d9b117c688/layout/generic/nsBlockReflowState.cpp#l121

Probably we should cc :jfkthame and :smontagu, and probably also :dbaron for the potential impact and solution.

Comment 16

[Xidorn Quan [:xidorn] UTC+10]

BTW, it seems I can only reproduce this problem if we update to some commit before bug 1159101 lands. It is not reproducible at least with the provided testcases in the current m-c even with my patches backed out. So probably this issue has also been fixed in some other way in other bugs.

:jfkthame, any idea about how this is fixed in some other place given my analysis in comment 15?

Comment 17

[Jonathan Kew [:jfkthame]]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #15)
> Now I know why patch of bug 1159101 hides this problem: we have a rule in
> html.css says
> 
> table { writing-mode: horizontal-tb !important; }
> 
> which means, with bug 1159101, all the inner table frames would be forced to
> have horizontal writing mode, while this problem happens only when the row
> element has a vertical writing mode. So, with that rule, bug 1159101 does
> fix this problem.

...but that rule is obviously a temporary band-aid that will be removed as soon as we have support for laying out tables in vertical mode.

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #16)
> BTW, it seems I can only reproduce this problem if we update to some commit
> before bug 1159101 lands. It is not reproducible at least with the provided
> testcases in the current m-c even with my patches backed out. So probably
> this issue has also been fixed in some other way in other bugs.
> 
> :jfkthame, any idea about how this is fixed in some other place given my
> analysis in comment 15?

This sounds like it may have been affected (fixed?) by bug 1152941.

Comment 18

[Jonathan Kew [:jfkthame]]

I updated to a revision just before bug 1159101, then applied the patch from bug 1152941 and confirmed that this fixes the assertions and crash in the testcase here in my local debug build.

Comment 19

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Jonathan Kew (:jfkthame) from comment #18)
> I updated to a revision just before bug 1159101, then applied the patch from
> bug 1152941 and confirmed that this fixes the assertions and crash in the
> testcase here in my local debug build.

It seems to me that patch does actually fix this problem. Thanks.

I guess we want to add some guard, though. I'll submit a patch for that soon.

Comment 20

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Applying this patch to the code before bug 1159101 independently fixes this crash. Although I guess we probably should always assert when we use mColGroups from non-first-in-flow table, other usages of that field do not seem to be as harmful as this one. Hence I think it's fine to leave them there.

Comment 21

[Mats Palmgren (inactive)]

Comment on attachment 8605194 [details] [diff] [review]
patch

It's not obvious that GetLastRealColGroup is using FirstInFlow
internally though, and is returning the correct frame.

I think I would prefer something like the following, because it's
less prone to error if we want to modify this code in the future:

if (NS_STYLE_DISPLAY_TABLE_COLUMN_GROUP == display->mDisplay) {
  auto firstInFlow = static_cast<nsTableFrame*>(FirstInFlow());
  if (this != firstInFlow) {
    nsFrameList colgroupFrame(f, f);
    firstInFlow->AppendFrames(aListID, colgroupFrame);
  } else {
    ... the existing code ...
  }

I think we need to fix this in the existing code as well:

        mColGroups.InsertFrame(nullptr, lastColGroup, f);

'nullptr' there means "don't bother reparenting 'f' b/c I know
it's got the right parent", but it's wrong in this case.
So I think we need s/nullptr/this/ there.
(and HomogenousInsertFrames has the same problem)

I think we also need a similar fix in InsertFrames, right?
It looks like it already creates two separate child lists, so
you can probably just divert the HomogenousInsertFrames call
(for the kColGroupList) to the first-in-flow, when needed.

Perhaps we should also assert in nsTableColGroupFrame
that all uses of a nsTableFrame is the first-in-flow?
Adding it in GetTableFrame() might be enough?

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 23

[Mats Palmgren (inactive)]

Comment on attachment 8606826 [details] [diff] [review]
patch

This looks good, although now that I think about InsertFrames
some more I'm hesitating on that change b/c of 'aPrevFrame'.

If it's non-null, perhaps it's a row-group frame that is
actually a child of the continuation?  That will lead to a bogus
frame tree if we use it for an insert on a different parent.

If 'aPrevFrame' is null, then perhaps that's not what we should
use either since trying to insert a col-group frame first in a
continuation is more like an append on the first-in-flow?

Maybe we should instead call firstInFlow->AppendFrames for both
these cases (instead of HomogenousInsertFrames)?

What do you think?

Comment 24

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

I guess you are right. To be honest, I'm not really care about what would happen if it inserts frames to a continuation table frame, since I don't think it should happen at all, and that's why I use MOZ_UNLIKELY.

AFAICS, spliting table frame only happens in paginated context, where there doesn't seem to be dynamic changes.

But anyway, your suggestion makes sense.

Comment 25

[Mats Palmgren (inactive)]

(In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #24)
> AFAICS, spliting table frame only happens in paginated context,

That assumption might be wrong with future changes like overflow:fragments or regions.

> where there doesn't seem to be dynamic changes.

I don't think it's impossible that that might change too.

Comment 26

[Mats Palmgren (inactive)]

Comment on attachment 8607234 [details] [diff] [review]
patch

Looks good to me, thanks.

Comment 27

[Xidorn Quan [:xidorn] UTC+10]

(In reply to Mats Palmgren (:mats) from comment #25)
> (In reply to Xidorn Quan [:xidorn] (UTC+12) from comment #24)
> > AFAICS, spliting table frame only happens in paginated context,
> 
> That assumption might be wrong with future changes like overflow:fragments
> or regions.
> 
> > where there doesn't seem to be dynamic changes.
> 
> I don't think it's impossible that that might change too.

We probably would have a lot of work to do then, I guess. The current table implementation doesn't seem to be robust in that situation. We would want to remove the MOZ_UNLIKELY once that assumption is no longer true.

Comment 28

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8607234 [details] [diff] [review]
patch

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Don't think it is possible, because this bug has been independently fixed by two other bugs.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
It happens only when vertical text is enabled, which haven't yet been switched on by default on beta or release. So no older branches are affected.

If not all supported branches, which bug introduced the flaw?
Whichever enables vertical text. For aurora and nightly, it is bug 1099032.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
N/A.

How likely is this patch to cause regressions; how much testing does it need?
The code added in this patch should simply be unreachable in current situation (or we should have had more security issues), so I don't think it would cause any regressions.

Comment 29

[Al Billings [:abillings - ex-MoCo]]

sec-approval+ for trunk.
Can an Aurora patch please be prepared and nominated as well?

Comment 30

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8607234 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: bug 1099032
[User impact if declined]: this patch adds protection to potential exploit
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: the added code should be unreachable, or we should have had some crash/exploit related to it
[String/UUID change made/needed]: n/a

Comment 31

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dac2fb6a6419

Hmm, it seems pulsebot cannot post in security bugs.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dac2fb6a6419

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6e6d08ff1329

Comment 34

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1430162118/

Revived ASan crashes using the following test cases:
* https://bug1159127.bmoattachments.org/attachment.cgi?id=8598431&t=dcKESheXLS
* https://bug1159127.bmoattachments.org/attachment.cgi?id=8598839&t=veAt6LFG1F

0x625000b057f8 is located 5880 bytes inside of 8192-byte region [0x625000b04100,0x625000b06100)
allocated by thread T0 here:
    #0 0x472111 in __interceptor_malloc _asan_rtl_
    #1 0x7f3694a1de86 in PL_ArenaAllocate plarena.c:203
    #2 0x7f368caa2395 in Allocate nsPresArena.cpp:99
    #3 0x7f368c9f6817 in AllocateByObjectID nsPresArena.h:93
    #4 0x7f368c9da3f0 in WalkRuleTree nsStyleStructList.h:127
    #5 0x7f368ca37274 in GetStylePosition nsStyleStructList.h:127

etc...

Went through verification using the following build:
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-central-linux64-asan/1437735281/
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-aurora-linux64-asan/1437744656/
* http://inbound-archive.pub.build.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-beta-linux64-asan/1437689448/

Test Cases Used:

- opened the two test cases mentioned above in both e10s and non-e10s modes
- ensured "layout.css.vertical-text.enabled;true", especially on fx40