1606492 - Assertion failure: !mForbiddenToFlush (This is bad!), at src/layout/base/PresShell.cpp:3922

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Tyson Smith [:tsmith]]

Attachment: testcase.html

Reduced with m-c:
BuildID=20191231094349
SourceStamp=66a1c07a8f48c5129aa3867813bacb6e7ef22d8c

Testcase requires pref layout.accessiblecaret.enabled=true

Assertion failure: !mForbiddenToFlush (This is bad!), at src/layout/base/PresShell.cpp:3922

#0 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)src/layout/base/PresShell.cpp:3998:39
#1 0x7fa6f4dd284e in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)src/dom/base/Document.cpp:9969:16
#2 0x7fa6f4dd27e9 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)src/dom/base/Document.cpp:9965:22
#3 0x7fa6f4da6aca in mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType)src/dom/base/Document.cpp:9890:3
#4 0x7fa6f3c788ba in nsDocLoader::DocLoaderIsEmpty(bool)src/uriloader/base/nsDocLoader.cpp:675:14
#5 0x7fa6f3c7ab54 in nsDocLoader::OnStopRequest(nsIRequest*, nsresult)src/uriloader/base/nsDocLoader.cpp:614:5
#6 0x7fa6f3c7b8bc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult)src/uriloader/base/nsDocLoader.cpp
#7 0x7fa6f21031ff in mozilla::net::nsLoadGroup::NotifyRemovalObservers(nsIRequest*, nsresult)src/netwerk/base/nsLoadGroup.cpp:594:22
#8 0x7fa6f2102052 in mozilla::net::nsLoadGroup::Cancel(nsresult)src/netwerk/base/nsLoadGroup.cpp:238:11
#9 0x7fa6f3c77f64 in nsDocLoader::Stop()src/uriloader/base/nsDocLoader.cpp:232:36
#10 0x7fa6fb813c8b in nsDocShell::Stop(unsigned int)src/docshell/base/nsDocShell.cpp:4284:5
#11 0x7fa6fb7fbf9b in nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**)src/docshell/base/nsDocShell.cpp
#12 0x7fa6fb823970 in nsDocShell::LoadURI(nsDocShellLoadState*, bool)src/docshell/base/nsDocShell.cpp:781:10
#13 0x7fa6f50302d7 in nsFrameLoader::ReallyStartLoadingInternal()src/dom/base/nsFrameLoader.cpp:662:23
#14 0x7fa6f502f90e in nsFrameLoader::ReallyStartLoading()src/dom/base/nsFrameLoader.cpp:540:17
#15 0x7fa6f4db75cf in mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders()src/dom/base/Document.cpp:8472:13
#16 0x7fa6f4db7259 in mozilla::dom::Document::EndUpdate()src/dom/base/Document.cpp:7080:3
#17 0x7fa6f4aad5b1 in mozAutoDocUpdate::~mozAutoDocUpdate()src/dom/base/mozAutoDocUpdate.h:34:18
#18 0x7fa6f4e0ec40 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool)src/dom/base/Element.cpp:2198:1
#19 0x7fa6f3e07393 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, bool)src/obj-firefox/dist/include/mozilla/dom/Element.h:829:12
#20 0x7fa6f92c5294 in mozilla::AccessibleCaret::SetCaretElementStyle(nsRect const&, float)src/layout/base/AccessibleCaret.cpp:321:18
#21 0x7fa6f92c4e3d in mozilla::AccessibleCaret::SetPosition(nsIFrame*, int)src/layout/base/AccessibleCaret.cpp:291:3
#22 0x7fa6f92cd24a in mozilla::AccessibleCaretManager::UpdateCaretsForSelectionMode(mozilla::EnumSet<mozilla::AccessibleCaretManager::UpdateCaretsHint, unsigned char> const&)::$_1::operator()(mozilla::AccessibleCaret*, nsIFrame*, int) constsrc/layout/base/AccessibleCaretManager.cpp:312:44
#23 0x7fa6f92cc019 in mozilla::AccessibleCaretManager::UpdateCaretsForSelectionMode(mozilla::EnumSet<mozilla::AccessibleCaretManager::UpdateCaretsHint, unsigned char> const&)src/layout/base/AccessibleCaretManager.cpp:332:7
#24 0x7fa6f92cb2cf in mozilla::AccessibleCaretManager::UpdateCarets(mozilla::EnumSet<mozilla::AccessibleCaretManager::UpdateCaretsHint, unsigned char> const&)src/layout/base/AccessibleCaretManager.cpp:193:7
#25 0x7fa6f92d1ebd in mozilla::AccessibleCaretManager::OnReflow()src/layout/base/AccessibleCaretManager.cpp:757:5
#26 0x7fa6f92c94ad in mozilla::AccessibleCaretEventHub::Reflow(double, double)src/layout/base/AccessibleCaretEventHub.cpp:616:11
#27 0x7fa6fb82ccc6 in nsDocShell::NotifyReflowObservers(bool, double, double)src/docshell/base/nsDocShell.cpp
#28 0x7fa6fb82ce5f in non-virtual thunk to nsDocShell::NotifyReflowObservers(bool, double, double)src/docshell/base/nsDocShell.cpp
#29 0x7fa6f92fa768 in mozilla::PresShell::DidDoReflow(bool)src/layout/base/PresShell.cpp:8969:15
#30 0x7fa6f9309d19 in mozilla::PresShell::ProcessReflowCommands(bool)src/layout/base/PresShell.cpp:9352:7
#31 0x7fa6f930882e in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush)src/layout/base/PresShell.cpp:4114:11
#32 0x7fa6f4dd284e in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)src/dom/base/Document.cpp:9969:16
#33 0x7fa6f4dd27e9 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush)src/dom/base/Document.cpp:9965:22
#34 0x7fa6f4da6aca in mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType)src/dom/base/Document.cpp:9890:3
#35 0x7fa6f3c788ba in nsDocLoader::DocLoaderIsEmpty(bool)src/uriloader/base/nsDocLoader.cpp:675:14
#36 0x7fa6f3c7ab54 in nsDocLoader::OnStopRequest(nsIRequest*, nsresult)src/uriloader/base/nsDocLoader.cpp:614:5
#37 0x7fa6f3c7b8bc in non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult)src/uriloader/base/nsDocLoader.cpp
#38 0x7fa6f21031ff in mozilla::net::nsLoadGroup::NotifyRemovalObservers(nsIRequest*, nsresult)src/netwerk/base/nsLoadGroup.cpp:594:22
#39 0x7fa6f2102052 in mozilla::net::nsLoadGroup::Cancel(nsresult)src/netwerk/base/nsLoadGroup.cpp:238:11
#40 0x7fa6f3c77f64 in nsDocLoader::Stop()src/uriloader/base/nsDocLoader.cpp:232:36
#41 0x7fa6fb813c8b in nsDocShell::Stop(unsigned int)src/docshell/base/nsDocShell.cpp:4284:5
#42 0x7fa6fb7fbf9b in nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**)src/docshell/base/nsDocShell.cpp
#43 0x7fa6fb823970 in nsDocShell::LoadURI(nsDocShellLoadState*, bool)src/docshell/base/nsDocShell.cpp:781:10
#44 0x7fa6f50302d7 in nsFrameLoader::ReallyStartLoadingInternal()src/dom/base/nsFrameLoader.cpp:662:23
#45 0x7fa6f502f90e in nsFrameLoader::ReallyStartLoading()src/dom/base/nsFrameLoader.cpp:540:17
#46 0x7fa6f4db75cf in mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders()src/dom/base/Document.cpp:8472:13
#47 0x7fa6f4db7259 in mozilla::dom::Document::EndUpdate()src/dom/base/Document.cpp:7080:3
#48 0x7fa6f4aad5b1 in mozAutoDocUpdate::~mozAutoDocUpdate()src/dom/base/mozAutoDocUpdate.h:34:18
#49 0x7fa6f4e0ec40 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool)src/dom/base/Element.cpp:2198:1
#50 0x7fa6f4e0e42b in mozilla::dom::Element::SetAttr(int, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool)src/obj-firefox/dist/include/mozilla/dom/Element.h:833:12
#51 0x7fa6f4e0e6fa in mozilla::dom::Element::SetAttribute(nsTSubstring<char16_t> const&, nsTSubstring<char16_t> const&, nsIPrincipal*, mozilla::ErrorResult&)src/dom/base/Element.cpp:1261:14
#52 0x7fa6f66dcfb3 in mozilla::dom::Element_Binding::setAttribute(JSContext*, JS::Handle<JSObject*>, void*, JSJitMethodCallArgs const&)src/obj-firefox/dom/bindings/ElementBinding.cpp:1306:24
#53 0x7fa6f6b6cafd in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*)src/dom/bindings/BindingUtils.cpp:3151:13
#54 0x7fa6fc3f7114 in CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), js::CallReason, JS::CallArgs const&)src/js/src/vm/Interpreter.cpp:452:13
#55 0x7fa6fc3f6818 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)src/js/src/vm/Interpreter.cpp:544:12
#56 0x7fa6fc3f820e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason)src/js/src/vm/Interpreter.cpp:608:10
#57 0x7fa6fc3ec763 in Interpret(JSContext*, js::RunState&)src/js/src/vm/Interpreter.cpp:3037:16
#58 0x7fa6fc3d4f38 in js::RunScript(JSContext*, js::RunState&)src/js/src/vm/Interpreter.cpp:424:10
#59 0x7fa6fc3f684f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct, js::CallReason)src/js/src/vm/Interpreter.cpp:580:13
#60 0x7fa6fc3f820e in InternalCall(JSContext*, js::AnyInvokeArgs const&, js::CallReason)src/js/src/vm/Interpreter.cpp:608:10
#61 0x7fa6fc3f8435 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>, js::CallReason)src/js/src/vm/Interpreter.cpp:625:8
#62 0x7fa6fc618b5a in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>)src/js/src/jsapi.cpp:2753:10
#63 0x7fa6f6622651 in 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:267:37
#64 0x7fa6f72be2f0 in void mozilla::dom::EventHandlerNonNull::Call<nsCOMPtr<mozilla::dom::EventTarget> >(nsCOMPtr<mozilla::dom::EventTarget> const&, mozilla::dom::Event&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&, char const*, mozilla::dom::CallbackObject::ExceptionHandling, JS::Realm*)src/obj-firefox/dist/include/mozilla/dom/EventHandlerBinding.h:364:12
#65 0x7fa6f72bcc20 in mozilla::JSEventHandler::HandleEvent(mozilla::dom::Event*)src/dom/events/JSEventHandler.cpp:201:12
#66 0x7fa6f728d690 in mozilla::EventListenerManager::HandleEventSubType(mozilla::EventListenerManager::Listener*, mozilla::dom::Event*, mozilla::dom::EventTarget*)src/dom/events/EventListenerManager.cpp:1071:22
#67 0x7fa6f728eed4 in mozilla::EventListenerManager::HandleEventInternal(nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event**, mozilla::dom::EventTarget*, nsEventStatus*, bool)src/dom/events/EventListenerManager.cpp:1263:17
#68 0x7fa6f727f085 in mozilla::EventTargetChainItem::HandleEvent(mozilla::EventChainPostVisitor&, mozilla::ELMCreationDetector&)src/dom/events/EventDispatcher.cpp:356:17
#69 0x7fa6f727ddc6 in mozilla::EventTargetChainItem::HandleEventTargetChain(nsTArray<mozilla::EventTargetChainItem>&, mozilla::EventChainPostVisitor&, mozilla::EventDispatchingCallback*, mozilla::ELMCreationDetector&)src/dom/events/EventDispatcher.cpp:558:16
#70 0x7fa6f7281507 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)src/dom/events/EventDispatcher.cpp:1056:11
#71 0x7fa6f7284c81 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*)src/dom/events/EventDispatcher.cpp
#72 0x7fa6f5058ffc in nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&)src/dom/base/nsINode.cpp:1119:17
#73 0x7fa6f4b267e8 in nsContentUtils::DispatchEvent(mozilla::dom::Document*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch)src/dom/base/nsContentUtils.cpp:4098:28
#74 0x7fa6f4b264a0 in nsContentUtils::DispatchTrustedEvent(mozilla::dom::Document*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, bool*)src/dom/base/nsContentUtils.cpp:4068:10
#75 0x7fa6f7505dc2 in mozilla::dom::HTMLMediaElement::DispatchEvent(nsTSubstring<char16_t> const&)src/dom/html/HTMLMediaElement.cpp:6177:10
#76 0x7fa6f1e03fe7 in mozilla::SchedulerGroup::Runnable::Run()src/xpcom/threads/SchedulerGroup.cpp:282:20
#77 0x7fa6f1e42524 in nsThread::ProcessNextEvent(bool, bool*)src/xpcom/threads/nsThread.cpp:1241:14
#78 0x7fa6f1e4c5f6 in NS_ProcessNextEvent(nsIThread*, bool)src/xpcom/threads/nsThreadUtils.cpp:486:10
#79 0x7fa6f2d87e15 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)src/ipc/glue/MessagePump.cpp:87:21
#80 0x7fa6f2c9148c in MessageLoop::RunInternal()src/ipc/chromium/src/base/message_loop.cc:315:10
#81 0x7fa6f2c91302 in MessageLoop::Run()src/ipc/chromium/src/base/message_loop.cc:290:3
#82 0x7fa6f8e8834a in nsBaseAppShell::Run()src/widget/nsBaseAppShell.cpp:137:27
#83 0x7fa6fc1c360d in XRE_RunAppShell()src/toolkit/xre/nsEmbedFunctions.cpp:946:20
#84 0x7fa6f2d88b19 in mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*)src/ipc/glue/MessagePump.cpp:237:9
#85 0x7fa6f2c9148c in MessageLoop::RunInternal()src/ipc/chromium/src/base/message_loop.cc:315:10
#86 0x7fa6f2c91302 in MessageLoop::Run()src/ipc/chromium/src/base/message_loop.cc:290:3
#87 0x7fa6fc1c2bf3 in XRE_InitChildProcess(int, char**, XREChildData const*)src/toolkit/xre/nsEmbedFunctions.cpp:781:34
#88 0x55bb3b675ceb in content_process_main(mozilla::Bootstrap*, int, char**)src/browser/app/../../ipc/contentproc/plugin-container.cpp:56:28
#89 0x55bb3b6760f7 in mainsrc/browser/app/nsBrowserApp.cpp:303:18

