Closed Bug 1578933 Opened 5 years ago Closed 5 years ago

crash near null in [@ MoveToNextLiveEntry]

Categories

(Core :: Layout, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox-esr60 --- wontfix
firefox-esr68 70+ fixed
firefox69 --- wontfix
firefox70 + fixed
firefox71 + fixed

People

(Reporter: tsmith, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, sec-high, testcase, Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70+r][adv-esr68.2+][adv-esr68.2+r])

Attachments

(3 files)

Attached file testcase.html

Reduced with m-c:
BuildID=20190904094319
SourceStamp=174361d152923aea2b24226a57cde3099509ed6a

==45055==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000010 (pc 0x7f9028c2214b bp 0x7ffc4a362d70 sp 0x7ffc4a362d60 T0)
==45055==The signal is caused by a READ memory access.
==45055==Hint: address points to the zero page.
    #0 0x7f9028c2214a in MoveToNextLiveEntry src/xpcom/ds/PLDHashTable.cpp:782:30
    #1 0x7f9028c2214a in PLDHashTable::Iterator::Next() src/xpcom/ds/PLDHashTable.cpp:755
    #2 0x7f90327c461e in FlushPendingScrollAnchorAdjustments src/layout/base/PresShell.cpp:2580:13
    #3 0x7f90327c461e in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4177
    #4 0x7f902cefbe09 in FlushPendingNotifications src/obj-firefox/dist/include/mozilla/PresShell.h:1446:5
    #5 0x7f902cefbe09 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) src/dom/base/Document.cpp:10001
    #6 0x7f902cf4bc58 in FlushPendingNotifications src/dom/base/Document.cpp:9931:3
    #7 0x7f902cf4bc58 in GetPrimaryFrame src/dom/base/Element.cpp:233
    #8 0x7f902cf4bc58 in mozilla::dom::Element::GetScrollFrame(nsIFrame**, mozilla::FlushType) src/dom/base/Element.cpp:663
    #9 0x7f902cf4ee81 in mozilla::dom::Element::GetClientAreaRect() src/dom/base/Element.cpp:985:28
    #10 0x7f902f131823 in ClientHeight src/obj-firefox/dist/include/mozilla/dom/Element.h:1276:50
    #11 0x7f902f131823 in mozilla::dom::Element_Binding::get_clientHeight(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitGetterCallArgs) src/obj-firefox/dom/bindings/ElementBinding.cpp:3665
    #12 0x7f902f783094 in bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3064:13
    #13 0x7f9036366e17 in CallJSNative src/js/src/vm/Interpreter.cpp:447:13
    #14 0x7f9036366e17 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:539
    #15 0x7f9036369b42 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:610:8
    #16 0x7f9036ed2738 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2723:10
    #17 0x7f902b0504b7 in Call src/obj-firefox/dist/include/jsapi.h:1620:10
    #18 0x7f902b0504b7 in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const src/js/xpconnect/wrappers/XrayWrapper.cpp:2089
    #19 0x7f9036585a76 in getInternal src/js/src/proxy/Proxy.cpp:344:19
    #20 0x7f9036585a76 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352
    #21 0x7f9036585cb3 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #22 0x7f9036585cb3 in getInternal src/js/src/proxy/Proxy.cpp:340
    #23 0x7f9036585cb3 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352
    #24 0x7f9036585cb3 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #25 0x7f9036585cb3 in getInternal src/js/src/proxy/Proxy.cpp:340
    #26 0x7f9036585cb3 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352
    #27 0x7f9036585cb3 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #28 0x7f9036585cb3 in getInternal src/js/src/proxy/Proxy.cpp:340
    #29 0x7f9036585cb3 in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352
    #30 0x7f90365872d8 in GetProperty src/js/src/vm/ObjectOperations-inl.h:114:12
    #31 0x7f90365872d8 in getInternal src/js/src/proxy/Proxy.cpp:340
    #32 0x7f90365872d8 in js::ProxyGetPropertyByValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:369
    #33 0x114bf3be71cf  (<unknown module>)
Flags: in-testsuite?
Flags: needinfo?(emilio)

This doesn't repro for me on the current nightly regardless of the value of layout.css.individual-transform.enabled. Any tip to reproduce? :)

Flags: needinfo?(emilio) → needinfo?(twsmith)

