1405547 - stylo: Assertion failure: aElement == doc->GetServoRestyleRoot() || BitsArePropagated [@ NoteDirtyElement]

Status: RESOLVED FIXED

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

Description

[Tyson Smith [:tsmith]]

Attachment: test_case.html

Assertion failure: aElement == doc->GetServoRestyleRoot() || BitsArePropagated(parent, aBits, doc->GetServoRestyleRoot()), at /src/dom/base/Element.cpp:4505

#0 NoteDirtyElement(mozilla::dom::Element*, unsigned int) /src/dom/base/Element.cpp:4504:3
#1 mozilla::ServoRestyleManager::SnapshotFor(mozilla::dom::Element*) /src/layout/base/ServoRestyleManager.cpp:1078:13
#2 mozilla::ServoRestyleManager::ContentStateChanged(nsIContent*, mozilla::EventStates) /src/layout/base/ServoRestyleManager.cpp:1274:36
#3 mozilla::PresShell::ContentStateChanged(nsIDocument*, nsIContent*, mozilla::EventStates) /src/layout/base/PresShell.cpp:4254:37
#4 nsDocument::ContentStateChanged(nsIContent*, mozilla::EventStates) /src/dom/base/nsDocument.cpp:5668:3
#5 mozilla::dom::Element::UpdateState(bool) /src/dom/base/Element.cpp:273:14
#6 mozilla::dom::Link::ResetLinkState(bool, bool) /src/dom/base/Link.cpp:792:15
#7 nsDocument::RefreshLinkHrefs() /src/dom/base/nsDocument.cpp:9692:23
#8 nsDocument::SetBaseURI(nsIURI*) /src/dom/base/nsDocument.cpp:3882:3
#9 mozilla::dom::SetBaseURIUsingFirstBaseWithHref(nsIDocument*, nsIContent*) /src/dom/html/HTMLSharedElement.cpp:201:14
#10 mozilla::dom::HTMLSharedElement::UnbindFromTree(bool, bool) /src/dom/html/HTMLSharedElement.cpp:294:7
#11 mozilla::dom::Element::UnbindFromTree(bool, bool) /src/dom/base/Element.cpp:2016:37
#12 nsGenericHTMLElement::UnbindFromTree(bool, bool) /src/dom/html/nsGenericHTMLElement.cpp:537:20
#13 nsINode::doRemoveChildAt(unsigned int, bool, nsIContent*, nsAttrAndChildArray&) /src/dom/base/nsINode.cpp:1945:9
#14 mozilla::dom::FragmentOrElement::RemoveChildAt(unsigned int, bool) /src/dom/base/FragmentOrElement.cpp:1363:5
#15 nsINode::RemoveChild(nsINode&, mozilla::ErrorResult&) /src/dom/base/nsINode.cpp:604:3
#16 mozilla::dom::NodeBinding::removeChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /src/obj-firefox/dom/bindings/NodeBinding.cpp:1012:45
#17 mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /src/dom/bindings/BindingUtils.cpp:3053:13
#18 js::CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) /src/js/src/jscntxtinlines.h:293:15
#19 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:495:16
#20 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#21 Interpret(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:3085:18
#22 js::RunScript(JSContext*, js::RunState&) /src/js/src/vm/Interpreter.cpp:435:12
#23 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /src/js/src/vm/Interpreter.cpp:513:15
#24 InternalCall(JSContext*, js::AnyInvokeArgs const&) /src/js/src/vm/Interpreter.cpp:540:12
#25 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /src/js/src/vm/Interpreter.cpp:559:10
#26 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /src/js/src/jsapi.cpp:2975:12
#27 mozilla::dom::EventHandlerNonNull::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /src/obj-firefox/dom/bindings/EventHandlerBinding.cpp:260:37
#28 void mozilla::dom::EventHandlerNonNull::Call<nsISupports*>(nsISupports* const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JSCompartment*) /src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:362:12
#29 mozilla::JSEventHandler::HandleEvent(nsIDOMEvent*) /src/dom/events/JSEventHandler.cpp:215:12
#30 mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, nsIDOMEvent*, mozilla::dom::EventTarget*) /src/dom/events/EventListenerManager.cpp:1112:51
#31 mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent**, mozilla::dom::EventTarget*, nsEventStatus*) /src/dom/events/EventListenerManager.cpp:1283:20
#32 mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:313:17
#33 mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&) /src/dom/events/EventDispatcher.cpp:462:16
#34 mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, nsIDOMEvent*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*) /src/dom/events/EventDispatcher.cpp:822:9
#35 nsDocumentViewer::LoadComplete(nsresult) /src/layout/base/nsDocumentViewer.cpp:1081:7
#36 nsDocShell::EndPageLoad(nsIWebProgress*, nsIChannel*, nsresult) /src/docshell/base/nsDocShell.cpp:7760:21
#37 nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /src/docshell/base/nsDocShell.cpp:7558:7
#38 non-virtual thunk to nsDocShell::OnStateChange(nsIWebProgress*, nsIRequest*, unsigned int, nsresult) /src/docshell/base/nsDocShell.cpp:7455:13
#39 nsDocLoader::DoFireOnStateChange(nsIWebProgress*, nsIRequest*, int&, nsresult) /src/uriloader/base/nsDocLoader.cpp:1320:3
#40 nsDocLoader::doStopDocumentLoad(nsIRequest*, nsresult) /src/uriloader/base/nsDocLoader.cpp:861:14
#41 nsDocLoader::DocLoaderIsEmpty(bool) /src/uriloader/base/nsDocLoader.cpp:750:9
#42 nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:632:5
#43 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsISupports*, nsresult) /src/uriloader/base/nsDocLoader.cpp:488:14
#44 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) /src/netwerk/base/nsLoadGroup.cpp:629:28
#45 imgRequestProxy::RemoveFromLoadGroup(bool) /src/image/imgRequestProxy.cpp:382:15
#46 imgRequestProxy::OnLoadComplete(bool) /src/image/imgRequestProxy.cpp:1004:5
#47 void mozilla::image::ImageObserverNotifier<mozilla::image::ObserverTable const*>::operator()<void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::{lambda(mozilla::image::IProgressObserver*)#9}>(void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::{lambda(mozilla::image::IProgressObserver*)#9}) /src/image/ProgressTracker.cpp:292:9
#48 void mozilla::image::SyncNotifyInternal<mozilla::image::ObserverTable const*>(mozilla::image::ObserverTable const* const&, bool, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /src/image/ProgressTracker.cpp:377:5
#49 mozilla::image::ProgressTracker::SyncNotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&)::$_1::operator()(mozilla::image::ObserverTable const*) const /src/image/ProgressTracker.cpp:412:5
#50 _ZNK7mozilla5image11CopyOnWriteINS0_13ObserverTableEE4ReadIZNS0_15ProgressTracker18SyncNotifyProgressEjRKNS_3gfx12IntRectTypedINS6_12UnknownUnitsEEEE3$_1EEDTclfp_scPKS2_LDnEEET_ /src/image/CopyOnWrite.h:154:12
#51 mozilla::image::ProgressTracker::SyncNotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&) /src/image/ProgressTracker.cpp:411:14
#52 mozilla::image::RasterImage::NotifyProgress(unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<unsigned int> const&, mozilla::image::DecoderFlags, mozilla::image::SurfaceFlags) /src/image/RasterImage.cpp:1757:28
#53 mozilla::image::RasterImage::NotifyForLoadEvent(unsigned int) /src/image/RasterImage.cpp:1035:3
#54 mozilla::image::RasterImage::NotifyDecodeComplete(mozilla::image::DecoderFinalStatus const&, mozilla::image::ImageMetadata const&, mozilla::image::DecoderTelemetry const&, unsigned int, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, mozilla::Maybe<unsigned int> const&, mozilla::image::DecoderFlags, mozilla::image::SurfaceFlags) /src/image/RasterImage.cpp:1844:7
#55 mozilla::image::IDecodingTask::NotifyDecodeComplete(mozilla::NotNull<mozilla::image::RasterImage*>, mozilla::NotNull<mozilla::image::Decoder*>)::$_2::operator()() const /src/image/IDecodingTask.cpp:130:12
#56 mozilla::detail::RunnableFunction<mozilla::image::IDecodingTask::NotifyDecodeComplete(mozilla::NotNull<mozilla::image::RasterImage*>, mozilla::NotNull<mozilla::image::Decoder*>)::$_2>::Run() /src/xpcom/threads/nsThreadUtils.h:527:5
#57 nsThread::ProcessNextEvent(bool, bool*) /src/xpcom/threads/nsThread.cpp:1039:14
#58 NS_ProcessNextEvent(nsIThread*, bool) /src/xpcom/threads/nsThreadUtils.cpp:524:10
#59 mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /src/ipc/glue/MessagePump.cpp:97:21
#60 MessageLoop::RunInternal() /src/ipc/chromium/src/base/message_loop.cc:326:10
#61 MessageLoop::Run() /src/ipc/chromium/src/base/message_loop.cc:299:3
#62 nsBaseAppShell::Run() /src/widget/nsBaseAppShell.cpp:158:27
#63 nsAppStartup::Run() /src/toolkit/components/startup/nsAppStartup.cpp:288:30
#64 XREMain::XRE_mainRun() /src/toolkit/xre/nsAppRunner.cpp:4701:22
#65 XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4865:8
#66 XRE_main(int, char**, mozilla::BootstrapConfig const&) /src/toolkit/xre/nsAppRunner.cpp:4960:21
#67 do_main(int, char**, char**) /src/browser/app/nsBrowserApp.cpp:231:22
#68 main /src/browser/app/nsBrowserApp.cpp:304:16
#69 __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291
#70 _start (firefox+0x41eaf4)

