1507674 - Hit MOZ_CRASH(... has still dirty bit true or animation-only dirty bit false) at servo/ports/geckolib/glue.rs:4978

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Hit MOZ_CRASH(<menuitem> (0x60d000054290) has still dirty bit true or animation-only dirty bit false) at servo/ports/geckolib/glue.rs:4978

#0 MOZ_CrashOOL(char const*, int, char const*) src/obj-firefox/dist/include/mozilla/Assertions.h:311:3
#1 GeckoCrashOOL src/toolkit/xre/nsAppRunner.cpp:5350:3
#2 gkrust_shared::panic_hook::hc3802bf49e7a75b8 src/toolkit/library/rust/shared/lib.rs:234:8
#3 core::ops::function::Fn::call::h224a14e75e4d5c68 src/libcore/ops/function.rs:78:4
#4 std::panicking::rust_panic_with_hook::h0e12cb2fc86d00fa /rustc/da5f414c2c0bfe5198934493f04c676e2b23ff2e/src/libstd/panicking.rs:481:16
#5 std::panicking::continue_panic_fmt::h141671b29fe0e27d /rustc/da5f414c2c0bfe5198934493f04c676e2b23ff2e/src/libstd/panicking.rs:391:4
#6 std::panicking::begin_panic_fmt::h598547fc766c278a /rustc/da5f414c2c0bfe5198934493f04c676e2b23ff2e/src/libstd/panicking.rs:346:4
#7 geckoservo::glue::Servo_AssertTreeIsClean::assert_subtree_is_clean::heab13ca3b948b446 src/servo/ports/geckolib/glue.rs:4978:8
#8 geckoservo::glue::Servo_AssertTreeIsClean::assert_subtree_is_clean::heab13ca3b948b446 src/servo/ports/geckolib/glue.rs:4983:16
#9 geckoservo::glue::Servo_AssertTreeIsClean::assert_subtree_is_clean::heab13ca3b948b446 src/servo/ports/geckolib/glue.rs:4983:16
#10 Servo_AssertTreeIsClean src/servo/ports/geckolib/glue.rs:4988:4
#11 mozilla::ServoStyleSet::AssertTreeIsClean() src/layout/style/ServoStyleSet.cpp:1155:5
#12 mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) src/layout/base/RestyleManager.cpp:3122:13
#13 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4359:39
#14 nsRefreshDriver::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:1907:18
#15 mozilla::RefreshDriverTimer::TickRefreshDrivers(mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) src/layout/base/nsRefreshDriver.cpp:301:7
#16 mozilla::RefreshDriverTimer::Tick(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:319:5
#17 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:676:16
#18 mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) src/layout/base/nsRefreshDriver.cpp:573:9
#19 mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) src/layout/ipc/VsyncChild.cpp:76:16
#20 mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PVsyncChild.cpp:167:20
#21 mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) src/obj-firefox/ipc/ipdl/PBackgroundChild.cpp:2280:28
#22 mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) src/ipc/glue/MessageChannel.cpp:2244:25
#23 mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) src/ipc/glue/MessageChannel.cpp:2171:17
#24 mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) src/ipc/glue/MessageChannel.cpp:2008:5
#25 mozilla::ipc::MessageChannel::MessageTask::Run() src/ipc/glue/MessageChannel.cpp:2041:15
#26 nsThread::ProcessNextEvent(bool, bool*) src/xpcom/threads/nsThread.cpp:1244:14
#27 NS_ProcessNextEvent(nsIThread*, bool) src/xpcom/threads/nsThreadUtils.cpp:530:10
#28 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:97:21
#29 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#30 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#31 nsBaseAppShell::Run() src/widget/nsBaseAppShell.cpp:158:27
#32 XRE_RunAppShell() src/toolkit/xre/nsEmbedFunctions.cpp:961:22
#33 mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) src/ipc/glue/MessagePump.cpp:269:9
#34 MessageLoop::RunInternal() src/ipc/chromium/src/base/message_loop.cc:325:10
#35 MessageLoop::Run() src/ipc/chromium/src/base/message_loop.cc:298:3
#36 XRE_InitChildProcess(int, char**, XREChildData const*) src/toolkit/xre/nsEmbedFunctions.cpp:787:34
#37 content_process_main(mozilla::Bootstrap*, int, char**) src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
#38 main src/browser/app/nsBrowserApp.cpp:287:18
#39 __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
#40 _start (/home/user/workspace/browsers/m-c-20181115161403-asan-debug/firefox+0x349f4)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