I double checked and I can repro with m-c:
BuildID=20190905094317
SourceStamp=df1cc95342be524ea4eb32f0ece1fa73d12cab14

Tested on Ubuntu 16.04 x86_64. With a clean profile I set layout.css.individual-transform.enabled=true then did a drag and drop with the test case. If you have issues I should be able to create a Pernosco session later today.

Using a debug build I get:

Assertion failure: !mApplyingAnchorAdjustment, at src/layout/generic/ScrollAnchorContainer.cpp:375

#0 mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() src/layout/generic/ScrollAnchorContainer.cpp:391:55
#1 mozilla::PresShell::FlushPendingScrollAnchorAdjustments() src/layout/base/PresShell.cpp:2582:23
#2 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4177:9
#3 mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) src/dom/base/Document.cpp:10021:16
#4 mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) src/dom/base/Document.cpp:10017:22
#5 mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) src/dom/base/Document.cpp:9951:3
#6 nsDocLoader::DocLoaderIsEmpty(bool) src/uriloader/base/nsDocLoader.cpp:675:14
#7 nsDocLoader::OnStopRequest(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp:614:5
#8 non-virtual thunk to nsDocLoader::OnStopRequest(nsIRequest*, nsresult) src/uriloader/base/nsDocLoader.cpp
#9 mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) src/netwerk/base/nsLoadGroup.cpp:568:22
#10 mozilla::net::nsLoadGroup::Cancel(nsresult) src/netwerk/base/nsLoadGroup.cpp:221:11
#11 nsDocLoader::Stop() src/uriloader/base/nsDocLoader.cpp:235:36
#12 nsDocShell::Stop(unsigned int) src/docshell/base/nsDocShell.cpp:4651:5
#13 nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**) src/docshell/base/nsDocShell.cpp
#14 nsDocShell::LoadURI(nsDocShellLoadState*) src/docshell/base/nsDocShell.cpp:777:10
#15 nsFrameLoader::ReallyStartLoadingInternal() src/dom/base/nsFrameLoader.cpp:661:23
#16 nsFrameLoader::ReallyStartLoading() src/dom/base/nsFrameLoader.cpp:539:17
#17 mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders() src/dom/base/Document.cpp:8492:13
#18 mozilla::dom::Document::EndUpdate() src/dom/base/Document.cpp:7014:3
#19 mozAutoDocUpdate::~mozAutoDocUpdate() src/dom/base/mozAutoDocUpdate.h:34:18
#20 mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool) src/dom/base/Element.cpp:2386:1
#21 mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, bool) src/obj-firefox/dist/include/mozilla/dom/Element.h:835:12
#22 mozilla::ScrollFrameHelper::SetCoordAttribute(mozilla::dom::Element*, nsAtom*, int) src/layout/generic/nsGfxScrollFrame.cpp:6357:13
#23 mozilla::ScrollFrameHelper::UpdateScrollbarPosition() src/layout/generic/nsGfxScrollFrame.cpp:5102:5
#24 mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsAtom*) src/layout/generic/nsGfxScrollFrame.cpp:3009:5
#25 mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, nsAtom*) src/layout/generic/nsGfxScrollFrame.cpp:2211:3
#26 mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, nsAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) src/layout/generic/nsGfxScrollFrame.cpp:2343:5
#27 mozilla::ScrollFrameHelper::ScrollTo(nsPoint, mozilla::ScrollMode, nsAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode) src/layout/generic/nsGfxScrollFrame.cpp:2246:3
#28 mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() src/layout/generic/ScrollAnchorContainer.cpp:378:17
#29 mozilla::PresShell::FlushPendingScrollAnchorAdjustments() src/layout/base/PresShell.cpp:2582:23
#30 mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) src/layout/base/PresShell.cpp:4177:9
#31 mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) src/dom/base/Document.cpp:10021:16
#32 mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) src/dom/base/Document.cpp:9951:3
#33 nsIContent::GetPrimaryFrame(mozilla::FlushType) src/dom/base/Element.cpp:233:10
#34 mozilla::dom::Element::GetScrollFrame(nsIFrame**, mozilla::FlushType) src/dom/base/Element.cpp:663:21
#35 mozilla::dom::Element::GetClientAreaRect() src/dom/base/Element.cpp:985:28
#36 mozilla::dom::Element::ClientHeight() src/obj-firefox/dist/include/mozilla/dom/Element.h:1276:50
#37 mozilla::dom::Element_Binding::get_clientHeight(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitGetterCallArgs) src/obj-firefox/dom/bindings/ElementBinding.cpp:3665:39
#38 bool mozilla::dom::binding_detail::GenericGetter<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) src/dom/bindings/BindingUtils.cpp:3064:13
#39 CallJSNative(JSContext*, bool (*)(JSContext*, unsigned int, JS::Value*), JS::CallArgs const&) src/js/src/vm/Interpreter.cpp:447:13
#40 js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) src/js/src/vm/Interpreter.cpp:539:12
#41 InternalCall(JSContext*, js::AnyInvokeArgs const&) src/js/src/vm/Interpreter.cpp:594:10
#42 js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) src/js/src/vm/Interpreter.cpp:610:8
#43 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/js/src/jsapi.cpp:2723:10
#44 JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JSObject*>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) src/obj-firefox/dist/include/jsapi.h:1620:10
#45 xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) const src/js/xpconnect/wrappers/XrayWrapper.cpp:2089:10
#46 js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:344:19
#47 js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352:10
#48 js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/vm/ObjectOperations-inl.h:114:12
#49 js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:340:14
#50 js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352:10
#51 js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/vm/ObjectOperations-inl.h:114:12
#52 js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:340:14
#53 js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352:10
#54 js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/vm/ObjectOperations-inl.h:114:12
#55 js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:340:14
#56 js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:352:10
#57 js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/vm/ObjectOperations-inl.h:114:12
#58 js::Proxy::getInternal(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<JS::PropertyKey>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:340:14
#59 js::ProxyGetPropertyByValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) src/js/src/proxy/Proxy.cpp:369:10
Flags: needinfo?(twsmith)