Comment 1

[Tyson Smith [:tsmith]]

I can't reproduce this now. So I'll close it.

Comment 2

[Tyson Smith [:tsmith]]

Attachment: test_case.html

I found a working testcase.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

INFO: Last good revision: 08e7d627c2017392af5ba26086e682a61cbc88dd
INFO: First bad revision: 7f9883dd37feac26fb95b629ad1010107f04603c
INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=08e7d627c2017392af5ba26086e682a61cbc88dd&tochange=7f9883dd37feac26fb95b629ad1010107f04603c

Comment 4

[Emilio Cobos Álvarez (:emilio)]

;_;, that test-case has everything. Thanks for it...

The patch in bug 1400936 again adds the assertion, so this was buggy before too, it was just not asserted... I can look into it though.

Comment 5

[Chris Peterson [:cpeterson]]

Assigning to Emilio for now because he said he would look into it.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1405547: Account for mid-unbind conditions in the BitsArePropagated assertion too.

I'm not adding a crashtest because the one provided is kinda unreliable, and I
suck and I wasn't able to find a better one (though I didn't try that hard
actually).

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

Comment 7

[Cameron McCormack (:heycam)]

Comment on attachment 8920774 [details]
Bug 1405547: Account for mid-unbind conditions in the BitsArePropagated assertion too.

