1415185 - Assertion failure: IsInUncomposedDoc() || IsInShadowTree() (This will end badly!), at /builds/worker/workspace/build/src/dom/base/nsIContentInlines.h:32

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jason Kratzer [:jkratzer]]

Attachment: trigger.html

Testcase found while fuzzing mozilla-central rev 923836aebbc3.

==16765==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f0b34291082 bp 0x7ffcfe8faaf0 sp 0x7ffcfe8faad0 T0)
==16765==The signal is caused by a WRITE memory access.
==16765==Hint: address points to the zero page.
    #0 0x7f0b34291081 in nsIContent::SetPrimaryFrame(nsIFrame*) /builds/worker/workspace/build/src/dom/base/nsIContentInlines.h:32:3
    #1 0x7f0b343eb6b2 in nsFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:822:15
    #2 0x7f0b345174fb in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsLineBox.cpp:402:14
    #3 0x7f0b343b308a in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:331:3
    #4 0x7f0b345174fb in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsLineBox.cpp:402:14
    #5 0x7f0b343b308a in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:331:3
    #6 0x7f0b34371816 in mozilla::DetailsFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/DetailsFrame.cpp:88:17
    #7 0x7f0b343b045a in nsFrameList::DestroyFramesFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsFrameList.cpp:59:12
    #8 0x7f0b343b34fb in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:226:11
    #9 0x7f0b342d418f in nsIFrame::Destroy() /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:674:5
    #10 0x7f0b343ddd0b in nsBlockFrame::DoRemoveFrame(nsIFrame*, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:5979:20
    #11 0x7f0b343de302 in nsBlockFrame::DoRemoveFrame(nsIFrame*, unsigned int) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:6081:3
    #12 0x7f0b343dce43 in nsBlockFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:5292:5
    #13 0x7f0b342aa7be in nsFrameManager::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsFrameManager.cpp:535:18
    #14 0x7f0b342a91fa in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:8827:5
    #15 0x7f0b3429ed1a in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10027:7
    #16 0x7f0b342a3820 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, nsCSSFrameConstructor::InsertionKind, TreeMatchContext*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:8210:5
    #17 0x7f0b342a61a3 in nsCSSFrameConstructor::IssueSingleInsertNofications(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7468:5
    #18 0x7f0b342a661e in nsCSSFrameConstructor::GetRangeInsertionPoint(nsIContent*, nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7515:7
    #19 0x7f0b342a4f29 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsIContent*, nsCSSFrameConstructor::InsertionKind, TreeMatchContext*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7669:5
    #20 0x7f0b341fb07f in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:1404:27
    #21 0x7f0b3425064a in mozilla::ServoRestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/ServoRestyleManager.cpp:1159:9
    #22 0x7f0b3421e35b in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4196:41
    #23 0x7f0b341b1e73 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1882:18
    #24 0x7f0b341bb2fe in mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:306:7
    #25 0x7f0b341bb0d4 in mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:327:5
    #26 0x7f0b341be5b5 in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:769:5
    #27 0x7f0b341bd656 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:682:35
    #28 0x7f0b341b97d7 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:528:20
    #29 0x7f0b2e500e8f in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1037:14
    #30 0x7f0b2e521aa0 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:513:10
    #31 0x7f0b2f0bdfe5 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #32 0x7f0b2f010137 in MessageLoop::RunInternal() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10
    #33 0x7f0b2f00ffc9 in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299:3
    #34 0x7f0b33ca3eda in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27
    #35 0x7f0b36ec3e21 in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:288:30
    #36 0x7f0b370389a8 in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4675:22
    #37 0x7f0b3703a5ca in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4837:8
    #38 0x7f0b3703b4f9 in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4932:21
    #39 0x4ed558 in do_main(int, char**, char**) /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:231:22
    #40 0x4ece7b in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:304:16
    #41 0x7f0b4d79182f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Reproduces with or without Stylo enabled.

INFO: Last good revision: 4f5a502acbe831a1d17d5af06efd63c0b3081a8e
INFO: First bad revision: cd9f14f9ea6d4bcd175874352c402613503a86d8
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4f5a502acbe831a1d17d5af06efd63c0b3081a8e&tochange=cd9f14f9ea6d4bcd175874352c402613503a86d8

Comment 2

[Mats Palmgren (inactive)]

I'll take a look...

Comment 3

[Mats Palmgren (inactive)]

Attachment: frame tree

The problem is nsBlockFrame::DoRemoveFrame destroys continuations
in first-to-last order.
In this case we have "Block(summary)(-1)@7efdeae28238" which happens
to be a NAC frame for the <details> element:
http://searchfox.org/mozilla-central/rev/ed212c79cfe86357e9a5740082b9364e7f6e526f/layout/generic/DetailsFrame.cpp#87
so we DestroyAnonymousContent in Destroy, which also UnbindFromTree
the content *including descendants* so when we bump into the primary
frame Text(0)"Details" (in its next-in-flow 7efdeae29a60) and call
SetPrimaryFrame(null) on its content, that text content is already
unbound.

Comment 4

[Mats Palmgren (inactive)]

Attachment: fix

Comment 5

[Mats Palmgren (inactive)]

Attachment: crashtest

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8926939 [details] [diff] [review]
fix

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

Looks good... I thought about it a bit and even though RemoveFrame calls into RemoveOutOfFlow and such, who calls Destroy() with a new PostDestroyData, I don't think they suffer about the same issue.

It'd be nice to thread aPostDestroyData all the way from RemoveFrame, which is the entry point for frame removals, though that's a much different change. Also, that'd change aDestroyRoot for the only frame that uses it (nsPlaceholderFrame), but I think it'd be fine... Maybe file a followup just to track a refactor on those lines?

::: layout/generic/nsIFrame.h
@@ +663,5 @@
>                      nsContainerFrame* aParent,
>                      nsIFrame*         aPrevInFlow) = 0;
>  
> +  using PostDestroyData = mozilla::layout::PostFrameDestroyData;
> +  struct MOZ_RAII AutoPostDestroyData {

nit: I believe braces should be in the next line.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69d0e8d0a165
Make nsBlockFrame::DoRemoveFrame handle anon/generated content from all the continuations it destroys.  r=emilio
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b177ba1e9d9
Crashtest.

Comment 8

[Mats Palmgren (inactive)]

I think RemoveFrame is only the entry for frame removals when we
remove *all* frames for a node.  We don't use it when we remove
continuations in layout for example.

Threading aPostDestroyData to RemoveFrame might be unnecessary.
Instead, we can probably fix this by enforcing that we always
delete continuations in reverse order (we do that already in
nsContainerFrame::DeleteNextInFlowChild).  We can probably
revert this patch if we change nsBlockFrame::DoRemoveFrame to
do the same.

(That invariant would be good for other reasons too: we've had
a bug in the past were a resource was owned by the first-in-flow
and when accessed during destruction it lead to UAF.)

Comment 9

[Mats Palmgren (inactive)]

(I filed bug 1416326 as a follow-up.)

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/69d0e8d0a165
https://hg.mozilla.org/mozilla-central/rev/5b177ba1e9d9

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1415185: Fix IsInSameAnonymousTree in Shadow DOM.

Review commit: https://reviewboard.mozilla.org/r/206376/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/206376/

Comment 12

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8935475 [details]
Bug 1415185: Fix IsInSameAnonymousTree in Shadow DOM.

Err, wrong bug...