Closed Bug 1282076 (CVE-2016-5274) Opened 8 years ago Closed 8 years ago

use-after-poison in nsFrameManager::CaptureFrameState

Categories

(Core :: DOM: Animation, defect, P1)

Product:
Core
Component:
DOM: Animation
Version:
47 Branch
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox47 + wontfix
firefox48 + wontfix
firefox49 + verified
firefox-esr45 49+ verified
firefox50 + verified

People

(Reporter: nils, Assigned: hiro)

References

Details

(5 keywords, Whiteboard: [adv-main49+][adv-esr45.4+])

Crash Data

Attachments

(3 files, 4 obsolete files)

Web animations' test case
1.63 KB, text/html
Details
Store all non-inherited structs which are stored on the rule node on the style context if the we have animation data
7.71 KB, patch
hiro
: review+
abillings
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-esr45+
abillings
: sec-approval+
Details | Diff | Splinter Review
Crash tests
4.59 KB, patch
heycam
: review+
Details | Diff | Splinter Review 
The testcase crashes the latest Firefox ASAN build (BuildID=20160624030212) as follows:

<script>
function start() {
        o0=document;
        o24=document.createElement('table');
        o35=window;
        o60=document.createElement('input');
        o24.appendChild(o60);
        o62=document.body;
        o66=document.createElement('input');
        o62.appendChild(o66);
        o60.innerHTML="<svg><color-profile><script><rect><animateColor><style><style>*{ all: unset<script><style>div<style>";
        o93=o60.querySelectorAll('*')[5];
        o97=o60.querySelectorAll('*')[9];
        document.body.appendChild(o24);
        o305=document.createTextNode("{}:first-line{");
        o93.appendChild(o305);
        o318=(new DOMParser()).parseFromString('','text/html');
        o320=o318.all[1];
        o355=document.createElement('style');
        o356=document.createTextNode("@keyframes key2{ from{ box-shadow: none}}#id2{ animation-name: key2; animation-duration: 0.01s");
        o355.appendChild(o356);
        o97.appendChild(o355);
        o66.style.display='list-item';
        o473=document.createElement('script');
        o24.appendChild(o473);
        o577=document.createElement('style');
        o320.appendChild(o577);
        o577.style.position='fixed';
        document.replaceChild(o318.documentElement,document.documentElement);
        o908=(new DOMParser()).parseFromString('','text/html');
        o911=o908.all[2];
        o911.style.display='inline';
        o577.id='id2';
        o1202=document.createElement('table');
        document.body=o911;
        document.body.appendChild(o1202);
        document.replaceChild(o0.documentElement,document.documentElement);
        o1232=o473.parentNode;
        o1233=o1232.parentNode;
        document.body=o1233;
        o35.scrollByLines(1);
        o577.style.position='absolute';
        setTimeout(f2, 4);
}
function f2() {
        o0.designMode='on';
        o0.execCommand('insertparagraph',false,null);
}
</script>
<body onload="start()"></body>


ASAN ouput:

=================================================================
==30757==ERROR: AddressSanitizer: use-after-poison on address 0x6250002eab58 at pc 0x7fcd17c53104 bp 0x7ffc2eca31b0 sp 0x7ffc2eca31a8
READ of size 8 at 0x6250002eab58 thread T0 (Web Content)
    #0 0x7fcd17c53103 in GetStateBits /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsIFrame.h:1565:46
    #1 0x7fcd17c53103 in nsFrameManager::CaptureFrameState(nsIFrame*, nsILayoutHistoryState*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsFrameManager.cpp:580
    #2 0x7fcd17aa8070 in CaptureStateForFramesOf /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:9144:5
    #3 0x7fcd17aa8070 in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, bool, nsCSSFrameConstructor::RemoveFlags, nsIContent**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:9563
    #4 0x7fcd17aa8d23 in mozilla::RestyleManager::RestyleElement(mozilla::dom::Element*, nsIFrame*, nsChangeHint, mozilla::RestyleTracker&, nsRestyleHint, mozilla::RestyleHintData const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleManager.cpp:1077:5
    #5 0x7fcd17ac8d0f in ProcessOneRestyle /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleTracker.cpp:94:5
    #6 0x7fcd17ac8d0f in mozilla::RestyleTracker::DoProcessRestyles() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleTracker.cpp:262
    #7 0x7fcd17ab03af in ProcessRestyles /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RestyleManager.h:536:7
    #8 0x7fcd17ab03af in mozilla::RestyleManager::ProcessPendingRestyles() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/RestyleManager.cpp:1807
    #9 0x7fcd17cdb6ca in ProcessPendingRestyles /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/RestyleManagerHandleInlines.h:74:3
    #10 0x7fcd17cdb6ca in PresShell::FlushPendingNotifications(mozilla::ChangesToFlush) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsPresShell.cpp:4119
    #11 0x7fcd138be6be in nsDocument::FlushPendingNotifications(mozFlushType) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsDocument.cpp:8393:7
    #12 0x7fcd177eee46 in nsComputedDOMStyle::UpdateCurrentStyleSources(bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsComputedDOMStyle.cpp:633:3
    #13 0x7fcd177f005b in nsComputedDOMStyle::GetPropertyCSSValue(nsAString_internal const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsComputedDOMStyle.cpp:796:3
    #14 0x7fcd177ee507 in nsComputedDOMStyle::GetPropertyValue(nsAString_internal const&, nsAString_internal&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsComputedDOMStyle.cpp:379:26
    #15 0x7fcd17500cde in nsHTMLCSSUtils::GetCSSInlinePropertyBase(nsINode*, nsIAtom*, nsAString_internal&, nsHTMLCSSUtils::StyleType) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsHTMLCSSUtils.cpp:511:5
    #16 0x7fcd174e9e89 in GetComputedProperty /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsHTMLCSSUtils.cpp:491:10
    #17 0x7fcd174e9e89 in nsHTMLEditor::GetAbsolutelyPositionedSelectionContainer(nsIDOMElement**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsHTMLAbsPosition.cpp:91
    #18 0x7fcd174f4486 in nsHTMLEditor::CheckSelectionStateForAnonymousButtons(nsISelection*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsHTMLAnonymousUtils.cpp:307:11
    #19 0x7fcd175e8495 in nsHTMLEditor::EndUpdateViewBatch() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsHTMLEditor.cpp:4802:11
    #20 0x7fcd174b04ae in nsEditor::EndPlaceHolderTransaction() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsEditor.cpp:965:7
    #21 0x7fcd175d381c in ~nsAutoPlaceHolderBatch /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsEditorUtils.h:51:9
    #22 0x7fcd175d381c in nsHTMLEditor::InsertBasicBlock(nsAString_internal const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsHTMLEditor.cpp:2126
    #23 0x7fcd175d2216 in nsHTMLEditor::SetParagraphFormat(nsAString_internal const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/libeditor/nsHTMLEditor.cpp:1709:12
    #24 0x7fcd1768a384 in nsParagraphStateCommand::SetState(nsIEditor*, nsString&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/composer/nsComposerCommands.cpp:654:10
    #25 0x7fcd176897b0 in nsMultiStateCommand::DoCommandParams(char const*, nsICommandParams*, nsISupports*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/editor/composer/nsComposerCommands.cpp:599:12
    #26 0x7fcd18a65bd3 in nsControllerCommandTable::DoCommandParams(char const*, nsICommandParams*, nsISupports*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/embedding/components/commandhandler/nsControllerCommandTable.cpp:162:10
    #27 0x7fcd18a5ca8e in DoCommandWithParams /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/embedding/components/commandhandler/nsBaseCommandController.cpp:152:10
    #28 0x7fcd18a5ca8e in non-virtual thunk to nsBaseCommandController::DoCommandWithParams(char const*, nsICommandParams*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/embedding/components/commandhandler/nsBaseCommandController.cpp:140
    #29 0x7fcd18a62caa in nsCommandManager::DoCommand(char const*, nsICommandParams*, mozIDOMWindowProxy*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/embedding/components/commandhandler/nsCommandManager.cpp:212:10
    #30 0x7fcd15f1ec29 in nsHTMLDocument::ExecCommand(nsAString_internal const&, bool, nsAString_internal const&, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/html/nsHTMLDocument.cpp:3249:10
    #31 0x7fcd154dcc76 in mozilla::dom::HTMLDocumentBinding::execCommand(JSContext*, JS::Handle<JSObject*>, nsHTMLDocument*, JSJitMethodCallArgs const&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/HTMLDocumentBinding.cpp:816:15
    #32 0x7fcd15753157 in mozilla::dom::GenericBindingMethod(JSContext*, unsigned int, JS::Value*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/bindings/BindingUtils.cpp:2784:13
    #33 0x7fcd1b5d66c0 in CallJSNative /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jscntxtinlines.h:235:15
    #34 0x7fcd1b5d66c0 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:452
    #35 0x7fcd1b5bd8c0 in CallFromStack /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:503:12
    #36 0x7fcd1b5bd8c0 in Interpret(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:2873
    #37 0x7fcd1b5a3858 in js::RunScript(JSContext*, js::RunState&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:398:12
    #38 0x7fcd1b5d6d88 in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:470:15
    #39 0x7fcd1b5d7461 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/vm/Interpreter.cpp:516:10
    #40 0x7fcd1b1211a8 in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/js/src/jsapi.cpp:2890:12
    #41 0x7fcd153a9c6d in mozilla::dom::Function::Call(JSContext*, JS::Handle<JS::Value>, nsTArray<JS::Value> const&, JS::MutableHandle<JS::Value>, mozilla::ErrorResult&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dom/bindings/FunctionBinding.cpp:36:8
    #42 0x7fcd135d07bb in Call<nsCOMPtr<nsISupports> > /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/dom/FunctionBinding.h:70:12
    #43 0x7fcd135d07bb in nsGlobalWindow::RunTimeoutHandler(nsTimeout*, nsIScriptContext*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsGlobalWindow.cpp:12271
    #44 0x7fcd135ae64c in nsGlobalWindow::RunTimeout(nsTimeout*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsGlobalWindow.cpp:12520:32
    #45 0x7fcd13546d61 in nsGlobalWindow::TimerCallback(nsITimer*, void*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsGlobalWindow.cpp:12766:3
    #46 0x7fcd10c4fdf7 in nsTimerImpl::Fire() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsTimerImpl.cpp:524:7
    #47 0x7fcd10c270fc in nsTimerEvent::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/TimerThread.cpp:286:3
    #48 0x7fcd10c33eb8 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1067:7
    #49 0x7fcd10cb26bc in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290:10
    #50 0x7fcd11a17eff in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:100:21
    #51 0x7fcd119862f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235:3
    #52 0x7fcd119862f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #53 0x7fcd119862f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #54 0x7fcd1737915f in nsBaseAppShell::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/widget/nsBaseAppShell.cpp:156:3
    #55 0x7fcd194050a7 in XRE_RunAppShell /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:834:12
    #56 0x7fcd119862f8 in RunInternal /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:235:3
    #57 0x7fcd119862f8 in RunHandler /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:228
    #58 0x7fcd119862f8 in MessageLoop::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/chromium/src/base/message_loop.cc:208
    #59 0x7fcd19404712 in XRE_InitChildProcess /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/toolkit/xre/nsEmbedFunctions.cpp:664:7
    #60 0x4e2bc5 in content_process_main(int, char**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/app/../contentproc/plugin-container.cpp:224:19
    #61 0x7fcd0dd1282f in __libc_start_main /build/glibc-GKVZIf/glibc-2.23/csu/../csu/libc-start.c:291
    #62 0x41e778 in _start (/home/nils/fuzzer3/firefox/plugin-container+0x41e778)