Huh, can repro now on the latest Nightly, that's odd...

Flags: needinfo?(emilio)

This is bad.

Group: layout-core-security, core-security
Group: core-security
Keywords: sec-high
Attached file Stack #1
Edit: clear because bugzilla tricked me before I finished the comment...

So what's going on is the following:

  • We run the onloadstart event on the media element (the <audio> element on the test-case). That triggers the go() function, all good so far.
  • We set the src of an <iframe>, then call setAttribute("ondurationchange", "x()") on the media element. That triggers the go function from the DOMSubtreeModified event handler, which scrolls
  • That goes through a doc update which does end up in Document::MaybeInitializeFinalizeFrameLoaders (as we have a frame loader to initialize).
  • That ends up calling nsDocShell::Stop on the iframe with id="c", which flushes style from DocLoaderIsEmpty() in order to figure out whether we're done with loads.
  • That flushes layout on the parent document, as we promote style flushes to layout flushes. That's also ~fine, though not great. The iframe document is about:blank at that point. It's a bit unfortunate that setting the src attribute of an about:blank iframe flushes the parent document layout, but that's just an observation... Anyhow at that point the stack looks like attachment 9090769 [details].
  • So we go and flush the parent document... Note that there's no sort of script blocker on the stack at this point.
  • We get to ProcessReflowCommands in PresShell::FlushPendingNotifications. We reflow the <audio> element, which has the controls attribute set, and it's the first time we reflow the controls div, so we post an internal resizevideocontrols event. That runs at a bad time, like:
#0  0x00007f2ccb8aad56 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)
    (aTarget=0x7f2cb10f78b0, aPresContext=0x7f2cb120b000, aEvent=0x7f2cb12f3820, aDOMEvent=0x7f2cb12dcd00, aEventStatus=0x7fff6e929794, aCallback=0x0, aTargets=0x0) at /home/emilio/src/moz/gecko/dom/events/EventDispatcher.cpp:717
#1  0x00007f2ccb8ae995 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*)
    (aTarget=0x7f2cb10f78b0, aEvent=0x0, aDOMEvent=0x7f2cb12dcd00, aPresContext=0x7f2cb120b000, aEventStatus=0x7fff6e929794) at /home/emilio/src/moz/gecko/dom/events/EventDispatcher.cpp:1142