Comment 1

[Andrew McCreight [:mccr8]]

Emilio, do you know who might know about whatever is going wrong here? It looks like some kind of reentrance issue. Thanks.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Ting-Yu is the most familiar with the AccessibleCaret code, but I think this is a more general problem with document updates I think... Triggering frame loads from Document::EndUpdate seems quite problematic.

Anyhow maybe he has thoughts.

Comment 3

[Tyson Smith [:tsmith]]

Emilio mentioned that this is somewhere between a sec-high and sec-moderate. Exploiting a UAF is probably not be possible because of this assert: https://searchfox.org/mozilla-central/rev/bff49201f24569809f0db36100b02dcb9bf8dd8d/layout/base/PresShell.cpp#877

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Document::EndUpdate is rather expected place to trigger stuff which may run scripts.

Perhaps AccessibleCaret should do its work within a scriptblocker and then have a script runner to set attribute value.
That would be closer to for example some XUL layout code.

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

AccessibleCaretManager already restricts itself from flushing layout in its Reflow() handler. So I think AccessibleCaret can avoid notifying document when setting attribute value in various places or when inserting anonymous caret contents (bug 1536907). In this way, no other operation or observers invoked via Document::EndUpdate, which can further flushing layout, will be called.

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1606492 - Notify the document for AccessibleCaret only if we need a layout flush.