0x6250002eab58 is located 6744 bytes inside of 8192-byte region [0x6250002e9100,0x6250002eb100)
allocated by thread T0 (Web Content) here:
    #0 0x4b51eb in __interceptor_malloc /builds/slave/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:52:3
    #1 0x7fcd1ef7bdd6 in PL_ArenaAllocate /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/nsprpub/lib/ds/plarena.c:210:27
    #2 0x7fcd179fbe99 in nsPresArena::Allocate(unsigned int, unsigned long) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsPresArena.cpp:165:3
    #3 0x7fcd17926c12 in AllocateByObjectID /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsPresArena.h:65:12
    #4 0x7fcd17926c12 in AllocateByObjectID /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsIPresShell.h:250
    #5 0x7fcd17926c12 in operator new /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsStyleStruct.h:2475
    #6 0x7fcd17926c12 in nsRuleNode::ComputeDisplayData(void*, nsRuleData const*, nsStyleContext*, nsRuleNode*, nsRuleNode::RuleDetail, mozilla::RuleNodeCacheConditions) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsRuleNode.cpp:5327
    #7 0x7fcd178fb8ce in nsRuleNode::WalkRuleTree(nsStyleStructID, nsStyleContext*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsRuleNode.cpp:2491:10
    #8 0x7fcd179eea5f in GetStyleDisplay<true> /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/layout/style/nsStyleStructList.h:98:1
    #9 0x7fcd179eea5f in nsStyleDisplay const* nsStyleContext::DoGetStyleDisplay<true>() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/layout/style/nsStyleStructList.h:98
    #10 0x7fcd1797723b in StyleDisplay /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/layout/style/nsStyleStructList.h:98:1
    #11 0x7fcd1797723b in nsStyleContext::SetStyleBits() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsStyleContext.cpp:687
    #12 0x7fcd17976fc6 in FinishConstruction /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsStyleContext.cpp:163:3
    #13 0x7fcd17976fc6 in nsStyleContext::nsStyleContext(nsStyleContext*, nsIAtom*, mozilla::CSSPseudoElementType, already_AddRefed<nsRuleNode>, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsStyleContext.cpp:121
    #14 0x7fcd1799a02c in NS_NewStyleContext(nsStyleContext*, nsIAtom*, mozilla::CSSPseudoElementType, nsRuleNode*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsStyleContext.cpp:1316:5
    #15 0x7fcd179a57ee in nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, mozilla::CSSPseudoElementType, mozilla::dom::Element*, unsigned int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsStyleSet.cpp:918:14
    #16 0x7fcd179aa6f3 in nsStyleSet::ResolveStyleFor(mozilla::dom::Element*, nsStyleContext*, TreeMatchContext&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/style/nsStyleSet.cpp:1366:10
    #17 0x7fcd17b1d612 in ResolveStyleFor /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/obj-firefox/dist/include/mozilla/StyleSetHandleInlines.h:93:3
    #18 0x7fcd17b1d612 in nsCSSFrameConstructor::ResolveStyleContext(nsStyleContext*, nsIContent*, nsFrameConstructorState*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:4959
    #19 0x7fcd17b19630 in ResolveStyleContext /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:4928:10
    #20 0x7fcd17b19630 in ResolveStyleContext /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:4944
    #21 0x7fcd17b19630 in nsCSSFrameConstructor::AddFrameConstructionItems(nsFrameConstructorState&, nsIContent*, bool, nsCSSFrameConstructor::InsertionPoint const&, nsCSSFrameConstructor::FrameConstructionItemList&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:5574
    #22 0x7fcd17b00233 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:10685:9
    #23 0x7fcd17b1712f in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:3998:9
    #24 0x7fcd17b2284c in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:6085:3
    #25 0x7fcd17b0f272 in ConstructFramesFromItemList /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:10502:5
    #26 0x7fcd17b0f272 in nsCSSFrameConstructor::CreateAnonymousFrames(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, PendingBinding*, nsFrameItems&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:4132
    #27 0x7fcd17b0b8b7 in nsCSSFrameConstructor::BeginBuildingScrollFrame(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, nsIAtom*, bool, nsContainerFrame*&) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:4546:3
    #28 0x7fcd17b083d5 in nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:2875:25
    #29 0x7fcd17b04852 in nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:2411:3
    #30 0x7fcd17b28a17 in nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsCSSFrameConstructor.cpp:7635:7
    #31 0x7fcd17cc31a7 in PresShell::Initialize(int, int) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/base/nsPresShell.cpp:1728:7
    #32 0x7fcd137f38ef in nsContentSink::StartLayout(bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/dom/base/nsContentSink.cpp:1210:19
    #33 0x7fcd12c02086 in nsHtml5TreeOpExecutor::StartLayout() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:612:3
    #34 0x7fcd12c0e83e in nsHtml5TreeOperation::Perform(nsHtml5TreeOpExecutor*, nsIContent**) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/parser/html/nsHtml5TreeOperation.cpp:990:7
    #35 0x7fcd12bff9d7 in nsHtml5TreeOpExecutor::RunFlushLoop() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/parser/html/nsHtml5TreeOpExecutor.cpp:448:21
    #36 0x7fcd12c044cb in nsHtml5ExecutorFlusher::Run() /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/parser/html/nsHtml5StreamParser.cpp:128:9
    #37 0x7fcd10c33eb8 in nsThread::ProcessNextEvent(bool, bool*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/threads/nsThread.cpp:1067:7
    #38 0x7fcd10cb26bc in NS_ProcessNextEvent(nsIThread*, bool) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/xpcom/glue/nsThreadUtils.cpp:290:10
    #39 0x7fcd11a17eff in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/ipc/glue/MessagePump.cpp:100:21

SUMMARY: AddressSanitizer: use-after-poison /builds/slave/m-cen-l64-asan-ntly-0000000000/build/src/layout/generic/nsIFrame.h:1565:46 in GetStateBits
Shadow bytes around the buggy address:
  0x0c4a80055510: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80055520: 00 00 00 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a80055530: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a80055540: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a80055550: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
=>0x0c4a80055560: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7[f7]f7 f7 f7 f7
  0x0c4a80055570: f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7 f7
  0x0c4a80055580: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a80055590: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a800555a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c4a800555b0: 00 00 00 00 00 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
==30757==ABORTING
Also crashes opt builds: bp-70a68fd7-cca6-48eb-8a61-72c152160627