#2  0x00007f2cc9ce675a in nsINode::DispatchEvent(mozilla::dom::Event&, mozilla::dom::CallerType, mozilla::ErrorResult&) (this=0x7f2cb10f78b0, aEvent=..., aCallerType=mozilla::dom::CallerType::System, aRv=...)
    at /home/emilio/src/moz/gecko/dom/base/nsINode.cpp:1061
#3  0x00007f2cc987b6ef in nsContentUtils::DispatchEvent(mozilla::dom::Document*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, mozilla::Trusted, bool*, mozilla::ChromeOnlyDispatch)
    (aDoc=0x7f2cb1246000, aTarget=0x7f2cb10f78b0, aEventName=..., aCanBubble=mozilla::CanBubble::eNo, aCancelable=mozilla::Cancelable::eNo, aComposed=mozilla::Composed::eDefault, aTrusted=mozilla::Trusted::eYes, aDefaultAction=0x0, aOnlyChromeDispatch=mozilla::ChromeOnlyDispatch::eNo) at /home/emilio/src/moz/gecko/dom/base/nsContentUtils.cpp:3973
#4  0x00007f2cc987b414 in nsContentUtils::DispatchTrustedEvent(mozilla::dom::Document*, nsISupports*, nsTSubstring<char16_t> const&, mozilla::CanBubble, mozilla::Cancelable, mozilla::Composed, bool*)
    (aDoc=0x7f2cb1246000, aTarget=0x7f2cb10f78b0, aEventName=..., aCanBubble=mozilla::CanBubble::eNo, aCancelable=mozilla::Cancelable::eNo, aComposed=mozilla::Composed::eDefault, aDefaultAction=0x0)
    at /home/emilio/src/moz/gecko/dom/base/nsContentUtils.cpp:3943
#5  0x00007f2ccd6dca35 in DispatchResizeEvent::Run() (this=0x7f2cb12b1d80) at /home/emilio/src/moz/gecko/layout/generic/nsVideoFrame.cpp:248
#6  0x00007f2cc9882d17 in nsContentUtils::RemoveScriptBlocker() () at /home/emilio/src/moz/gecko/dom/base/nsContentUtils.cpp:5178
#7  0x00007f2cc8edc549 in nsAutoScriptBlocker::~nsAutoScriptBlocker() (this=0x7fff6e929ab0) at /home/emilio/src/moz/gecko/dom/base/nsContentUtils.h:3333
#8  0x00007f2ccd36e9b2 in mozilla::PresShell::ProcessReflowCommands(bool) (this=0x7f2cb10a3000, aInterruptible=false) at /home/emilio/src/moz/gecko/layout/base/PresShell.cpp:9441
#9  0x00007f2ccd36e200 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7f2cb10a3000, aFlush=...) at /home/emilio/src/moz/gecko/layout/base/PresShell.cpp:4174
#10 0x00007f2cc98d2346 in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7f2cb10a3000, aType=...) at /home/emilio/src/moz/gecko/obj-debug-noopt/dist/include/mozilla/PresShell.h:1446
#11 0x00007f2cc9aad6f1 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7f2cb1246000, aFlush=...) at /home/emilio/src/moz/gecko/dom/base/Document.cpp:10019
#12 0x00007f2cc9aad69e in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7f2cb10f8000, aFlush=...) at /home/emilio/src/moz/gecko/dom/base/Document.cpp:10015
#13 0x00007f2cc9a9330a in mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) (this=0x7f2cb10f8000, aType=mozilla::FlushType::Style) at /home/emilio/src/moz/gecko/dom/base/Document.cpp:9949
#14 0x00007f2cc8d54b74 in nsDocLoader::DocLoaderIsEmpty(bool) (this=0x7f2cb13ed800, aFlushLayout=true) at /home/emilio/src/moz/gecko/uriloader/base/nsDocLoader.cpp:675
#15 0x00007f2cc8d56553 in nsDocLoader::OnStopRequest(nsIRequest*, nsresult) (this=0x7f2cb13ed800, aRequest=0x7f2cb125fe98, aStatus=-2142568446) at /home/emilio/src/moz/gecko/uriloader/base/nsDocLoader.cpp:614
#16 0x00007f2cc74bafc2 in mozilla::net::nsLoadGroup::RemoveRequest(nsIRequest*, nsISupports*, nsresult) (this=0x7f2cb10a6d50, request=0x7f2cb125fe98, ctxt=0x0, aStatus=-2142568446)
    at /home/emilio/src/moz/gecko/netwerk/base/nsLoadGroup.cpp:568