It is OK not to notify the document when creating caret elements.
However when changing caret's position, we only notify the document when
layout flush is needed.

Effectively, this avoids notifying the document in passive callbacks
including OnReflow(), OnScrollStart(), OnScrollStart(), and
OnScrollPositionChanged(). In other cases such as dragging carets,
notifying the document is needed, or the caret's position is not
updated at all.

Depends on D58663

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1606492 - Add nsAutoScriptBlocker to PresShell::DidDoReflow().

Add a script block to prevent reflow observers from running the scripts,
which may flush layout, until the end of DidDoReflow().

Specifically, Document::MaybeInitializeFinalizeFrameLoaders() can flush
layout somewhere down in the stack as bug 1606492 comment 0 shows.
Adding a script block can force it to schedule its runnable to run at
the end of DidDoReflow().

Also, HandlePostedReflowCallbacks() can flush layout. It's better to check
mIsDestroying before proceeding.

Comment 8

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

https://hg.mozilla.org/integration/autoland/rev/f7b4438f499f2598fee82412bf00a91cb0a02c4d
https://hg.mozilla.org/mozilla-central/rev/f7b4438f499f

Comment 9

[Ryan VanderMeulen [:RyanVM]]

Just wanted to double-check - this isn't an issue on the release branches because it depends on a non-default pref, right? No way to trigger this bug otherwise?