I reproduced the opt crash in Release and Beta, but not in ESR-45. It's possible the UAF is still there lurking in older builds -- we won't know until we find out what's actually wrong here. CC'ing jwatt and heycam because they've touched the crashing file, but the cause of the problem might be elsewhere.
Group: core-security → layout-core-security
Crash Signature: [@ nsFrameManager::CaptureFrameState ]
status-firefox47: --- → affected
status-firefox48: --- → affected
status-firefox49: --- → affected
status-firefox-esr45: --- → unaffected
tracking-firefox48: --- → +
tracking-firefox49: --- → +
Keywords: crash, csectype-uaf, regression, regressionwindow-wanted, sec-high, testcase
Last good revision: 43f67d61062ac9e7d4202ef49a89d30f18583f84
First bad revision: c5153ecb53ba4c2a484a17f93d701881f280bfc8
Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=43f67d61062ac9e7d4202ef49a89d30f18583f84&tochange=c5153ecb53ba4c2a484a17f93d701881f280bfc8

So caused by either bug 1242872 or bug 1239945.
tracking-firefox47: --- → ?
tracking-firefox50: --- → ?
Flags: in-testsuite?
Keywords: regressionwindow-wanted
Thank you, Ryan.  I confirmed the culprit changeset is;
https://hg.mozilla.org/integration/mozilla-inbound/rev/c920a4fb4664
Assignee: nobody → hiikezoe
Status: NEW → ASSIGNED
This seems to be a case that we need to call RequestRestyle(Layer) even if there is no animation properties changes.  The trigger line which we need to call RequestRestyle(Layer) if there is no animation properties changes is 
 o577.style.position='absolute';

Before changeset c920a4fb4664, we had called RequestRestyle(Layer) (moreover we called SchedulePaint) whenever we call UpdateAnimations with old animations,  but after the changeset, we don't call RequestRestyle(Layer) if there is no animation properties changes.  As a result, nsCSSFrameConstructor::RecreateFramesForContent is not called when needed.

Yes, this bug, of course, is caused by web-animation.  You can confirm it if you change 'o577.id='id2' to 'o577.animate({ opacity: [0, 1] }, 100)'.
Blocks: 1242872
Component: Layout → DOM: Animation
Here is a test case for web-animations.
It seems that the trigger is this changeset, especially GetAnimationRule call.
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab454c4469b7
I am looking into it.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> This seems to be a case that we need to call RequestRestyle(Layer) even if
> there is no animation properties changes.  The trigger line which we need to
> call RequestRestyle(Layer) if there is no animation properties changes is 
>  o577.style.position='absolute';

