1644819 - crash at null in [@ nsFlexContainerFrame::FlexItemIterator::FlexItemIterator]

Status: RESOLVED FIXED

(Core :: Layout: Flexbox, defect)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c 20200609-7f7b98339065

==85002==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7ffa98b97870 bp 0x7ffd22139c10 sp 0x7ffd22139be0 T0)
==85002==The signal is caused by a READ memory access.
==85002==Hint: address points to the zero page.
    #0 0x7ffa98b9786f in Length /builds/worker/workspace/obj-build/dist/include/nsTArray.h:402:37
    #1 0x7ffa98b9786f in end /builds/worker/workspace/obj-build/dist/include/nsTArray.h:1212:34
    #2 0x7ffa98b9786f in nsFlexContainerFrame::FlexItemIterator::FlexItemIterator(nsTArray<nsFlexContainerFrame::FlexLine> const&) src/layout/generic/nsFlexContainerFrame.cpp:1092:29
    #3 0x7ffa98b9744a in nsFlexContainerFrame::GenerateFlexLines(nsFlexContainerFrame::SharedFlexData const&, nsTArray<nsFlexContainerFrame::FlexLine>&) src/layout/generic/nsFlexContainerFrame.cpp:3964:20
    #4 0x7ffa98b9a753 in nsFlexContainerFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) src/layout/generic/nsFlexContainerFrame.cpp:4380:5
    #5 0x7ffa9892d4e9 in mozilla::PresShell::DoReflow(nsIFrame*, bool, mozilla::OverflowChangedTracker*) src/layout/base/PresShell.cpp:9576:11
    #6 0x7ffa9893fab7 in mozilla::PresShell::ProcessReflowCommands(bool) src/layout/base/PresShell.cpp:9749:24
    #7 0x7ffa9893e52d in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4230:11
    #8 0x7ffa93e1e8fd in FlushPendingNotifications /builds/worker/workspace/obj-build/dist/include/mozilla/PresShell.h:1441:5
    #9 0x7ffa93e1e8fd in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) src/dom/base/Document.cpp:10022:16
    #10 0x7ffa93f875b0 in mozilla::dom::Selection::ScrollIntoView(short, mozilla::ScrollAxis, mozilla::ScrollAxis, int) src/dom/base/Selection.cpp:2968:31
    #11 0x7ffa93f8f6eb in mozilla::dom::Selection::ScrollSelectionIntoViewEvent::Run() src/dom/base/Selection.cpp:2901:14
    #12 0x7ffa988cd4ff in nsRefreshDriver::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1943:13
    #13 0x7ffa988dbba6 in TickDriver src/layout/base/nsRefreshDriver.cpp:373:13
    #14 0x7ffa988dbba6 in mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:350:7
    #15 0x7ffa988db7a5 in mozilla::RefreshDriverTimer::Tick(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:367:5
    #16 0x7ffa988eab12 in RunRefreshDrivers src/layout/base/nsRefreshDriver.cpp:819:5
    #17 0x7ffa988eab12 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::layers::BaseTransactionId<mozilla::VsyncIdType>, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:737:16
    #18 0x7ffa988e9f0b in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyParentProcessVsync() src/layout/base/nsRefreshDriver.cpp:639:7
    #19 0x7ffa988d8b92 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:538:20
    #20 0x7ffa8fe4c9c8 in nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1236:14
    #21 0x7ffa8fe575ec in NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:501:10
    #22 0x7ffa911d595f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:87:21
    #23 0x7ffa910b3b57 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #24 0x7ffa910b3b57 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #25 0x7ffa910b3b57 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #26 0x7ffa98433e98 in nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:137:27
    #27 0x7ffa9bfe1e86 in XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:913:20
    #28 0x7ffa910b3b57 in RunInternal src/ipc/chromium/src/base/message_loop.cc:315:10
    #29 0x7ffa910b3b57 in RunHandler src/ipc/chromium/src/base/message_loop.cc:308:3
    #30 0x7ffa910b3b57 in MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:290:3
    #31 0x7ffa9bfe146f in XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:744:34
    #32 0x563706ba4793 in content_process_main src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
    #33 0x563706ba4793 in main src/browser/app/nsBrowserApp.cpp:303:18

Comment 1

[Tyson Smith [:tsmith]]

A Pernosco session is available here: https://pernos.co/debug/cKoeIh2bQlnj3pQupSD0jw/index.html

Comment 2

[Jonathan Kew [:jfkthame]]

FWIW, the testcase didn't crash for me (in Nightly on macOS); maybe it's somewhat platform-dependent?

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

I can reproduce this via the following steps in my local debug build:

1. Set layout.accessiblecaret.enabled=true.
2. Load the test case in layout debugger.
3. Click the text "aaa".
4. Hit "Reload" button.

Anyway, from the Pernosco session, I see there's an assertion before the crash like