Comment 10

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Although layout.accessiblecaret.enabled=false by default, this is still an issue on release branch for device with touch screen like phones or tablet because layout.accessiblecaret.enabled_on_touch=true.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Thanks for the quick reply! In that case, it sounds like we should have Beta and ESR68 approval requests. The patch grafts cleanly to Beta as-landed but would need rebasing for ESR68, though.

Comment 12

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 9119225 [details]
Bug 1606492 - Add nsAutoScriptBlocker to PresShell::DidDoReflow().

Beta/Release Uplift Approval Request

• User impact if declined: Dev edition users on touchable devices can crash because of the assertion here.
  https://searchfox.org/mozilla-central/rev/be7d1f2d52dd9474ca2df145190a817614c924e4/layout/base/PresShell.cpp#3925
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Minor code change to delay scripts running.
• String changes made/needed: None

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)

Thanks for the quick reply! In that case, it sounds like we should have Beta and ESR68 approval requests. The patch grafts cleanly to Beta as-landed but would need rebasing for ESR68, though.

I run the crashtest locally on ESR68, and it didn't crash. I think uplifting my patch to beta should be sufficient.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 9119225 [details]
Bug 1606492 - Add nsAutoScriptBlocker to PresShell::DidDoReflow().

Fixes a crash. Approved for 73.0b3.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0c58033c5c7

Comment 16

[Frederik Braun [:freddy]]

Ting-Yu, can you please remember to delay checking in crash tests for security bugs. See https://wiki.mozilla.org/Security/Firefox_security_bug_fixing#Landing_tests for more info.