I'm curious why we need to call RequestRestyle in this case?
(In reply to Brian Birtles (:birtles) from comment #6)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #4)
> > This seems to be a case that we need to call RequestRestyle(Layer) even if
> > there is no animation properties changes.  The trigger line which we need to
> > call RequestRestyle(Layer) if there is no animation properties changes is 
> >  o577.style.position='absolute';
> 
> I'm curious why we need to call RequestRestyle in this case?

I don't quite understand it yet.  I just knew before that changeset, we called RequestRestyle(Layer), and this bug does not happen.  So, I might be wrong.

Anyway, I think reframing process (I am not sure this word is right here) without RequestRestyle(Layer) does not invoked when needed in failure cases.
On debug build the test case causes an assertion:

Assertion failure: aListID == kPrincipalList || aListID == kNoReflowPrincipalList (unexpected child list), at /home/ikezoe/mozilla-central/layout/generic/nsContainerFrame.cpp:150
http://hg.mozilla.org/mozilla-central/file/c9a70b64f2fa/layout/generic/nsContainerFrame.cpp#l149

When I checked the listID of the parent frame;

0x00007fa7ed7ab43c in nsContainerFrame::RemoveFrame (this=<optimized out>, aListID=<optimized out>, aOldFrame=0x7fa7bf74da68) at /home/ikezoe/mozilla-central/layout/generic/nsContainerFrame.cpp:149
149	  MOZ_ASSERT(aListID == kPrincipalList || aListID == kNoReflowPrincipalList,
(rr) up
#1  0x00007fa7ed7213b1 in nsFrameManager::RemoveFrame (this=this@entry=0x7fa7c8f77cc0, aListID=mozilla::layout::kAbsoluteList, aOldFrame=aOldFrame@entry=0x7fa7bf74da68)
    at /home/ikezoe/mozilla-central/layout/base/nsFrameManager.cpp:510
510	    parentFrame->RemoveFrame(aListID, aOldFrame);
(rr) p aListID
$1 = mozilla::layout::kAbsoluteList
(rr) up
#2  0x00007fa7ed7fcd4e in nsPlaceholderFrame::DestroyFrom (this=0x7fa7beb3d6c0, aDestructRoot=0x7fa7c85f4480) at /home/ikezoe/mozilla-central/layout/generic/nsPlaceholderFrame.cpp:168
168	      fm->RemoveFrame(listId, oof);
(rr) p oof->mParent->HasAbsolutelyPositionedChildren()
$2 = true
(rr) p oof->mParent->GetAbsoluteContainingBlock()->mChildListID
$3 = mozilla::layout::kFixedList

This frame's child id was actually kFixedList but nsLayoutUtils::GetChildListNameFor() returned kAbsoluteList.

A problem in this failure case in nsLayoutUtils::GetChildListNameFor is StyleDisplay() against the frame.
http://hg.mozilla.org/mozilla-central/file/c9a70b64f2fa/layout/base/nsLayoutUtils.cpp#l1413

The disp->mPosition was NS_STYLE_POSITION_ABSOLUTE, it did not match what the frame has child id itself.
I found the reason why calling RequestRestyle(Layer) does not cause this issue.  
In restyling process between setTimeout() and 'o0.designMode='on'', CalcSyleDifference in CaptureChange returns NS_STYLE_HINT_NONE instead of nsChangeHint_ReconstructFrame, as a result, reframing is skipped, then the child id mismatch in comment 8 happens.

[1] http://hg.mozilla.org/mozilla-central/file/9b428173a088/layout/base/RestyleManager.cpp#l2780

The reason why CalcSyleDifference returns NS_STYLE_HINT_NONE is that PeekStyleDisplay() for the old style context is nullptr there.  Actually the difference should be from position:fixed to position:absolute there (i.e.,  nsChangeHint_ReconstructFrame).  I don't know why the display is null yet.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> The reason why CalcSyleDifference returns NS_STYLE_HINT_NONE is that
> PeekStyleDisplay() for the old style context is nullptr there.  Actually the
> difference should be from position:fixed to position:absolute there (i.e., 
> nsChangeHint_ReconstructFrame).  I don't know why the display is null yet.

The reason is that we are hit by this condition[1] in nsRuleNode::GetStyleData.
http://hg.mozilla.org/mozilla-central/file/70e05c6832e8/layout/style/nsRuleNode.cpp#l10264

We have an animation and a pseudo element 'o305=document.createTextNode("{}:first-line{")'.
I am going to investigate bug 1181011 which introduced this condition.
I have no idea what we can tweak nsRuleNode::GetStyleData for this bug cases.

So, this patch calls RequestRestyle to force to do layout when we have animations inside a pseudo element even if there is no property changes.

This is a behavior what we did before c920a4fb4664, so it's basically no risk, it has only a performance impact but the impact should be relatively small.

I'd like to hear opinions from Cameron, but he will be away until July 10.
Tracking 50+ for this crash.
tracking-firefox50: ? → +
Comment on attachment 8768286 [details] [diff] [review]
Force to call RequestRestyle when we have animations inside an pseudo element

In failure case, reparenting frame is not processed because RestyleManager::CaptureChange can not detect position style change from 'fixed' to 'absolute' due to nsStyleContext::CalcDifference in RestyleManager::CaptureChange does not return nsChangeHint_ReconstructFrame.  That's because because nsRuleNode::GetStyleData returns nullptr if the style has animation and is inside a pseudo-element.

Before this changeset, https://hg.mozilla.org/integration/mozilla-inbound/rev/c920a4fb4664, it did not appear since we called RequestRestyle(Layer), so reparenting frame was processed regardless of CaptureChange results.

This patch calls RequestRestyle(Layer) if we have animations inside a pseudo-element just like we did before, can be easily uplifted.

Cameron, what do you think of?
Attachment #8768286 - Flags: review?(cam)
Comment on attachment 8768286 [details] [diff] [review]
Force to call RequestRestyle when we have animations inside an pseudo element

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

(In reply to Hiroyuki Ikezoe (:hiro) from comment #9)
> In restyling process between setTimeout() and 'o0.designMode='on'',
> CalcSyleDifference in CaptureChange returns NS_STYLE_HINT_NONE instead of
> nsChangeHint_ReconstructFrame, as a result, reframing is skipped, then the
> child id mismatch in comment 8 happens.

When CalcStyleData calls PeekStyleData, it interprets a nullptr return value as meaning "we don't care about this struct", since we never requested it, and therefore don't need to generate any change hints for it.  However, it looks like the bug 1181011 changes violated that expectation.  Normally, when we compute a reset struct, and the rule node decides that it can't be cached, we store the struct pointer on the style context:

http://searchfox.org/mozilla-central/rev/bc94fc8653d9821caf87f5af0e2cd830a09ca8d3/layout/style/nsRuleNode.cpp#2843-2845

If the rule node does decide it can be cached, we avoid storing the struct pointer on the style context (since the style context can just ask the rule node for it, and we can maybe avoid allocating nsRuleNode::mResetData).

The problem that bug 1181011 was intended to address was that a single nsRuleNode, for an animation rule, can represent different style data, depending on whether the style context is for something inside a pseudo-element or not.  But the way we handle this is by still allowing the struct to be cached on the rule node (as long as its RuleNodeCacheConditions say it's OK), but then choosing not to return it from nsRuleNode::GetStyleData.  That means we don't store the struct pointer on the style context, which means PeekStyleData thinks we never computed the struct.

So I think we should fix this problem, rather than solve the crash by forcing the restyle in the animation code.  There are a couple of ways we could do it: we could always store reset struct pointers on the style context (even if cached unconditionally on the rule node) if we have animation data.  Or we could add an "in a pseudo" value to RuleNodeCacheConditions to be used if the rule node has animation data, like I was going to do in bug 1181011 comment 18 (though I'm not sure why that patch didn't work).

I think the former is the smaller change, so better for uplifting.  This is a small memory increase for style contexts with animations on them.

(The better solution would be to do bug 1181011 comment 25, which would use separate rule nodes for in-pseudo-animation-rule and not-in-pseudo-animation-rule, but that's a bigger change.)
Attachment #8768286 - Flags: review?(cam) → review-
Thank you, Cameron for detailed explanation!

(In reply to Cameron McCormack (:heycam) from comment #15)
> So I think we should fix this problem, rather than solve the crash by
> forcing the restyle in the animation code.  There are a couple of ways we
> could do it: we could always store reset struct pointers on the style
> context (even if cached unconditionally on the rule node) if we have
> animation data.

Initially I couldn't not understand what you mean, especially where the reset struct pointers stores.  After digging into the code, if I understand it correctly, I think you meant removing !(HasAnimationData() && ParentHasPseudoElementData(aContext)) in STYLE_STRUCT_RESET macro[1].

[1] http://hg.mozilla.org/mozilla-central/file/5fd14a66be31/layout/style/nsRuleNode.h#l943

Is this correct?
Flags: needinfo?(cam)
(In reply to Hiroyuki Ikezoe (:hiro) from comment #16)
> Thank you, Cameron for detailed explanation!
> 
> (In reply to Cameron McCormack (:heycam) from comment #15)
> Initially I couldn't not understand what you mean, especially where the
> reset struct pointers stores.  After digging into the code, if I understand
> it correctly, I think you meant removing !(HasAnimationData() &&
> ParentHasPseudoElementData(aContext)) in STYLE_STRUCT_RESET macro[1].
> 
> [1]
> http://hg.mozilla.org/mozilla-central/file/5fd14a66be31/layout/style/
> nsRuleNode.h#l943
> 
> Is this correct?

No, not removing this condition, since that would undo the fix from bug 1181011.  (It would mean that structs cached on the rule node get returned even if we have animation data in a pseudo-element, which was the problematic case.)

Here are all the places we currently cache a struct on the style context:

* http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsStyleContext.h#601-604
* http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsStyleContext.cpp#447-451

These are when we compute a new inherited struct, or get an existing one that was cached on the rule node.  We always cache inherited structs on the style context.

* http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsRuleNode.cpp#87-91

This is when we get an existing reset struct that was cached on the rule node with conditions.

* http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsRuleNode.cpp#2464-2473

This is when we are computing a new struct (inherited or reset), but we find we can just inherit the same struct from the parent style context.

* http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsRuleNode.cpp#2833-2841

This is when we just computed a new reset struct, which can be cached on the rule node with conditions.

* http://searchfox.org/mozilla-central/rev/92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsRuleNode.cpp#2843-2845

This is when we just computed a new reset struct, and it can't be cached on the rule node.


These last two are in COMPUTE_END_RESET, so just when we finished computing a new reset struct.  It's the first case in this macro -- when we have a reset struct that can be cached on the rule node without conditions -- that we want to adjust so that it caches the struct on the style context too, if HasAnimationData() is true.
Flags: needinfo?(cam)
(In reply to Cameron McCormack (:heycam) from comment #17)
> http://searchfox.org/mozilla-central/rev/
> 92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsRuleNode.cpp#2833-
> 2841
> 
> This is when we just computed a new reset struct, which can be cached on the
> rule node with conditions.
> 
> *
> http://searchfox.org/mozilla-central/rev/
> 92616e983b8ad6e99ec148f341e146c3c6fa312a/layout/style/nsRuleNode.cpp#2843-
> 2845
> 
> This is when we just computed a new reset struct, and it can't be cached on
> the rule node.
> 
> 
> These last two are in COMPUTE_END_RESET, so just when we finished computing
> a new reset struct.  It's the first case in this macro -- when we have a
> reset struct that can be cached on the rule node without conditions -- that
> we want to adjust so that it caches the struct on the style context too, if
> HasAnimationData() is true.

Hmm, then I really don't understand it.  In the test case in comment 0, as far as I can confirm, COMPUTE_END_RESET for Display against an old nsStyleContext passed to CalcStyleDifferenct is not called between setTimeout() and 'o0.designMode='on'', it's called after document.execCommand('insertparagraph',false,null), but it's too late.
Maybe I misunderstood the problem then.  Looking through the test case, I think the element in question -- o577, the <style> element -- should have its Display struct computed during the flush that happens inside the scrollByLines() call.  Is this what happens?  (I don't have a build handy to test right now.)
(In reply to Cameron McCormack (:heycam) from comment #19)
> Maybe I misunderstood the problem then.  Looking through the test case, I
> think the element in question -- o577, the <style> element -- should have
> its Display struct computed during the flush that happens inside the
> scrollByLines() call.  Is this what happens?  (I don't have a build handy to
> test right now.)

Yes, the style context at that time (the flush caused by scrollByLines) has a valid Display struct, but the style context is not the old style context passed to CalcStyleDifference.
On IRC, Cameron taught me a lot of things to debug this issue.  I've been tracking down this issue with a fix suggested in comment 17.  I am still in the middle way, but found some interesting facts.

* Two style contexts are created for the same nsIFrame during the flush caused by scrollByLines().
 * The first one is maybe created in somewhere inside nsCSSFrameConstructor::ProcessChildren. This might be wrong, I couldn't track down where it exactly comes from.
 * The second one is created in RestyleManager::ReparentStyleContext.
  http://hg.mozilla.org/mozilla-central/file/08f8a5aacd83/layout/base/RestyleManager.cpp#l2385
* Two style context has the same rule node.
 * I don't know it's normal or not.
* StyleDisplay() is called against both of the style context.
 1. StyleDisplay() is called against the first style context.
 2. It leads to call nsRuleNode::WalkRuleTree and then COMPUTE_END_RESET.
 3. Display struct is stored in mStyleData.mResetData and is set against the *first style context*
 4. The second style is generated.
 5. StyleDisplay() is called against the second style context.
 6. It leads to call nsRuleNode::WalkRuleTree but not call COMPUTE_END_RESET because ...
 7. the rule node has the previous Display struct so we can get startStruct here, then .. 
   http://hg.mozilla.org/mozilla-central/file/08f8a5aacd83/layout/style/nsRuleNode.cpp#l2343
 8. early return here
   http://hg.mozilla.org/mozilla-central/file/08f8a5aacd83/layout/style/nsRuleNode.cpp#l2431
* This second style context is a problematic style context.  Its rule node has Display struct in mStyleData.mResetData but the style context itself does not have the Display struct. So
* PeekStyleDisplay() against the second style context returns nullptr because the second style context does not have the Display struct in mCachedResetData and the style context has animations inside a pseudo element.

Now I am inclined to conclude this issue is not related bug 1181011. I think the issue has been in our code before bug 1181011.

Two questions;
1) Is it normal behavior that multiple style contexts have the same rule node?
2) If so, don't we clear mStyleData.mResetData of the rule node when RestyleManager::ReparentStyleContext is called?

I should investigate further, but anyway, thank you Cameron for instructing me.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> 
> Two questions;
> 1) Is it normal behavior that multiple style contexts have the same rule
> node?

This is normal case when reparenting style context.

> 2) If so, don't we clear mStyleData.mResetData of the rule node when
> RestyleManager::ReparentStyleContext is called?

I found an important comment.
http://hg.mozilla.org/mozilla-central/file/08f8a5aacd83/layout/base/RestyleManager.cpp#l2479

In this test case, position style is changed after reparenting context.  So a problem is inside position style code?
(In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> * StyleDisplay() is called against both of the style context.
>  1. StyleDisplay() is called against the first style context.
>  2. It leads to call nsRuleNode::WalkRuleTree and then COMPUTE_END_RESET.

This was wrong. StyleDisplay() is called against other style context here　and it leads to call nsRuleNode::WalkRuleTree against the same rule node of the first style context.  Maybe the style context for body element?  I am confused.
(In reply to Hiroyuki Ikezoe (:hiro) from comment #23)
> (In reply to Hiroyuki Ikezoe (:hiro) from comment #21)
> > * StyleDisplay() is called against both of the style context.
> >  1. StyleDisplay() is called against the first style context.
> >  2. It leads to call nsRuleNode::WalkRuleTree and then COMPUTE_END_RESET.
> 
> This was wrong. StyleDisplay() is called against other style context here
> and it leads to call nsRuleNode::WalkRuleTree against the same rule node of
> the first style context.  Maybe the style context for body element?  I am
> confused.

I am sorry for the confusion. The StyleDisplay() is actually called against a style context for an animated element (id=id2 in the test case) but not associated with the same nsIFrame of the other two style context.  I think this is the real first style context during ScrollByLines.
We were setting the Display struct to the rule node here. So calling nsStyleContext::SetStyle in COMPUTE_END_RESET is not effective unfortunately.
http://searchfox.org/mozilla-central/source/layout/style/nsRuleNode.cpp#1744
Calling nsStyleContext::SetStyle() (with AddStyleBit) inside if (HasAnimationData()) check in STYLE_STRUCT_RESET, as Cameron suggested on IRC, causes memory leak (the struct pointer I think).  I got an assertion here.
http://hg.mozilla.org/mozilla-central/file/711963e8daa3/layout/base/nsPresShell.cpp#l840

Calling nsStyleContext::SetStyle() without AddStyleBit causes another use-after-poison in nsStyleContext::StyleTextReset() in the test case comment 0.

So I tried to move the call to nsRuleNode::GetStyleData(), and it failed with the same use-after-poison.  And then I moved the call to the place just before returning the struct found in mResetData in nsRuleNode::WalkRuleTree(), it does not cause any errors as far as I confirmed.  This patch is it.

To confirm that it does not cause any side-effets I did a try with this change.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=25fb69e1a8e8

Of course our test cases might not cover failure cases, but I have no idea other than this.

Cameron, any thoughts?
Attachment #8768286 - Attachment is obsolete: true
Attachment #8771809 - Flags: feedback?(cam)
status-firefox47: affected → wontfix
Version: 50 Branch → 47 Branch
Attachment #8771809 - Attachment is patch: true
OK, so just to summarize my understanding, here are the codepaths under nsRuleNode::GetStyleData that nsStyleContext::StyleData can call into to get a non-inherited struct.

1. We have a non-inherited struct already stored on the rule node, and it's not the problematic case of a rule node with animation data and pseudo-element data.  We return the struct immediately, and don't set the bit.  nsStyleContext::GetStyleData doesn't cache the struct on the style context.

==> This case needs changing, since we want all non-inherited structs that were stored on the rule node to be cached on the style context if we have animation data.  (Though I suppose this is not the case that triggers the problem in this bug, as you've identified that one in comment 26.)  To handle this, we can do

  if (HasAnimationData()) {
    aContext->AddStyleBit(...);
    aContext->SetStyle(...);
  }

like you do in your comment 26 patch.

2. nsRuleNode::GetStyleData calls into WalkRuleTree, and we return early with a struct from higher up in the rule tree (because we know that our rule node doesn't add any relevant properties), which is the case your comment 26 patch takes care of.  This change looks right.

3. WalkRuleTree finds we have |inherit| specified for all properties in the struct, and we |return parentStruct|.  The existing code there should be OK for non-inherited structs too.

4. We are computing structs for the root.  This doesn't need special handling.

5. WalkRuleTree computes a new struct, and in COMPUTE_END_RESET we decide we can cache it without dependencies.

==> This case still needs changing, I think.  We need a block just like the comment 26 patch in here too.

6. WalkRuleTree computes a new struct, and in COMPUTE_END_RESET we decide we can cache it with dependencies.  The existing code here should be OK.

7. WalkRuleTree computes a new struct, and in COMPUTE_END_RESET we decide we can't cache it on the rule node.  The existing code here should be OK as we explicitly cache the struct on the style context.


So basically, any time we either look up an existing non-inherited struct cached on the rule node, or the rule node computes a new one that gets cached on the rule node too, and HasAnimationData() is true, we need to cache on the style context too.

It sounds like from your previous comment that the #5 change above was causing a leak, but I'm not sure why that is.  Can you show me the patch you had for that?  Also please try a patch that handles all three of these cases.  Thank you!
Thank you, Cameron!

https://hg.mozilla.org/try/rev/4c8a65db67fa7bf0289b229dc788c4e9cf4762de
Here is a try with all the three changes.
(In reply to Cameron McCormack (:heycam) from comment #27)

> It sounds like from your previous comment that the #5 change above was
> causing a leak, but I'm not sure why that is.  Can you show me the patch you
> had for that? 

Actually the change which causes a leak is the change in STYLE_STRUCT_RESET macro.
https://hg.mozilla.org/try/rev/4c8a65db67fa7bf0289b229dc788c4e9cf4762de#l2.14

Though, in the changeset of the try, it also has a null check of newData, it results the same leak without the null check.  I did also add aComputeData check, it didn't help either.
I think the nsStyleStruct.h change you have in your patch needs to go inside the if statement in nsRuleNode::GetStyleData.  With the current location of that change, we will end up recording some newly computed structs that nsRuleNode returned with the bit set, meaning we won't delete it when the style context goes away.
Ah, now I fully understand what you said.  The leak does not happen any more.

A new try.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=363dd896402e
Thank you, Cameron. The try result seems good.
Attachment #8771809 - Attachment is obsolete: true
Attachment #8771809 - Flags: feedback?(cam)
Attachment #8772596 - Flags: review?(cam)
Attachment #8772596 - Attachment is patch: true
Comment on attachment 8772596 [details] [diff] [review]
Store all non-inherited structs which are stored on the rule node on the style context if the we have animation data

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

Thanks, this is just about right.  A couple of things to address:

s/the we/we/ in the commit message.

::: layout/style/nsRuleNode.cpp
@@ +2428,5 @@
>      if (!isReset) {
>        aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
> +    } else if (HasAnimationData()) {
> +      // If we have animation data, the struct should be cached on the style
> +      // context so that we can peek the struct.

In this and the other comments, can you add "See comment in AnimValuesStyleRule::MapRuleInfoInto".

@@ +10284,5 @@
> +        // If we have animation data, the struct should be cached on the style
> +        // context so that we can peek the struct.
> +        aContext->AddStyleBit(nsCachedStyleData::GetBitForSID(aSID));
> +        aContext->SetStyle(aSID, const_cast<void*>(data));
> +      }

We need to add this change to the macro-defined nsRuleNode::GetStyle##name_ functions, too (which I forgot to mention).
Attachment #8772596 - Flags: review?(cam) → review-
> We need to add this change to the macro-defined nsRuleNode::GetStyle##name_ functions,
> too (which I forgot to mention).

(But only for the non-inherited struct ones defined with STYLE_STRUCT_RESET.)
Calling nsStyleContext's functions in nsRuleNode.h causes header inclusion hell.  I had to add a static function.
Attachment #8772596 - Attachment is obsolete: true
Attachment #8772662 - Flags: review?(cam)
Comment on attachment 8772662 [details] [diff] [review]
Store all non-inherited structs which are stored on the rule node on the style context if the we have animation data

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

r=me with this.  Thank you!

::: layout/style/nsRuleNode.h
@@ +1073,5 @@
>                                      nsTimingFunction& aResult);
>  
> +  // Store style struct on the style context and tell the style context
> +  // that it doesn't own the data
> +  static void StoreStyleOnContext(nsStyleContext* aContext,

Let's make this private, since we only need to call it from within nsRuleNode methods.
Attachment #8772662 - Flags: review?(cam) → review+
This is a regression from bug 1181011.
Blocks: 1181011
Made the static function private.  Carrying over review+.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=19308801b3c0

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
* Not easily. 

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?
* 45ESR.  The test case in comment 0 does not affect 45ESR, but I think there are other cases to cause this use-after-poison on 45ESR.  It will be more difficult to attack though.

If not all supported branches, which bug introduced the flaw?
* Bug 1181011.

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
* Yes.  This patch can be applied to 45ESR.

How likely is this patch to cause regressions; how much testing does it need?
* Not likely.  It slightly increases memory consumption on sites that have animations, but it's negligible.
Attachment #8772662 - Attachment is obsolete: true
Attachment #8772704 - Flags: sec-approval?
Attachment #8772704 - Flags: review+
I believe this is too late to make the current ESR45 or Firefox 48 releases. Sylvestre?
Flags: needinfo?(sledru)
The patch seems to touch important parts of the code: if we can wait, it would be better.
Flags: needinfo?(sledru)
Comment on attachment 8772704 [details] [diff] [review]
Store all non-inherited structs which are stored on the rule node on the style context if the we have animation data

sec-approval+ for trunk. Let's get an Aurora patch made and nominated as well (and in just after trunk).
Attachment #8772704 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2cfc74a93beafabdd057fc6e1b4582baa9bb078
Bug 1282076 - Store all non-inherited structs which are stored on the rule node on the style context if we have animation data. r=heycam
Comment on attachment 8772704 [details] [diff] [review]
Store all non-inherited structs which are stored on the rule node on the style context if the we have animation data

Approval Request Comment
[Feature/regressing bug #]: bug 1181011
[User impact if declined]: A security hole caused by use-after-poison.
[Describe test coverage new/current, TreeHerder]: Not included in this patch.  Two test cases (in comment 0 and comment 5) will land after this bug is in public.  I've confirmed that both of the test cases work fine locally.
[Risks and why]: Low, it essentially increases memory consumption, but it's negligible.
[String/UUID change made/needed]: None
Attachment #8772704 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f2cfc74a93be
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Attachment #8772704 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I assume from comment 38 and comment 39 this should be status-firefox48: wontfix?
Group: layout-core-security → core-security-release
Flags: sec-bounty?
status-firefox-esr45: unaffected → affected
tracking-firefox-esr45: --- → 49+
Flags: sec-bounty? → sec-bounty+
Is ESR45 really affected? This bug was introduced by bug 1242872 in Firefox 47 and not uplifted anywhere as far as I can see--but perhaps I'm misunderstanding something about our ESR releases.
Flags: needinfo?(dveditz)
(In reply to Brian Birtles (:birtles) from comment #47)
> Is ESR45 really affected? This bug was introduced by bug 1242872 in Firefox
> 47 and not uplifted anywhere as far as I can see--but perhaps I'm
> misunderstanding something about our ESR releases.

Nevermind, I see comment 37 now.
Flags: needinfo?(dveditz)
Hiro, can we have an uplift request to esr45? Thanks
Flags: needinfo?(hiikezoe)
Comment on attachment 8772704 [details] [diff] [review]
Store all non-inherited structs which are stored on the rule node on the style context if the we have animation data

[Approval Request Comment]
User impact if declined: A security hole caused by use-after-poison.
Fix Landed on Version: 49
Risk to taking this patch (and alternatives if risky):  Low, this patch has been landed on mozilla-central and 49 branch for a while and the patch slightly increases memory consumption.
String or UUID changes made by this patch: none
Flags: needinfo?(hiikezoe)
Attachment #8772704 - Flags: approval-mozilla-esr45?
Comment on attachment 8772704 [details] [diff] [review]
Store all non-inherited structs which are stored on the rule node on the style context if the we have animation data

Let's take it in esr45, should be in 45.4
Attachment #8772704 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+
Flags: qe-verify+
Reproduced the crash on FX 48.0.2.
Verified fixed FX 49.0, 50.0a2 (2016-09-05), 45.4.0 ESR on Win 7, Ubuntu 14.04, OS X 10.11.
Status: RESOLVED → VERIFIED
status-firefox49: fixed → verified
status-firefox50: fixed → verified
status-firefox-esr45: fixed → verified
Whiteboard: [adv-main49+][adv-esr45.4+]
Alias: CVE-2016-5274
Attached patch Crash tests — — Splinter Review 
Firefox 48 and ESR 45.4.0 have been released. Now I think it's time to land test cases.

This includes a test case in comment 0 and attachment 8767517 [details].  An interesting fact is that current mozilla-central does not cause this use-after-poison even if the fix (attachment 8772704 [details] [diff] [review]) is reverted but it's worth to add test cases.
Attachment #8793634 - Flags: review?(cam)
Comment on attachment 8793634 [details] [diff] [review]
Crash tests

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

> Firefox 48 and ESR 45.4.0 have been released.

(Firefox 49.)  r=me with a suitable commit message.
Attachment #8793634 - Flags: review?(cam) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd2b75b3e3ff85f25ffbe9166db4b1f0a93a8a09
Bug 1282076 - Crashtest that struct data is cached if we have animation data. r=heycam
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: