Closed Bug 1404324 Opened 7 years ago Closed 7 years ago

stylo: AddressSanitizer: use-after-poison [@ HasView] with READ of size 8

Categories

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

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- disabled
firefox57 --- fixed
firefox58 --- fixed

People

(Reporter: jkratzer, Assigned: emilio)

References

(Blocks 2 open bugs)

Details

(Keywords: crash, csectype-framepoisoning, sec-low)

Attachments

(4 files, 2 obsolete files)

trigger-hasView.html
1.02 KB, text/html
Details
trigger-Type.html
912 bytes, text/html
Details
More reduced testcase
209 bytes, text/plain
Details
Use the placeholder state to remove out-of-flows that aren't real descendants of the destruction root.
7.31 KB, patch
bzbarsky
: review+
ritu
: approval-mozilla-beta+
Details | Diff | Splinter Review
Found while fuzzing mozilla-central rev e6c32278f32c. Will update shortly with a testcase. ================================================================= ==13659==ERROR: AddressSanitizer: use-after-poison on address 0x625006354d48 at pc 0x7fad2338b6da bp 0x7ffea758dd70 sp 0x7ffea758dd68 READ of size 8 at 0x625006354d48 thread T0 #0 0x7fad2338b6d9 in HasView /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2639:36 #1 0x7fad2338b6d9 in nsContainerFrame::PositionChildViews(nsIFrame*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1020 #2 0x7fad2338b272 in nsContainerFrame::PositionChildViews(nsIFrame*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:1023:9 #3 0x7fad2337fbc2 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:972:5 #4 0x7fad2337e575 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:330:7 #5 0x7fad2317750c in mozilla::PresShell::DoReflow(nsIFrame*, bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:9380:11 #6 0x7fad2318b541 in mozilla::PresShell::ProcessReflowCommands(bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:9553:24 #7 0x7fad2318a7a7 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4207:11 #8 0x7fad1f0e6db0 in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:557:5 #9 0x7fad1f0e6db0 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:8499 #10 0x7fad1ee9dcf2 in mozilla::dom::Element::GetPrimaryFrame(mozilla::FlushType) /builds/worker/workspace/build/src/dom/base/Element.cpp:2262:10 #11 0x7fad2139ef31 in nsGenericHTMLElement::GetInnerText(mozilla::dom::DOMString&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/html/nsGenericHTMLElement.cpp:3063:8 #12 0x7fad2090ef04 in mozilla::dom::HTMLElementBinding::get_innerText(JSContext*, JS::Handle<JSObject*>, nsGenericHTMLElement*, JSJitGetterCallArgs) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/HTMLElementBinding.cpp:272:9 #13 0x7fad20bde1e6 in mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:2922:13 #14 0x7fad27028e64 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #15 0x7fad27028e64 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #16 0x7fad2702a89f in InternalCall /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:540:12 #17 0x7fad2702a89f in Call /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:559 #18 0x7fad2702a89f in js::CallGetter(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:674 #19 0x7fad27fc5e93 in CallGetter /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2122:16 #20 0x7fad27fc5e93 in GetExistingProperty<js::AllowGC::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2175 #21 0x7fad27fc5e93 in NativeGetPropertyInline<js::AllowGC::CanGC> /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2378 #22 0x7fad27fc5e93 in js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/NativeObject.cpp:2414 #23 0x7fad272476fc in GetProperty /builds/worker/workspace/build/src/js/src/vm/NativeObject.h:1590:12 #24 0x7fad272476fc in GetObjectElementOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter-inl.h:524 #25 0x7fad272476fc in GetElementOperation /builds/worker/workspace/build/src/js/src/vm/Interpreter-inl.h:630 #26 0x7fad272476fc in js::jit::DoGetElemFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICGetElem_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jit/BaselineIC.cpp:833 #27 0x2b4b9f585946 (<unknown module>) 0x625006354d48 is located 5192 bytes inside of 8192-byte region [0x625006353900,0x625006355900) allocated by thread T0 here: #0 0x4bc3bc in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 #1 0x7fad1c314dcf in AllocateChunk /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:179:15 #2 0x7fad1c314dcf in InternalAllocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:214 #3 0x7fad1c314dcf in Allocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:72 #4 0x7fad1c314dcf in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:77 #5 0x7fad23375923 in AllocateByFrameID /builds/worker/workspace/build/src/layout/base/nsPresArena.h:38:12 #6 0x7fad23375923 in AllocateFrame /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:203 #7 0x7fad23375923 in operator new /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:33 #8 0x7fad23375923 in NS_NewViewportFrame(nsIPresShell*, nsStyleContext*) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:30 #9 0x7fad23216364 in nsCSSFrameConstructor::ConstructRootFrame() /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2830:5 #10 0x7fad23170c1b in mozilla::PresShell::Initialize(int, int) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:1754:36 #11 0x7fad1f030800 in nsContentSink::StartLayout(bool) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:1286:26 #12 0x7fad1f0e6b36 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:8470:13 #13 0x7fad1ee9dcf2 in mozilla::dom::Element::GetPrimaryFrame(mozilla::FlushType) /builds/worker/workspace/build/src/dom/base/Element.cpp:2262:10 #14 0x7fad21ef962c in mozilla::dom::SVGTransformableElement::GetBBox(mozilla::dom::SVGBoundingBoxOptions const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/svg/SVGTransformableElement.cpp:178:21 #15 0x7fad1fcf451d in mozilla::dom::SVGGraphicsElementBinding::getBBox(JSContext*, JS::Handle<JSObject*>, mozilla::dom::SVGGraphicsElement*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/SVGGraphicsElementBinding.cpp:367:60 #16 0x7fad20be0600 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3053:13 #17 0x7fad27028e64 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #18 0x7fad27028e64 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #19 0x7fad27012cc6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #20 0x7fad27012cc6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084 #21 0x7fad26ffa089 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #22 0x7fad2702b777 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:724:15 #23 0x7fad2702bfe2 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:756:12 #24 0x7fad27a7a6e9 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4667:12 #25 0x7fad1f1e43d9 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:265:8 #26 0x7fad228be548 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2244:25 #27 0x7fad228b997c in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1884:10 #28 0x7fad1ecd597f in nsContentUtils::RemoveScriptBlocker() /builds/worker/workspace/build/src/dom/base/nsContentUtils.cpp:5645:15 #29 0x7fad1f0c1b69 in nsDocument::EndUpdate(unsigned int) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:5332:3 #30 0x7fad213d122c in nsHTMLDocument::EndUpdate(unsigned int) /builds/worker/workspace/build/src/dom/html/nsHTMLDocument.cpp:2507:15 #31 0x7fad1f1bc7fe in ~mozAutoDocUpdate /builds/worker/workspace/build/src/dom/base/mozAutoDocUpdate.h:40:18 #32 0x7fad1f1bc7fe in nsINode::ReplaceOrInsertBefore(bool, nsINode*, nsINode*, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/base/nsINode.cpp:2537 #33 0x7fad1f8d0ca1 in InsertBefore /builds/worker/workspace/build/src/dom/base/nsINode.h:1840:12 #34 0x7fad1f8d0ca1 in AppendChild /builds/worker/workspace/build/src/dom/base/nsINode.h:1844 #35 0x7fad1f8d0ca1 in mozilla::dom::NodeBinding::appendChild(JSContext*, JS::Handle<JSObject*>, nsINode*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/NodeBinding.cpp:885 #36 0x7fad20be0600 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3053:13 #37 0x7fad27028e64 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #38 0x7fad27028e64 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #39 0x7fad27012cc6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #40 0x7fad27012cc6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084 #41 0x7fad26ffa089 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #42 0x7fad2702b777 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:724:15 SUMMARY: AddressSanitizer: use-after-poison /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2639:36 in HasView Shadow bytes around the buggy address: 0x0c4a80c62950: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80c62960: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80c62970: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80c62980: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80c62990: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f7 =>0x0c4a80c629a0: f7 f7 f7 f7 f7 f7 f7 f7 f7[f7]f7 f7 f7 f7 f7 f7 0x0c4a80c629b0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 00 0x0c4a80c629c0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80c629d0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 f7 0x0c4a80c629e0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 0x0c4a80c629f0: f7 f7 f7 f7 f7 00 00 00 00 00 00 00 00 00 00 00 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 ==13659==ABORTING
Attached file trigger-hasView.html —
Attached file trigger-Type.html —
This modified testcase triggers a slightly different stacktrace. ==20591==ERROR: AddressSanitizer: use-after-poison on address 0x62500126e405 at pc 0x7fb11f31bfe9 bp 0x7ffe4124bdd0 sp 0x7ffe4124bdc8 READ of size 1 at 0x62500126e405 thread T0 #0 0x7fb11f31bfe8 in Type /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2770:38 #1 0x7fb11f31bfe8 in IsTableColGroupFrame /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/FrameTypeList.h:96 #2 0x7fb11f31bfe8 in mozilla::ReflowInput::Init(nsPresContext*, mozilla::LogicalSize const*, nsMargin const*, nsMargin const*) /builds/worker/workspace/build/src/layout/generic/ReflowInput.cpp:376 #3 0x7fb11f48ce91 in nsHTMLScrollFrame::ReflowScrolledFrame(mozilla::ScrollReflowInput*, bool, bool, mozilla::ReflowOutput*, bool) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:525:18 #4 0x7fb11f48e8de in nsHTMLScrollFrame::ReflowContents(mozilla::ScrollReflowInput*, mozilla::ReflowOutput const&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:660:3 #5 0x7fb11f491a89 in nsHTMLScrollFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/nsGfxScrollFrame.cpp:1037:3 #6 0x7fb11f359c13 in nsContainerFrame::ReflowChild(nsIFrame*, nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, int, int, unsigned int, nsReflowStatus&, nsOverflowContinuationTracker*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:976:14 #7 0x7fb11f358575 in mozilla::ViewportFrame::Reflow(nsPresContext*, mozilla::ReflowOutput&, mozilla::ReflowInput const&, nsReflowStatus&) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:330:7 #8 0x7fb11f1565cc in mozilla::PresShell::DoReflow(nsIFrame*, bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:8936:11 #9 0x7fb11f16a601 in mozilla::PresShell::ProcessReflowCommands(bool) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:9109:24 #10 0x7fb11f169867 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4182:11 #11 0x7fb11f0dda34 in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:566:5 #12 0x7fb11f0dda34 in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1956 #13 0x7fb11f0e778e in WillRefresh /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2269:5 #14 0x7fb11f0e778e in non-virtual thunk to nsRefreshDriver::WillRefresh(mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2264 #15 0x7fb11f0dce8c in nsRefreshDriver::Tick(long, mozilla::TimeStamp) /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:1886:12 #16 0x7fb11f0e6d99 in nsRefreshDriver::FinishedWaitingForTransaction() /builds/worker/workspace/build/src/layout/base/nsRefreshDriver.cpp:2189:5 #17 0x7fb11a687530 in mozilla::layers::ClientLayerManager::DidComposite(unsigned long, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /builds/worker/workspace/build/src/gfx/layers/client/ClientLayerManager.cpp:529:32 #18 0x7fb11a767efb in mozilla::layers::CompositorBridgeChild::RecvDidComposite(unsigned long const&, unsigned long const&, mozilla::TimeStamp const&, mozilla::TimeStamp const&) /builds/worker/workspace/build/src/gfx/layers/ipc/CompositorBridgeChild.cpp:536:8 #19 0x7fb119773276 in mozilla::layers::PCompositorBridgeChild::OnMessageReceived(IPC::Message const&) /builds/worker/workspace/build/src/obj-firefox/ipc/ipdl/PCompositorBridgeChild.cpp:1473:20 #20 0x7fb1190db989 in mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2119:25 #21 0x7fb1190d899f in mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message&&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:2049:17 #22 0x7fb1190da0d4 in mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1895:5 #23 0x7fb1190da728 in mozilla::ipc::MessageChannel::MessageTask::Run() /builds/worker/workspace/build/src/ipc/glue/MessageChannel.cpp:1928:15 #24 0x7fb1183397e2 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14 #25 0x7fb1183534b8 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:524:10 #26 0x7fb11e842455 in SpinEventLoopUntil<mozilla::ProcessFailureBehavior::ReportToCaller, (lambda at /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:3080:31)> /builds/worker/workspace/build/src/obj-firefox/dist/include/nsThreadUtils.h:323:25 #27 0x7fb11e842455 in mozilla::dom::XMLHttpRequestMainThread::SendInternal(mozilla::dom::BodyExtractorBase const*) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:3080 #28 0x7fb11e843cd2 in mozilla::dom::XMLHttpRequestMainThread::Send(JSContext*, mozilla::dom::Nullable<mozilla::dom::DocumentOrBlobOrArrayBufferViewOrArrayBufferOrFormDataOrURLSearchParamsOrUSVString> const&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/dom/xhr/XMLHttpRequestMainThread.cpp:2886:11 #29 0x7fb11c41b9e7 in mozilla::dom::XMLHttpRequestBinding::send(JSContext*, JS::Handle<JSObject*>, mozilla::dom::XMLHttpRequest*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/XMLHttpRequestBinding.cpp:1249:9 #30 0x7fb11cbb9cb0 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3053:13 #31 0x7fb123002c44 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #32 0x7fb123002c44 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #33 0x7fb122fecaa6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #34 0x7fb122fecaa6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084 #35 0x7fb122fd3e69 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #36 0x7fb123005557 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:724:15 #37 0x7fb123005dc2 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:756:12 #38 0x7fb123a544c9 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4667:12 #39 0x7fb11b1bda89 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:265:8 #40 0x7fb11e89d608 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2244:25 #41 0x7fb11e898a3c in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1884:10 #42 0x7fb11e87c2a5 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1585:10 #43 0x7fb11e878808 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:149:18 #44 0x7fb11a0dca4f in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:225:18 #45 0x7fb11a0dca4f in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:700 #46 0x7fb11a0d64fa in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:501:7 #47 0x7fb11a0e04cb in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:130:20 #48 0x7fb1183397e2 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14 #49 0x7fb1183534b8 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:524:10 #50 0x7fb1190e3601 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21 #51 0x7fb11904596b in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #52 0x7fb11904596b in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #53 0x7fb11904596b in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #54 0x7fb11e9fe9ef in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27 #55 0x7fb122b5a741 in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:288:30 #56 0x7fb122d4b70b in XREMain::XRE_mainRun() /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4701:22 #57 0x7fb122d4d328 in XREMain::XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4865:8 #58 0x7fb122d4e75b in XRE_main(int, char**, mozilla::BootstrapConfig const&) /builds/worker/workspace/build/src/toolkit/xre/nsAppRunner.cpp:4960:21 #59 0x4ebfe3 in do_main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:231:22 #60 0x4ebfe3 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:304 #61 0x7fb13606f82f in __libc_start_main /build/glibc-bfm8X4/glibc-2.23/csu/../csu/libc-start.c:291 #62 0x41db38 in _start (/home/forb1dden/builds/mc-asan/firefox+0x41db38) 0x62500126e405 is located 2821 bytes inside of 8192-byte region [0x62500126d900,0x62500126f900) allocated by thread T0 here: #0 0x4bc3bc in malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:64:3 #1 0x7fb1182ededf in AllocateChunk /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:179:15 #2 0x7fb1182ededf in InternalAllocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:214 #3 0x7fb1182ededf in Allocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:72 #4 0x7fb1182ededf in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:77 #5 0x7fb11f34f923 in AllocateByFrameID /builds/worker/workspace/build/src/layout/base/nsPresArena.h:38:12 #6 0x7fb11f34f923 in AllocateFrame /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:203 #7 0x7fb11f34f923 in operator new /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:33 #8 0x7fb11f34f923 in NS_NewViewportFrame(nsIPresShell*, nsStyleContext*) /builds/worker/workspace/build/src/layout/generic/ViewportFrame.cpp:30 #9 0x7fb11f1f0364 in nsCSSFrameConstructor::ConstructRootFrame() /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:2830:5 #10 0x7fb11f14fcdb in mozilla::PresShell::Initialize(int, int) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:1729:36 #11 0x7fb11b009eb0 in nsContentSink::StartLayout(bool) /builds/worker/workspace/build/src/dom/base/nsContentSink.cpp:1286:26 #12 0x7fb11b0c01e6 in nsDocument::FlushPendingNotifications(mozilla::FlushType, mozilla::FlushTarget) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:8470:13 #13 0x7fb11ae7b145 in GetPrimaryFrame /builds/worker/workspace/build/src/dom/base/Element.cpp:2262:10 #14 0x7fb11ae7b145 in mozilla::dom::Element::GetClientRects() /builds/worker/workspace/build/src/dom/base/Element.cpp:1054 #15 0x7fb11c6c77fc in mozilla::dom::ElementBinding::getClientRects(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:2163:63 #16 0x7fb11cbb9cb0 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3053:13 #17 0x7fb123002c44 in CallJSNative /builds/worker/workspace/build/src/js/src/jscntxtinlines.h:293:15 #18 0x7fb123002c44 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:495 #19 0x7fb122fecaa6 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:546:12 #20 0x7fb122fecaa6 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3084 #21 0x7fb122fd3e69 in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:435:12 #22 0x7fb123005557 in js::ExecuteKernel(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value const&, js::AbstractFramePtr, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:724:15 #23 0x7fb123005dc2 in js::Execute(JSContext*, JS::Handle<JSScript*>, JSObject&, JS::Value*) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:756:12 #24 0x7fb123a544c9 in ExecuteScript(JSContext*, JS::AutoObjectVector&, JS::Handle<JSScript*>, JS::Value*) /builds/worker/workspace/build/src/js/src/jsapi.cpp:4667:12 #25 0x7fb11b1bda89 in nsJSUtils::ExecutionContext::CompileAndExec(JS::CompileOptions&, JS::SourceBufferHolder&, JS::MutableHandle<JSScript*>) /builds/worker/workspace/build/src/dom/base/nsJSUtils.cpp:265:8 #26 0x7fb11e89d608 in mozilla::dom::ScriptLoader::EvaluateScript(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:2244:25 #27 0x7fb11e898a3c in mozilla::dom::ScriptLoader::ProcessRequest(mozilla::dom::ScriptLoadRequest*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1884:10 #28 0x7fb11e87c2a5 in mozilla::dom::ScriptLoader::ProcessScriptElement(nsIScriptElement*) /builds/worker/workspace/build/src/dom/script/ScriptLoader.cpp:1585:10 #29 0x7fb11e878808 in mozilla::dom::ScriptElement::MaybeProcessScript() /builds/worker/workspace/build/src/dom/script/ScriptElement.cpp:149:18 #30 0x7fb11a0dca4f in AttemptToExecute /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIScriptElement.h:225:18 #31 0x7fb11a0dca4f in nsHtml5TreeOpExecutor::RunScript(nsIContent*) /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:700 #32 0x7fb11a0d64fa in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/worker/workspace/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:501:7 #33 0x7fb11a0e04cb in nsHtml5ExecutorFlusher::Run() /builds/worker/workspace/build/src/parser/html/nsHtml5StreamParser.cpp:130:20 #34 0x7fb1183397e2 in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1039:14 #35 0x7fb1183534b8 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:524:10 #36 0x7fb1190e3601 in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21 #37 0x7fb11904596b in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:326:10 #38 0x7fb11904596b in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:319 #39 0x7fb11904596b in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:299 #40 0x7fb11e9fe9ef in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:158:27 #41 0x7fb122b5a741 in nsAppStartup::Run() /builds/worker/workspace/build/src/toolkit/components/startup/nsAppStartup.cpp:288:30 SUMMARY: AddressSanitizer: use-after-poison /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:2770:38 in Type Shadow bytes around the buggy address: 0x0c4a80245c30: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80245c40: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80245c50: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80245c60: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 0x0c4a80245c70: 00 00 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 =>0x0c4a80245c80:[f7]f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 0x0c4a80245c90: f7 f7 f7 f7 f7 f7 f7 f7 00 00 00 00 00 00 00 00 0x0c4a80245ca0: 00 00 00 00 00 00 00 00 00 00 f7 f7 f7 f7 f7 f7 0x0c4a80245cb0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 0x0c4a80245cc0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 0x0c4a80245cd0: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 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 ==20591==ABORTING
Only reproduces with Stylo enabled. INFO: Last good revision: 91a488108e10bfd4df90ccf8b738ae5c4a0f0dc1 INFO: First bad revision: ab3c85d4d199c903f6359e276def141d67a000d7 INFO: Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=91a488108e10bfd4df90ccf8b738ae5c4a0f0dc1&tochange=ab3c85d4d199c903f6359e276def141d67a000d7 I've confirmed that the regression range is the same for both testcases too.
Blocks: 1324619
Has Regression Range: --- → yes
status-firefox55: --- → unaffected
status-firefox56: --- → disabled
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox-esr52: --- → unaffected
Component: Layout → CSS Parsing and Computation
Summary: AddressSanitizer: use-after-poison [@ HasView] with READ of size 8 → stylo: AddressSanitizer: use-after-poison [@ HasView] with READ of size 8
Also FWIW, this was crashing vanilla opt builds without requiring fuzzPriv to reproduce.
Priority: -- → P2
Flags: needinfo?(emilio)
Flags: needinfo?(bzbarsky)
Attached file More reduced testcase —
Assignee: nobody → emilio
(Didn't intend to take it just yet) So, I have a clear idea of why this happens, but I don't know of an easy fix off-hand. This is a pretty similar problem to bug 1402218. This trips a few assertions before running to the UAF. The reason why we're running into it is a mix of multiple reasons: (1) The rule tree entries mutating under the hood. (2) The first-line reparenting stuff reparenting eagerly frames that end up going away. (3) The frames looking at their style context to look the child list they belong to at removal time[1]. So, (1) is basically the same problem as bug 1402218, but for CSS rules. In particular, we don't tear down and rebuild the rule tree when CSS rules change because we don't have cached data on it. I'd really prefer to keep it that way, but this causes the same "same rule node doesn't represent the same style as before" problem as bug 1402218. Even rebuilding the whole rule tree, we'd need to actually somehow clone all the CSS rules (or at least the declaration blocks) for it to not have the same problem, and that seems fairly wasteful. (2) mixed with the <del> frame being out-of-flow, causes the first-line reparenting stuff to cause the <del> style to go from position: absolute to position: sticky, even though it has the same rule node. (3) is the bit that actually causes us to miss to delete the frame, and the subsequent UAF. When we see that <del>'s position has changed, we reframe it. We then infer the primary frame's child list is the kFloatList frame list, because our position is sticky due to (2) and (1). We are in the kAbsoluteList frame list though, so we don't delete the frame. So, that's the problem overview. Now, re. solutions to solve it, I guess there are a couple, with varying degrees of usefulness. The solution I'd implement, right now, is: (0) Stop looking at the frame's style to determine the child list an out of flow goes in. In particular, it seems to me that since out of flows always have a corresponding placeholder frame, and a subsequent PlaceholderFrameProperty, seems easy enough to just store the child list in that frame property, and use it. Given out of flow frames need to go through that placeholder frame path already, seems like the less risky change. Other solutions are, in order of preference, I think: (1) Do the first-line reparenting more lazily, once we've processed other change hints. That sounds doable, but somewhat gross. (2) Implement some kind of rule tree cloning, and clone it when stylesheets change. I think I'd want to discuss with Boris about how does that look to him before going ahead with (0). Luckily he already has a ni? for this bug. [1]: http://searchfox.org/mozilla-central/rev/298033405057ca7aa5099153797467eceeaa08b5/layout/base/nsLayoutUtils.cpp#1543
Assignee: emilio → nobody
Flags: needinfo?(emilio)
See Also: → 1402218
Argh. Clearly I made assumptions about how the ruletree works in stylo that are just not true.... I think for purposes of 57, we should pursue the safest possible fix. In this case, I think that means replicating the existing Gecko behavior as closely as possible, to avoid more unpleasant surprises. The key part of the Gecko behavior is that it does not update style contexts on things we plan to reframe. This is something that fundamentally changed with stylo's first-line stuff, because first-line indiscriminately reparents everything it sees. So what I would suggest is that we set a flag on frames we skip updating styles on because we plan to reframe them; I believe we have free bool bits on frames. Then the first-letter machinery also skips those frames. I agree that solution (0) is probably a good idea for safety's sake, and I think we should do it, in addition to what I propose. But I think it's a riskier change, in the sense of changing how something works fairly significantly, and am a bit worried about risk for 57. Solution (1) is sort of what I'm proposing, in effect, but maybe the above description is less gross than the way Emilio was thinking of it. ;) Solution (2) is something I agree we should avoid if we can, for stylo. We should also look into whether the cloning we do for inline style is really needed....
Flags: needinfo?(bzbarsky)
> Then the first-letter machinery also skips those frames. first-_line_, of course.
Group: core-security → layout-core-security
Emilio says bz is going to write the patch here.
Assignee: nobody → bzbarsky
Flags: needinfo?(bzbarsky)
Comment on attachment 8914398 [details] [diff] [review] Make sure to not reparent style contexts on about-to-be-destroyed frames Review of attachment 8914398 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/ServoRestyleManager.cpp @@ +811,5 @@ > + // frame itself). > + for (; > + styleFrame; > + styleFrame = nsLayoutUtils::GetNextContinuationOrIBSplitSibling(styleFrame)) { > + styleFrame->SetAboutToBeDestroyed(); I suspect this needs to account for display: contents too, right (going to the closest flattened tree parent with frames?). Otherwise it can also cause badness, I believe, given you won't find more ReconstructFrame hints down the tree. Also, need to make sure that the behavior is correct if we have elements up in the tree that get reframed for a given style change, but I believe that's ok because their style cannot have change in a significant way anyway, otherwise we'd have found their change hint instead. ::: layout/generic/nsIFrame.h @@ +2031,5 @@ > > /** > + * Return true if this frame is about to be destroyed. If true, > + * style recomputation should ensure that this frame's style context > + * does NOT get modified. Can we assert this in SetStyleContext()?
Attachment #8914398 - Flags: review?(emilio)
> I suspect this needs to account for display: contents too, right (going to the closest flattened tree parent with frames?). Hrm. So this is a situation where !styleFrame, but our element has display:contents, so we plan to reframe all its descendants? In that situation we should really mark all the frames for its kids, I guess. This is a bit sucky. I'll think about how to do it. Marking the nearest ancestor with frames is wrong, because that ancestor may not actually be being destroyed, right? Or does it get destroyed when we reframe a display:contents thing?
Flags: needinfo?(emilio)
Alternately, we could flag the node, but there are kinda limited bits there... and then we have to remember to unflag it too.
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #12) > > I suspect this needs to account for display: contents too, right (going to the closest flattened tree parent with frames?). > > Hrm. So this is a situation where !styleFrame, but our element has > display:contents, so we plan to reframe all its descendants? Right > In that situation we should really mark all the frames for its kids, I > guess. This is a bit sucky. I'll think about how to do it. Marking the > nearest ancestor with frames is wrong, because that ancestor may not > actually be being destroyed, right? Or does it get destroyed when we > reframe a display:contents thing? Hmm, yeah, I meant flagging all the closest ancestor frame's descendants, but that doesn't work because not all of them would necessarily be descendants of the frame. I'd just do (0), doesn't sound much less risky at this point I think. Or, flagging the node if it is a temporary measure and we uplift this code, then kill it.
Flags: needinfo?(emilio)
What worries me about just doing (0) is whether we manage to locate all the places that look at the frame's style. It's really not clear to me what the risk tradeoff is here, but it seems like a much more open-ended problem...
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #15) > What worries me about just doing (0) is whether we manage to locate all the > places that look at the frame's style. It's really not clear to me what the > risk tradeoff is here, but it seems like a much more open-ended problem... I think the only one we need to worry about nsLayoutUtils::GetChildListNameFor, but ICBW.
Talked to Emilio on IRC. I think's he's convinced me that the need-to-audit surface for (0) is smaller than I thought. He's going to write up a corresponding patch, with comments and stuff, I hope... I still think my approach can work, but display:contents makes it a lot more complicated, less obviously correct, and more likely to be slow.
Assignee: bzbarsky → emilio
Flags: needinfo?(bzbarsky)
Attached patch WIP (obsolete) — — Splinter Review
This is something like what I'm thinking about. I've haven't run it through try yet, but seems to do the work. I'm not quite sure why the old code was relying on the ancestor frame to delete the frame if needed. I can rejigger the patch to keep doing that. This is the only point I've found from ContentRemoved where we look at the old style in a way that it is actually dangerous. I'm not sure if I'm supposed to push this patch to try, if not, I guess I have a bunch of tests to run locally.
Attachment #8914471 - Flags: feedback?(bzbarsky)
This is a sec-low, so pushing it to try is fine, imo.
Comment on attachment 8914471 [details] [diff] [review] WIP >+ MOZ_ASSERT(!(aChildFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)); Probably worth making this MOZ_DIAGNOSTIC_ASSERT. Also, the OOF bits further down can go away, right? >+ MOZ_ASSERT(false, "unknown list"); Definitely MOZ_DIAGNOSTIC_ASSERT. If this is going wrong, we really want to know, and computing the predicate is cheap. ;) >- !nsLayoutUtils::IsProperAncestorFrame(aDestructRoot, oof)) { Why is losing this test ok? I've been trying to see whether we can end up seeing the bogus style data in some way. Things I've thought of: 1) Layout doing reparenting after style change. But that would always be preceded by a style recomputation and restyle processing, hence reframing. 2) Frame construction under the thing with bogus style. Might not work quite right, but then we'll reframe it all anyway, afaict. 3) Painting, same as #1. 4) nsLayoutUtils::GetNearestScrollableFrame. This could be kinda confused in cases when it does not flush style, but it could be confused in those cases no matter what, I would think. 5) nsFrameTraversal bits. These worry me a bit.... Not finding anything else right this second.
Attachment #8914471 - Flags: feedback?(bzbarsky) → feedback+
I'll make sure to include a crashtest. I've audited everything that can be reached from both ProcessRestyledFrames and from ContentRemoved (including DestroyFrom impls, what a joy of a morning), and I haven't found anything dangerous.
Attachment #8914398 - Attachment is obsolete: true
Attachment #8914471 - Attachment is obsolete: true
Attachment #8914754 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky [:bz] (still digging out from vacation mail) from comment #20) > Comment on attachment 8914471 [details] [diff] [review] > WIP > > >+ MOZ_ASSERT(!(aChildFrame->GetStateBits() & NS_FRAME_OUT_OF_FLOW)); > > Probably worth making this MOZ_DIAGNOSTIC_ASSERT. Done. > Also, the OOF bits further down can go away, right? Yes, will kill them before landing. > >+ MOZ_ASSERT(false, "unknown list"); > > Definitely MOZ_DIAGNOSTIC_ASSERT. If this is going wrong, we really want to > know, and computing the predicate is cheap. ;) Fair enough, just did that.
Comment on attachment 8914754 [details] [diff] [review] Use the placeholder state to remove out-of-flows that aren't real descendants of the destruction root. > Yes, will kill them before landing. Doesn't look like that's in this patch. But followup is OK for this part. r=me to land this assuming that followup gets filed.
Attachment #8914754 - Flags: review?(bzbarsky) → review+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #25) > I just went ahead and removed that branch. > > remote: > https://hg.mozilla.org/integration/mozilla-inbound/rev/ > cb247c8a0fe5dca1f9f8c58e53b4dd2ac0cd92c5 Err, that's a crappy commit message... I backed that out and landed with the right message in https://hg.mozilla.org/integration/mozilla-inbound/rev/5a034cdb5132
Blocks: 1405605
Comment on attachment 8914754 [details] [diff] [review] Use the placeholder state to remove out-of-flows that aren't real descendants of the destruction root. Approval Request Comment [Feature/Bug causing the regression]: 1324619 [User impact if declined]: crashes [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: no [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 too much [Why is the change risky/not risky?]: we're choosing which list to remove a frame for from a source of information that doesn't change, instead of one that may change and in which case we mess up stuff. [String changes made/needed]: none
Attachment #8914754 - Flags: approval-mozilla-beta?
I'd like this to bake in Nightly for a few days, will take in b7 most likely.
Group: layout-core-security → core-security-release
Comment on attachment 8914754 [details] [diff] [review] Use the placeholder state to remove out-of-flows that aren't real descendants of the destruction root. Stylo related, Beta57+
Attachment #8914754 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Emilio Cobos Álvarez [:emilio] from comment #28) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: no > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on Emilio's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
See Also: → 1421053
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: