1472020 - Assertion failure: !mMutationGuard.Mutated(0), at src/dom/base/ChildIterator.h:220

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P3)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20180628100518
SourceStamp=b429b9fb68f1a954c4a9f8dba8e845cf7f569a56

Assertion failure: !mMutationGuard.Mutated(0), at src/dom/base/ChildIterator.h:220

#0 mozilla::dom::AllChildrenIterator::~AllChildrenIterator() src/dom/base/ChildIterator.h:220:28
#1 nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) src/layout/base/nsCSSFrameConstructor.cpp:7990:3
#2 nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) src/layout/base/nsCSSFrameConstructor.cpp:9169:5
#3 mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) src/layout/base/RestyleManager.cpp:1512:25
#4 mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) src/layout/base/RestyleManager.cpp:2997:9
#5 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4287:41
#6 nsRefreshDriver::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1897:18
#7 mozilla::RefreshDriverTimer::TickRefreshDrivers(long, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301:7
#8 mozilla::RefreshDriverTimer::Tick(long, mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:319:5
#9 mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:760:5
#10 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:673:35
#11 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run() src/layout/base/nsRefreshDriver.cpp:519:20
#12 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1051:14
#13 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:519:10
#14 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#15 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#16 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#17 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
#18 nsAppStartup::Run() src/toolkit/components/startup/nsAppStartup.cpp:290:30
#19 XREMain::XRE_mainRun() src/toolkit/xre/nsAppRunner.cpp:4746:22
#20 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4891:8
#21 XRE_main(int, char**, mozilla::BootstrapConfig const&) src/toolkit/xre/nsAppRunner.cpp:4983:21
#22 do_main(int, char**, char**) src/browser/app/nsBrowserApp.cpp:233:22
#23 main src/browser/app/nsBrowserApp.cpp:311:16
#24 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#25 _start (firefox+0x4236e4)

Comment 1

[Tyson Smith [:tsmith]]

Attachment: prefs.js

required to repro

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Heh, I knew fuzzing accessiblecaret was going to yield some interesting stuff. Thanks!

Comment 3

[Emilio Cobos Álvarez (:emilio)]

This one is fun, I'm glad I asked the Fuzzing Team to fuzz with layout.accessiblecaret.enabled... :)

So this is basically complaining because we're doing a bunch of DOM mutations from the nsCanvasFrame code, cloning nodes and what not:

  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/layout/generic/nsCanvasFrame.cpp#160

We also do a bunch of notifying insertions, etc. while creating the canvas frame from:

  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/layout/generic/nsCanvasFrame.cpp#127

Init() creates an AccessibleCaret, which calls InsertAnonymousContent:

  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/layout/base/AccessibleCaret.cpp#220

I'm not sure how we can organize this code to avoid this but the current setup is not quite ideal... :(

Comment 4

[Boris Zbarsky [:bzbarsky]]

Can we move that work to a scriptrunner?  The destroy work presumably not, but the InsertAnonymousContent bits.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #4)
> Can we move that work to a scriptrunner?  The destroy work presumably not,
> but the InsertAnonymousContent bits.

Sure, we could also not notify and let nsCanvasFrame do the right thing in the loop we do before calling Init().

I don't really understand:

  https://searchfox.org/mozilla-central/rev/d2966246905102b36ef5221b0e3cbccf7ea15a86/layout/generic/nsCanvasFrame.cpp#160

Though... We're just cloning subtrees just because, and replacing the old pointer with a clone. I don't understand why that's needed. DestroyAnonymousContent will unbind all those, and CreateAnonymousContent will bind them again to the new tree...

Comment 6

[Boris Zbarsky [:bzbarsky]]

> and replacing the old pointer with a clone

Are we?  We stash the CloneNode result inside doc->GetAnonymousContents(), no?  Then DestroyAnonymousContent unbinds and destroys the originals and CreateAnonymousContent grabs the things from doc->GetAnonymousContents().

Though that stuff looks like it only supports one canvasframe per document; not sure whether that makes sense...

Comment 7

[Emilio Cobos Álvarez (:emilio)]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #6)
> > and replacing the old pointer with a clone
> 
> Are we?  We stash the CloneNode result inside doc->GetAnonymousContents(),
> no?  Then DestroyAnonymousContent unbinds and destroys the originals and
> CreateAnonymousContent grabs the things from doc->GetAnonymousContents().

Right, the point is that DestroyAnonymousContent only unbinds the subtree, which makes sense, but there's no reason we couldn't reuse the same nodes, I'd think...

> Though that stuff looks like it only supports one canvasframe per document;
> not sure whether that makes sense...

Right...

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1472020: Make AccessibleCaret a bit saner. r=bz

* Avoid processing anon content in nsCanvasFrame, then getting more anon
   content via AccessibleCaretEventHub::Init. Instead call Init before creating
   the custom content container. We could also throw a script runner at it I
   guess, but this prevents the reentrancy issue.

 * Avoid cloning nodes on shutdown, just use the same node (already cloned in
   InsertAnonymousContent) node instead.

   The RemoveChild in GetAnonymousContent to handle the reframes instead of
   cloning around is a bit hacky, but I don't think it's really worth extending
   PostDestroyData for this special case.

 * Reuse the ContentInfo stuff, instead of notifying while appending and that
   kind of stuff.

Comment 9

[Phabricator Automation]

Comment on attachment 8988850 [details]
Bug 1472020: Make AccessibleCaret a bit saner. r=bz

Ting-Yu Lin [:TYLin] (UTC-7) has approved the revision.

https://phabricator.services.mozilla.com/D1889

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8988850 [details]
Bug 1472020: Make AccessibleCaret a bit saner. r=bz

Boris Zbarsky [:bzbarsky, bz on IRC] (vacation Aug 16-27) has approved the revision.

Comment 11

[Johann Hofmann [:johannh]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9a4d66f5f3f8ff76b64824b8fbb66c310a7443
Bug 1472020 - Make AccessibleCaret a bit saner. r=bz,TYLin

Comment 12

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/5c9a4d66f5f3