So the issue is that SVG filter code posts a restyle at a time where we're already done restyling, from here:

#0  0x00007fe7650bc71c in style::gecko::wrapper::GeckoElement::note_explicit_hints (self=0x7fff3a065a60, restyle_hint=..., change_hint=...) at servo/components/style/gecko/wrapper.rs:840
#1  0x00007fe7648f470e in Servo_NoteExplicitHints (element=0x7fe735655310, restyle_hint=..., change_hint=...) at servo/ports/geckolib/glue.rs:4528
#2  0x00007fe75fbd50cf in mozilla::RestyleManager::PostRestyleEvent(mozilla::dom::Element*, nsRestyleHint, nsChangeHint)
    (this=0x7fe736266500, aElement=0x7fe735655310, aRestyleHint=0, aMinChangeHint=2049) at /home/emilio/src/moz/gecko-static-analysis/layout/base/RestyleManager.cpp:2271
#3  0x00007fe75ff22dd1 in mozilla::SVGFilterObserverListForCSSProp::OnRenderingChange() (this=0x7fe749fe5070)
    at /home/emilio/src/moz/gecko-static-analysis/layout/svg/SVGObserverUtils.cpp:737
#4  0x00007fe75ff40cf6 in mozilla::SVGFilterObserverList::Invalidate() (this=0x7fe749fe5070) at /home/emilio/src/moz/gecko-static-analysis/layout/svg/SVGObserverUtils.cpp:594
#5  0x00007fe75ff222ab in mozilla::SVGFilterObserver::OnRenderingChange() (this=0x7fe735615d00) at /home/emilio/src/moz/gecko-static-analysis/layout/svg/SVGObserverUtils.cpp:627
#6  0x00007fe75ff213cd in mozilla::SVGRenderingObserver::OnNonDOMMutationRenderingChange() (this=0x7fe735615d00)
    at /home/emilio/src/moz/gecko-static-analysis/layout/svg/SVGObserverUtils.cpp:165
#7  0x00007fe75ff23951 in mozilla::SVGRenderingObserverSet::InvalidateAll() (this=0x7fe749fe53d0) at /home/emilio/src/moz/gecko-static-analysis/layout/svg/SVGObserverUtils.cpp:990
#8  0x00007fe75ff25ad7 in mozilla::SVGObserverUtils::InvalidateDirectRenderingObservers(mozilla::dom::Element*, unsigned int) (aElement=0x7fe73c61c740, aFlags=0)
    at /home/emilio/src/moz/gecko-static-analysis/layout/svg/SVGObserverUtils.cpp:1649
#9  0x00007fe75ff1d067 in mozilla::SVGObserverUtils::InvalidateDirectRenderingObservers(nsIFrame*, unsigned int) (aFrame=0x7fe73560bdd0, aFlags=0)
    at /home/emilio/src/moz/gecko-static-analysis/layout/svg/SVGObserverUtils.cpp:1661
#10 0x00007fe75fd65b04 in nsIFrame::FinishAndStoreOverflow(nsOverflowAreas&, nsSize, nsSize*, nsStyleDisplay const*)
    (this=0x7fe73560bdd0, aOverflowAreas=..., aNewSize=..., aOldSize=0x0, aStyleDisplay=0x0) at /home/emilio/src/moz/gecko-static-analysis/layout/generic/nsFrame.cpp:9697
#11 0x00007fe75fd64efd in nsIFrame::UpdateOverflow() (this=0x7fe73560bdd0) at /home/emilio/src/moz/gecko-static-analysis/layout/generic/nsFrame.cpp:7607
#12 0x00007fe75fc168dc in mozilla::OverflowChangedTracker::Flush() (this=0x7fe736266530)
    at /home/emilio/src/moz/gecko-static-analysis/obj-x86_64-pc-linux-gnu/dist/include/mozilla/OverflowChangedTracker.h:111
#13 0x00007fe75fc0d199 in mozilla::RestyleManager::FlushOverflowChangedTracker() (this=0x7fe736266500)
    at /home/emilio/src/moz/gecko-static-analysis/obj-x86_64-pc-linux-gnu/dist/include/mozilla/RestyleManager.h:224
#14 0x00007fe75fbf96eb in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) (this=0x7fe736266500, aFlags=mozilla::ServoTraversalFlags::Empty)
    at /home/emilio/src/moz/gecko-static-analysis/layout/base/RestyleManager.cpp:3120
