Closed Bug 1507674 Opened 2 years ago Closed 10 months ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla72
Tracking Status
firefox-esr68 - wontfix
firefox65 --- wontfix
firefox66 --- wontfix
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- wontfix
firefox71 --- wontfix
firefox72 --- fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: assertion, testcase, Whiteboard: [fuzzblocker], [wptsync upstream])

Attachments

(3 files)

Attached file 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)
Flags: in-testsuite?
Flags: needinfo?(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 :(
Flags: needinfo?(emilio)
Assignee: nobody → emilio
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.
Priority: -- → P3
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.
Blocks: 794442

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?

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

Whiteboard: [fuzzblocker]

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

Flags: needinfo?(emilio)
Flags: needinfo?(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

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.

Flags: needinfo?(emilio)
Attachment #9025584 - Attachment description: Bug 1507674 - Flush the overflow changed tracker earlier. → Bug 1507674 - Flush the overflow changed tracker earlier. r=jwatt
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0fd8713b9050
Refactor svg observer setup to not do silly things when observing an ancestor. r=longsonr
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b3853521bcf7
Flush the overflow changed tracker earlier. r=jwatt
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/20340 for changes under testing/web-platform/tests
Whiteboard: [fuzzblocker] → [fuzzblocker], [wptsync upstream]
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Status: NEW → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Upstream PR merged by moz-wptsync-bot

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

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

Flags: in-testsuite? → in-testsuite+

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

Flags: needinfo?(emilio)

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

Flags: needinfo?(emilio)
You need to log in before you can comment on or make changes to this bug.