#17 0x00007f2cc74b8dfa in mozilla::net::nsLoadGroup::Cancel(nsresult) (this=0x7f2cb10a6d50, status=-2142568446) at /home/emilio/src/moz/gecko/netwerk/base/nsLoadGroup.cpp:221
#18 0x00007f2cc8d546e9 in nsDocLoader::Stop() (this=0x7f2cb13ed800) at /home/emilio/src/moz/gecko/uriloader/base/nsDocLoader.cpp:235
#19 0x00007f2ccf62be55 in nsDocShell::Stop() (this=0x7f2cb13ed800) at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.h:213
#20 0x00007f2ccf5c59cd in nsDocShell::Stop(unsigned int) (this=0x7f2cb13ed800, aStopFlags=1) at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:4651
#21 0x00007f2ccf5d5c7d in nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**) (this=0x7f2cb13ed800, aLoadState=0x7f2cb2393120, aDocShell=0x0, aRequest=0x0)
    at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:9568
  • It is a bad time because we have pending scroll anchor adjustments, and that event does flush layout:
(rr) p DumpJSStack()
0 get(obj = [cross-compartment wrapper], prop = "clientHeight", [object HTMLAudioElement]) ["chrome://global/content/elements/videocontrols.js":2043:20]
    this = [object Object]
1 updateReflowedDimensions() ["chrome://global/content/elements/videocontrols.js":2078:8]
    this = [object Object]
2 handleControlEvent(aEvent = [cross-compartment wrapper]) ["chrome://global/content/elements/videocontrols.js":848:17]
    this = [object Object]
3 handleEvent(aEvent = [cross-compartment wrapper]) ["chrome://global/content/elements/videocontrols.js":621:15]
    this = [object Object]
4 go() ["file:///home/emilio/src/moz/gecko/t.html":13:4]
    this = [object Window]
5 onloadstart(event = [object Event]) ["file:///home/emilio/src/moz/gecko/t.html":1:0]
    this = [object HTMLAudioElement]
  • That reentrant flush manages to fire yet another reentrant flush via the scrolling code:
#0  0x00007f2ccb8aad56 in mozilla::EventDispatcher::Dispatch(nsISupports*, nsPresContext*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsEventStatus*, mozilla::EventDispatchingCallback*, nsTArray<mozilla::dom::EventTarget*>*)
    (aTarget=0x7f2cb1098220, aPresContext=0x7f2cb1363800, aEvent=0x7f2cb12f38b0, aDOMEvent=0x7f2cb12fc6d0, aEventStatus=0x0, aCallback=0x0, aTargets=0x0) at /home/emilio/src/moz/gecko/dom/events/EventDispatcher.cpp:717
#1  0x00007f2ccb8ae995 in mozilla::EventDispatcher::DispatchDOMEvent(nsISupports*, mozilla::WidgetEvent*, mozilla::dom::Event*, nsPresContext*, nsEventStatus*)
    (aTarget=0x7f2cb1098220, aEvent=0x0, aDOMEvent=0x7f2cb12fc6d0, aPresContext=0x7f2cb1363800, aEventStatus=0x0) at /home/emilio/src/moz/gecko/dom/events/EventDispatcher.cpp:1142
#2  0x00007f2ccd409e52 in nsDocumentViewer::PermitUnloadInternal(unsigned int*, bool*) (this=0x7f2cb13039b0, aPermitUnloadFlags=0x7fff6e920834, aPermitUnload=0x7fff6e920bff)
    at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:1334
#3  0x00007f2ccd409921 in nsDocumentViewer::PermitUnload(unsigned int, bool*) (this=0x7f2cb13039b0, aPermitUnloadFlags=0, aPermitUnload=0x7fff6e920bff) at /home/emilio/src/moz/gecko/layout/base/nsDocumentViewer.cpp:1261
#4  0x00007f2cc996e53d in nsIContentViewer::PermitUnload(bool*) (this=0x7f2cb13039b0, canUnload=0x7fff6e920bff) at /home/emilio/src/moz/gecko/obj-debug-noopt/dist/include/nsIContentViewer.h:83
#5  0x00007f2ccf5d4f6e in nsDocShell::InternalLoad(nsDocShellLoadState*, nsIDocShell**, nsIRequest**) (this=0x7f2caf285800, aLoadState=0x7f2cb2393c80, aDocShell=0x0, aRequest=0x0)
    at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:9416