https://reviewboard.mozilla.org/r/191762/#review197004

::: commit-message-bb484:3
(Diff revision 1)
> +I'm not adding a crashtest because the one provided is kinda unreliable, and I
> +suck and I wasn't able to find a better one (though I didn't try that hard
> +actually).

While I think it is fine to describe in a commit message why we don't include particular changes that readers might expect (such as a test case), I don't think it's the right place to be talking about how much effort the patch author put in to find one.  (That, as a message to the reviewer, would be better in a Bugzilla comment.)

Comment 8

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8920774 [details]
Bug 1405547: Account for mid-unbind conditions in the BitsArePropagated assertion too.

https://reviewboard.mozilla.org/r/191762/#review197004

> While I think it is fine to describe in a commit message why we don't include particular changes that readers might expect (such as a test case), I don't think it's the right place to be talking about how much effort the patch author put in to find one.  (That, as a message to the reviewer, would be better in a Bugzilla comment.)

Fair enough, though bugzilla comments sometimes get ignored by other reviewers... I wish there was a "Reviewer disclaimer" message or something like that.

Comment 9

[Cameron McCormack (:heycam)]

That's a fair point.  I will try hard to check Bugzilla comments when reviewing. :-)

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8920774 [details]
Bug 1405547: Account for mid-unbind conditions in the BitsArePropagated assertion too.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/191762/diff/1-2/

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/70f88a44c023
Account for mid-unbind conditions in the BitsArePropagated assertion too. r=heycam

Comment 12

[Chris Peterson [:cpeterson]]

We don't need to uplift this fix to Beta 57 because it just fixes a debug assertion failure.

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/70f88a44c023