###!!! ASSERTION: reflow roots should never split: '!target->GetNextInFlow() && !target->GetPrevInFlow()', file /builds/worker/checkouts/gecko/layout/base/PresShell.cpp

The final crash happens when a flex container's continuation find a null SharedFlexData::Prop().

Flex container can be a dynamic reflow root, so if a flex container's first-in-flow being a reflow root, becoming FullyComplete after reflow, the SharedFlexData will be deleted.

Maybe we should prevent any container frames being a dynamic reflow root if they are split? I think it's the container's parent's responsibility to delete the container's next-in-flow.

Note: grid container doesn't have the issue because it deletes SharedGridData only when there's no next-in-flow.

Comment 4

[Daniel Holbert [:dholbert]]

(In reply to Ting-Yu Lin [:TYLin] (UTC-7) from comment #3)

Maybe we should prevent any container frames being a dynamic reflow root if they are split? I think it's the container's parent's responsibility to delete the container's next-in-flow.

That makes sense to me.

How about this:
(1) nsCSSFrameConstructor::CreateContinuingFrame() should clear the dynamic-reflow-root bit from aFrame (under the assumption that it might be set, and now needs to be unset now that we're creating a continuation).
(2) nsIFrame::CanBeDynamicReflowRoot() should check if we are in a continuation chain (i.e. if we have a prev-in-flow or a next-in-flow), and if so, we should return false.

Item (1) ensures that we clean up after ourselves, if we thought something could be a dynamic reflow root and then we fragment it.

Item (2) ensures that we don't go back and mark that same frame as a dynamic reflow root again in a subsequent reflow (and ensures that all its downstream continuations don't get marked as dynamic reflow roots).

Does that make sense? If so: TYLin, do you have cycles to spin up a patch for this? (I could, too, if that's better).

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Both (1) and (2) make sense to me. I'm already experimenting (2) a bit, so I can take care of this.

I have pushed the crashtest to try. This bug can be reproduced on Linux 64 and Android with AccessibleCaret enabled. This is good, and we don't need to find another test to simulate the manual steps in comment 3.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f9ddd3195514bfb77baef66d17d7682025e3c6ee

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1644819 Part 1 - Assert that SharedFlexData is valid in flex container's continuations.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1644819 Part 2 - Prevent a split frame from being a dynamic reflow root.

Depends on D79584

Comment 8

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Bug 1644819 Part 3 - Add a crashtest.

Depends on D79585

Comment 9

[Daniel Holbert [:dholbert]]

This looks great - thanks!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4d39bd6871b8
Part 1 - Assert that SharedFlexData is valid in flex container's continuations. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/3b87f74ba068
Part 2 - Prevent a split frame from being a dynamic reflow root. r=dholbert
https://hg.mozilla.org/integration/autoland/rev/f4f558d1cc36
Part 3 - Add a crashtest. r=dholbert

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Hmm, should the condition be on a frame that is splittable rather than split? In particular, what happens if you have a big block in a multicol which happens to be a valid reflow root (and not split), but it changes in a way that should become split? Isn't the parent or fragmentainer's responsibility to split it?

Comment 12

[Daniel Holbert [:dholbert]]

Hmm, I think you're right. (I think the condition we care about here is "the frame is splittable and is at risk of being split due to having a fragmentainer ancestor").

Here's a concrete scenario where we'd have trouble with our current approach, I think:

• fixed-height multicol element
• ...with a child that is eligible to be a dynamic reflow root (via e.g. having styles like in https://searchfox.org/mozilla-central/rev/20ad814e4cc3d5eb3e33ccec6ed2b17255205aca/layout/generic/test/test_dynamic_reflow_root_disallowal.html#46-51 )
• The dynamic-reflow-root element then receives a dynamic tweak to give it a really tall child (say, a really tall display:block with a paragraph of text)
• This new tall element overflows the multicol and should fragment when it's inserted.
• ...but if we initiate its reflow from the dynamic reflow root, then we may not fragment it (to make the needed overflow containers) correctly.

Here's a demo/testcase:
https://jsfiddle.net/dholbert/e1cp9a63/

STR:

1. Load testcase, and click button.

EXPECTED RESULTS: The added teal box should fragment between the columns.
ACTUAL RESULTS: It doesn't fragment. It just overflows.

If you remove display:flow-root or will-change:transform, then you get EXPECTED RESULTS (because we don't make it a dynamic reflow root). But neither of those changes make sense as things that should the behavior here.

Comment 13

[Daniel Holbert [:dholbert]]

I spun off Bug 1645642 to address comment 11 - comment 12 here.

Comment 14

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/4d39bd6871b8
https://hg.mozilla.org/mozilla-central/rev/3b87f74ba068
https://hg.mozilla.org/mozilla-central/rev/f4f558d1cc36

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:TYLin, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment 16

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

This fixed an artificial testcase, which is very rare on real sites. I feel it's ok for my patches to ride the train. Also, the proper fix should be in Bug 1645642 .