#6  0x00007f2ccf5d2822 in nsDocShell::LoadURI(nsDocShellLoadState*) (this=0x7f2caf285800, aLoadState=0x7f2cb2393c80) at /home/emilio/src/moz/gecko/docshell/base/nsDocShell.cpp:777
#7  0x00007f2cc9cca16f in nsFrameLoader::ReallyStartLoadingInternal() (this=0x7f2cb10a6940) at /home/emilio/src/moz/gecko/dom/base/nsFrameLoader.cpp:661
#8  0x00007f2cc9cc9690 in nsFrameLoader::ReallyStartLoading() (this=0x7f2cb10a6940) at /home/emilio/src/moz/gecko/dom/base/nsFrameLoader.cpp:539
#9  0x00007f2cc9a9cf6a in mozilla::dom::Document::MaybeInitializeFinalizeFrameLoaders() (this=0x7f2cb1246000) at /home/emilio/src/moz/gecko/dom/base/Document.cpp:8490
#10 0x00007f2cc9a9ccb0 in mozilla::dom::Document::EndUpdate() (this=0x7f2cb1246000) at /home/emilio/src/moz/gecko/dom/base/Document.cpp:7012
#11 0x00007f2cc9836927 in mozAutoDocUpdate::~mozAutoDocUpdate() (this=0x7fff6e921380) at /home/emilio/src/moz/gecko/dom/base/mozAutoDocUpdate.h:34
#12 0x00007f2cc9ad0948 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, nsIPrincipal*, bool)
    (this=0x7f2caf221670, aNamespaceID=0, aName=0x7f2cc2a30ea0 <mozilla::detail::gGkAtoms+60408>, aPrefix=0x0, aValue=..., aSubjectPrincipal=0x0, aNotify=true) at /home/emilio/src/moz/gecko/dom/base/Element.cpp:2386
#13 0x00007f2cc8ec9edb in mozilla::dom::Element::SetAttr(int, nsAtom*, nsAtom*, nsTSubstring<char16_t> const&, bool)
    (this=0x7f2caf221670, aNameSpaceID=0, aName=0x7f2cc2a30ea0 <mozilla::detail::gGkAtoms+60408>, aPrefix=0x0, aValue=..., aNotify=true) at /home/emilio/src/moz/gecko/obj-debug-noopt/dist/include/mozilla/dom/Element.h:835
#14 0x00007f2cc8ed11f5 in mozilla::dom::Element::SetAttr(int, nsAtom*, nsTSubstring<char16_t> const&, bool) (this=0x7f2caf221670, aNameSpaceID=0, aName=0x7f2cc2a30ea0 <mozilla::detail::gGkAtoms+60408>, aValue=..., aNotify=true)
    at /home/emilio/src/moz/gecko/obj-debug-noopt/dist/include/mozilla/dom/Element.h:831
#15 0x00007f2ccd5cc6e1 in mozilla::ScrollFrameHelper::SetCoordAttribute(mozilla::dom::Element*, nsAtom*, int) (this=0x7f2cb106f248, aElement=0x7f2caf221670, aAtom=0x7f2cc2a30ea0 <mozilla::detail::gGkAtoms+60408>, aSize=1096)
    at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:6357
#16 0x00007f2ccd5c4bfa in mozilla::ScrollFrameHelper::UpdateScrollbarPosition() (this=0x7f2cb106f248) at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:5102
#17 0x00007f2ccd5c2462 in mozilla::ScrollFrameHelper::ScrollToImpl(nsPoint, nsRect const&, nsAtom*) (this=0x7f2cb106f248, aPt=..., aRange=..., aOrigin=0x7f2cc2a35490 <mozilla::detail::gGkAtoms+78312>)
    at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:3009
#18 0x00007f2ccd5c27a6 in mozilla::ScrollFrameHelper::CompleteAsyncScroll(nsRect const&, nsAtom*) (this=0x7f2cb106f248, aRange=..., aOrigin=0x7f2cc2a36684 <mozilla::detail::gGkAtoms+82908>)
    at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:2211