#15 0x00007fe75fbda14d in mozilla::RestyleManager::ProcessPendingRestyles() (this=0x7fe736266500) at /home/emilio/src/moz/gecko-static-analysis/layout/base/RestyleManager.cpp:3163
#16 0x00007fe75fbd9ad3 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7fe7362ed000, aFlush=...)
    at /home/emilio/src/moz/gecko-static-analysis/layout/base/PresShell.cpp:4359
#17 0x00007fe75ccdb180 in nsIPresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7fe7362ed000, aType=...)
    at /home/emilio/src/moz/gecko-static-analysis/layout/base/nsIPresShell.h:591
#18 0x00007fe75fba316f in nsRefreshDriver::Tick(mozilla::TimeStamp) (this=0x7fe73c11a800, aNowTime=...) at /home/emilio/src/moz/gecko-static-analysis/layout/base/nsRefreshDriver.cpp:1907
#19 0x00007fe75fbabf35 in mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, mozilla::TimeStamp) (driver=0x7fe73c11a800, now=...)

That looks slightly borked... But we already need to deal with all this reentrant change hints madness due to SVG :(

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1507674 - Flush the overflow changed tracker earlier. r=jwatt

So we do it while we're still handling re-entrant changes for SVG, since SVG can
post change hints from UpdateOverflow().

That looks really bogus over-all, looks like it could post change hints during
layout, but we already have a mechanism to deal with at least the
during-restyle-hint processing hints at least, so use it.

Comment 3

[Jonathan Watt [:jwatt]]

FWIW this is an instance of the issues bug 794442 was filed for. After my recent overhaul of the SVGObserverUtils code it should be a lot easier (not necessarily easy) to fix these issues.

Comment 4

[Eliza Balazs [:ebalazs_]]

Backed out changeset 0564666f34b2 (Bug 1507674) for crashtests in reftest/tests/dom/base/crashtests/1397795.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&selectedJob=218316547&revision=0564666f34b28960ab4e1a4e2ba6b62c74e542ca

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=218316547&repo=autoland&lineNumber=6986

Backout push: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Cusercancel%2Crunnable&revision=997e573df3677442ab25da9f7f6508bcae8b070e

Comment 5

[Tyson Smith [:tsmith]]

The fuzzers are still hitting this fairly often. Since that is the case and we have a patch can we push to get it landed?

Comment 6

[Tyson Smith [:tsmith]]

An SVG fuzzer hit this a few thousand times over the weekend. Marking as fuzzblocker.

Comment 7

[Emilio Cobos Álvarez (:emilio)]

I'll rebase the patch, and see where it stands, I think I fixed some similar issues in the recent past.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

It is still not fixed, but I wrote some code that should avoid the problem: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3dc8a5921c1d8fe70d2ef3f80c9da444d3aeee37

Comment 9

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1507674 - Refactor svg observer setup to not do silly things when observing an ancestor. r=jwatt,longsonr

Right now we post updates and it "works" because we prevent the UpdateOverflow
call if we're during reflow.

If this happens during styling however this is not sound (and it is not sound
in general and has caused badness in the past, as noted by the other
workarounds).

Make it sound by preventing to observe ancestors, and do it everywhere, removing
various ad-hoc hacks that were spread around elsewhere.

This changes expectations of two tests:

• clip-path-recursion-002.svg: Now we consider the inner clip-path reference
  invalid. This matches WebKit and Blink, and I don't see any spec text
  explicitly asking for our old behavior, so I just changed the test.

• element-paint-recursion.html: Changes the expectations of elements
  referencing themselves via -moz-element(). Now it is invalid, instead of
  painting ourselves once inside ourselves, which was a bit wild on its own.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=159aaab1c920d7d87e585cfa9d370a9b0aa277fc

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0fd8713b9050
Refactor svg observer setup to not do silly things when observing an ancestor. r=longsonr

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3853521bcf7
Flush the overflow changed tracker earlier. r=jwatt

Comment 13

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/20340 for changes under testing/web-platform/tests

Comment 14

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 15

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/0fd8713b9050
https://hg.mozilla.org/mozilla-central/rev/b3853521bcf7

Comment 16

[Web Platform Test Sync Bot (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 17

[Tyson Smith [:tsmith]]

Thanks Emilio! This issue was very popular with the fuzzers.

Comment 18

[Liz Henry (:lizzard) (relman/hg->git project)]

A bit late for 71 /esr uplift. We could aim for ESR uplift in the next cycle.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Is there a user impact which justifies ESR backport consideration or can we let this fix ride the regular trains to release?

Comment 20

[Emilio Cobos Álvarez (:emilio)]

Not really, we'd just process some changes a frame late or such.

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Thanks