#19 0x00007f2ccd5c2f88 in mozilla::ScrollFrameHelper::ScrollToWithOrigin(nsPoint, mozilla::ScrollMode, nsAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode)
    (this=0x7f2cb106f248, aScrollPosition=..., aMode=mozilla::ScrollMode::Instant, aOrigin=0x7f2cc2a36684 <mozilla::detail::gGkAtoms+82908>, aRange=0x0, aSnap=nsIScrollbarMediator::DISABLE_SNAP)
    at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:2343
#20 0x00007f2ccd5be790 in mozilla::ScrollFrameHelper::ScrollTo(nsPoint, mozilla::ScrollMode, nsAtom*, nsRect const*, nsIScrollbarMediator::ScrollSnapMode)
    (this=0x7f2cb106f248, aScrollPosition=..., aMode=mozilla::ScrollMode::Instant, aOrigin=0x7f2cc2a36684 <mozilla::detail::gGkAtoms+82908>, aRange=0x0, aSnap=nsIScrollbarMediator::DISABLE_SNAP)
    at /home/emilio/src/moz/gecko/layout/generic/nsGfxScrollFrame.cpp:2246
#21 0x00007f2ccd4bb239 in mozilla::layout::ScrollAnchorContainer::ApplyAdjustments() (this=0x7f2cb106f398) at /home/emilio/src/moz/gecko/layout/generic/ScrollAnchorContainer.cpp:378
#22 0x00007f2ccd367020 in mozilla::PresShell::FlushPendingScrollAnchorAdjustments() (this=0x7f2cb10a3000) at /home/emilio/src/moz/gecko/layout/base/PresShell.cpp:2582
#23 0x00007f2ccd36e219 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7f2cb10a3000, aFlush=...) at /home/emilio/src/moz/gecko/layout/base/PresShell.cpp:4177
#24 0x00007f2cc98d2346 in mozilla::PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7f2cb10a3000, aType=...) at /home/emilio/src/moz/gecko/obj-debug-noopt/dist/include/mozilla/PresShell.h:1446
#25 0x00007f2cc9aad6f1 in mozilla::dom::Document::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x7f2cb1246000, aFlush=...) at /home/emilio/src/moz/gecko/dom/base/Document.cpp:10019
#26 0x00007f2cc9a9330a in mozilla::dom::Document::FlushPendingNotifications(mozilla::FlushType) (this=0x7f2cb1246000, aType=mozilla::FlushType::Layout) at /home/emilio/src/moz/gecko/dom/base/Document.cpp:9949
#27 0x00007f2cc9acaf66 in nsIContent::GetPrimaryFrame(mozilla::FlushType) (this=0x7f2caf3f1800, aType=mozilla::FlushType::Layout) at /home/emilio/src/moz/gecko/dom/base/Element.cpp:233
#28 0x00007f2cc9acd0c9 in mozilla::dom::Element::GetScrollFrame(nsIFrame**, mozilla::FlushType) (this=0x7f2caf3f1800, aFrame=0x7fff6e922278, aFlushType=mozilla::FlushType::Layout) at /home/emilio/src/moz/gecko/dom/base/Element.cpp:663

And that tries to apply anchor adjustments reentrantly, which is terrible. Need to think about the best fix for this.

Have to jump to a meeting, but will take care of this. Things that are wrong:

  • JS code running at that time after we reflow but before we apply scroll anchoring stuff.
  • Scroll anchoring stuff triggering layout (o.O).
Assignee: nobody → emilio

This is a type of crash why WeakFrames were originally added. Often it is more elegant to fix the issue in some other way, but
one could possibly initialize a WeakFrame before ScrollTo call, and after the call check whether the frame is still alive, and if not, return.

Well, that's alright, the scrolling code is full of those. But the issue is not only the scrolling code, but the fact that we assume that nothing happens from this line to the next, but in this case we're re-entering layout. That's not good.

Priority: -- → P2

I wanted to fix the more general problem and script-block more of
FlushPendingNotifications, but simple attempts to do that have resulted in
terribly orange try runs with very bizarre failures, so in the "perfect is the
enemy of good" spirit, fix the issue at hand (scroll anchoring adjustments not
dealing with layout reentering beneath them) by running them while
script-blocked, which is the right thing to do anyway.

Flags: needinfo?(emilio)

The patch makes an WPT pass (again, we have bug 1521272 on file...).

So maybe we should just land it in that bug instead? WDYT Daniel?

Flags: needinfo?(dveditz)

Comment on attachment 9095516 [details]
Bug 1578933 - Run scroll anchoring adjustments when blocking script.

Security Approval Request

  • How easily could an exploit be constructed based on the patch?: Not too easily, I'd think. The commit message hints the thing, but that can be changed of course.
  • Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?: Yes
  • Which older supported branches are affected by this flaw?: all
  • If not all supported branches, which bug introduced the flaw?: None
  • Do you have backports for the affected branches?: No
  • If not, how different, hard to create, and risky will they be?: Should apply cleanly.
  • How likely is this patch to cause regressions; how much testing does it need?: not likely. It actually makes one test pass.
Attachment #9095516 - Flags: sec-approval?

Since this is disabled in beta and only on in nightly, it seems safe enough.

sec-approval+.

Attachment #9095516 - Flags: sec-approval? → sec-approval+

(In reply to Al Billings [:abillings] from comment #13)

Since this is disabled in beta and only on in nightly, it seems safe enough.

sec-approval+.

I'm pretty sure this can repro with another combination of transform or properties that are enabled everywhere. Does that change your mind?

Flags: needinfo?(abillings)

Also would be useful to know your opinion on comment 11.

I don't think it changes my mind but I'll defer to Dan.
My last day at Mozilla is tomorrow...

Flags: needinfo?(abillings)

(In reply to Emilio Cobos Álvarez (:emilio) from comment #11)

The patch makes an WPT pass (again, we have bug 1521272 on file...). So maybe we should just land it in that bug instead? WDYT Daniel?

I don't see a need to obscure this patch by landing in the "fix WPT" bug. Changing the timing of the flush doesn't easily let you work back to a reproducing testcase. The fuzzing-based test Tyson filed looked like an uninteresting near-null crash and that might be what other folks end up with if they try. For future code maintenance I would think check-in comments that lead back to the discussion here would be more useful than the practically empty bug 1521272.

(In reply to Emilio Cobos Álvarez (:emilio) from comment #14)

I'm pretty sure this can repro with another combination of transform or properties that are enabled everywhere. Does that change your mind?

Not about the sec-approval, but it does imply we need to also take this fix on Beta and ESR.

Alright. Will land then. Thanks both!

Group: layout-core-security → core-security-release
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Please nominate this for Beta and ESR68 approval when you get a chance.

Flags: needinfo?(emilio)

Comment on attachment 9095516 [details]
Bug 1578933 - Run scroll anchoring adjustments when blocking script.

Beta/Release Uplift Approval Request

  • User impact if declined: Sec-high
  • 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): Relatively low-risk fix for an scroll-anchoring reentrancy issue.
  • String changes made/needed: none

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration:
  • User impact if declined: sec
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Relatively low-risk fix for an scroll-anchoring reentrancy issue.
  • String or UUID changes made by this patch: none
Flags: needinfo?(emilio)
Attachment #9095516 - Flags: approval-mozilla-esr68?
Attachment #9095516 - Flags: approval-mozilla-beta?
Flags: in-testsuite? → in-testsuite+

Comment on attachment 9095516 [details]
Bug 1578933 - Run scroll anchoring adjustments when blocking script.

Fixes a scroll anchoring sec bug. Approved for 70.0b11 and 68.2esr.

Attachment #9095516 - Flags: approval-mozilla-esr68?
Attachment #9095516 - Flags: approval-mozilla-esr68+
Attachment #9095516 - Flags: approval-mozilla-beta?
Attachment #9095516 - Flags: approval-mozilla-beta+
Flags: qe-verify-
Whiteboard: [post-critsmash-triage]

Actually I'd like to land the crashtest as well once this is everywhere.

Flags: in-testsuite+ → in-testsuite?
Whiteboard: [post-critsmash-triage] → [post-critsmash-triage][adv-main70+][adv-main70-rollup]
Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70-rollup] → [post-critsmash-triage][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup]
Whiteboard: [post-critsmash-triage][adv-main70+][adv-main70-rollup][adv-esr68.2+][adv-esr68.2-rollup] → [post-critsmash-triage][adv-main70+][adv-main70+r][adv-esr68.2+][adv-esr68.2+r]
Group: core-security-release
Regressions: 1719483
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: