Tyson Smith [:tsmith] Reporter
	Description • 6 years ago

Marking s-s just in case.

Found with m-c: 20180613-c7a7df27ff38

I will upload the reduced testcase when it is complete.

==6102==ERROR: AddressSanitizer: use-after-poison on address 0x6250009ed4a8 at pc 0x7f8bb7eaac3f bp 0x7ffef180cd80 sp 0x7ffef180cd78
READ of size 8 at 0x6250009ed4a8 thread T0 (file:// Content)
    #0 0x7f8bb7eaac3e in get /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:296:27
    #1 0x7f8bb7eaac3e in operator mozilla::ComputedStyle * /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/RefPtr.h:309
    #2 0x7f8bb7eaac3e in Style /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:783
    #3 0x7f8bb7eaac3e in PresContext /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:628
    #4 0x7f8bb7eaac3e in mozilla::DisplayItemData::Destroy() /builds/worker/workspace/build/src/layout/painting/FrameLayerBuilder.h:137
    #5 0x7f8bb81d3116 in nsIFrame::RemoveDisplayItemDataForDeletion() /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:965:10
    #6 0x7f8bb7e20587 in mozilla::PresShell::NotifyDestroyingFrame(nsIFrame*) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:2111:11
    #7 0x7f8bb81405b3 in nsFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsFrame.cpp:790:10
    #8 0x7f8bb8377f87 in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsLineBox.cpp:403:14
    #9 0x7f8bb80d5f4c in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:327:3
    #10 0x7f8bb80d6f18 in DestroyFramesFrom /builds/worker/workspace/build/src/layout/generic/nsFrameList.cpp:59:12
    #11 0x7f8bb80d6f18 in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:230
    #12 0x7f8bb8377f87 in nsLineBox::DeleteLineList(nsPresContext*, nsLineList&, nsIFrame*, nsFrameList*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsLineBox.cpp:403:14
    #13 0x7f8bb80d5f4c in nsBlockFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsBlockFrame.cpp:327:3
    #14 0x7f8bb80d6f18 in DestroyFramesFrom /builds/worker/workspace/build/src/layout/generic/nsFrameList.cpp:59:12
    #15 0x7f8bb80d6f18 in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:230
    #16 0x7f8bb8159170 in nsCanvasFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsCanvasFrame.cpp:167:21
    #17 0x7f8bb80d6f18 in DestroyFramesFrom /builds/worker/workspace/build/src/layout/generic/nsFrameList.cpp:59:12
    #18 0x7f8bb80d6f18 in nsContainerFrame::DestroyFrom(nsIFrame*, mozilla::layout::PostFrameDestroyData&) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:230
    #19 0x7f8bb8176c67 in Destroy /builds/worker/workspace/build/src/layout/generic/nsIFrame.h:679:5
    #20 0x7f8bb8176c67 in nsContainerFrame::RemoveFrame(mozilla::layout::FrameChildListID, nsIFrame*) /builds/worker/workspace/build/src/layout/generic/nsContainerFrame.cpp:179
    #21 0x7f8bb7f15a87 in RemoveFrame /builds/worker/workspace/build/src/layout/base/nsFrameManager.cpp:126:18
    #22 0x7f8bb7f15a87 in nsCSSFrameConstructor::ContentRemoved(nsIContent*, nsIContent*, nsCSSFrameConstructor::RemoveFlags) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:8115
    #23 0x7f8bb7efe63c in nsCSSFrameConstructor::RecreateFramesForContent(nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:9178:5
    #24 0x7f8bb7e7f4fb in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:1512:25
    #25 0x7f8bb7e8fd43 in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:2997:9
    #26 0x7f8bb7e35ebd in ProcessPendingRestyles /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3074:3
    #27 0x7f8bb7e35ebd in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4285
    #28 0x7f8bb1e358c9 in FlushPendingNotifications /builds/worker/workspace/build/src/layout/base/nsIPresShell.h:575:5
    #29 0x7f8bb1e358c9 in nsIDocument::FlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7495
    #30 0x7f8bb1ba22d8 in mozilla::dom::Element::GetBoundingClientRect() /builds/worker/workspace/build/src/dom/base/Element.cpp:2276:10
    #31 0x7f8bb431ca5a in mozilla::dom::ElementBinding::getBoundingClientRect(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Element*, JSJitMethodCallArgs const&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/ElementBinding.cpp:2812:59
    #32 0x7f8bb4bacac9 in bool mozilla::dom::binding_detail::GenericMethod<mozilla::dom::binding_detail::NormalThisPolicy, mozilla::dom::binding_detail::ThrowExceptions>(JSContext*, unsigned int, JS::Value*) /builds/worker/workspace/build/src/dom/bindings/BindingUtils.cpp:3285:13
    #33 0x7f8bbc39ebeb in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/JSContext-inl.h:274:15
    #34 0x7f8bbc383c79 in CallFromStack /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:526:12
    #35 0x7f8bbc383c79 in Interpret(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:3122
    #36 0x7f8bbc36f8fa in js::RunScript(JSContext*, js::RunState&) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:421:12
    #37 0x7f8bbc39f38d in js::InternalCallOrConstruct(JSContext*, JS::CallArgs const&, js::MaybeConstruct) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:493:15
    #38 0x7f8bbc3a01d2 in js::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, js::AnyInvokeArgs const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/vm/Interpreter.cpp:539:10
    #39 0x7f8bbcee96aa in JS::Call(JSContext*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::HandleValueArray const&, JS::MutableHandle<JS::Value>) /builds/worker/workspace/build/src/js/src/jsapi.cpp:2965:12
    #40 0x7f8bb3a83dcf in mozilla::dom::IdleRequestCallback::Call(JSContext*, JS::Handle<JS::Value>, mozilla::dom::IdleDeadline&, mozilla::ErrorResult&) /builds/worker/workspace/build/src/obj-firefox/dom/bindings/WindowBinding.cpp:863:8
    #41 0x7f8bb1c08c38 in Call /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/WindowBinding.h:720:12
    #42 0x7f8bb1c08c38 in mozilla::dom::IdleRequest::IdleRun(nsPIDOMWindowInner*, double, bool) /builds/worker/workspace/build/src/dom/base/IdleRequest.cpp:74
    #43 0x7f8bb19e9aba in nsGlobalWindowInner::RunIdleRequest(mozilla::dom::IdleRequest*, double, bool) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:698:19
    #44 0x7f8bb19e74cb in nsGlobalWindowInner::ExecuteIdleRequest(mozilla::TimeStamp) /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp:728:21
    #45 0x7f8badf8217a in nsThread::ProcessNextEvent(bool, bool*) /builds/worker/workspace/build/src/xpcom/threads/nsThread.cpp:1088:14
    #46 0x7f8badfa78d1 in NS_ProcessNextEvent(nsIThread*, bool) /builds/worker/workspace/build/src/xpcom/threads/nsThreadUtils.cpp:519:10
    #47 0x7f8baf11b3be in mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) /builds/worker/workspace/build/src/ipc/glue/MessagePump.cpp:97:21
    #48 0x7f8baf02094c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #49 0x7f8baf02094c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #50 0x7f8baf02094c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #51 0x7f8bb76e7116 in nsBaseAppShell::Run() /builds/worker/workspace/build/src/widget/nsBaseAppShell.cpp:157:27
    #52 0x7f8bbc08b90e in XRE_RunAppShell() /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:896:22
    #53 0x7f8baf02094c in RunInternal /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:325:10
    #54 0x7f8baf02094c in RunHandler /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:318
    #55 0x7f8baf02094c in MessageLoop::Run() /builds/worker/workspace/build/src/ipc/chromium/src/base/message_loop.cc:298
    #56 0x7f8bbc08aacc in XRE_InitChildProcess(int, char**, XREChildData const*) /builds/worker/workspace/build/src/toolkit/xre/nsEmbedFunctions.cpp:722:34
    #57 0x4f54c1 in content_process_main /builds/worker/workspace/build/src/browser/app/../../ipc/contentproc/plugin-container.cpp:50:30
    #58 0x4f54c1 in main /builds/worker/workspace/build/src/browser/app/nsBrowserApp.cpp:287
    #59 0x7f8bd190f82f in __libc_start_main /build/glibc-Cl5G7W/glibc-2.23/csu/../csu/libc-start.c:291
    #60 0x4248bc in _start (/home/ubuntu/firefox/firefox+0x4248bc)

0x6250009ed4a8 is located 5032 bytes inside of 8192-byte region [0x6250009ec100,0x6250009ee100)
allocated by thread T0 (file:// Content) here:
    #0 0x4c5003 in malloc /builds/worker/workspace/moz-toolchain/src/llvm/projects/compiler-rt/lib/asan/asan_malloc_linux.cc:88:3
    #1 0x7f8badf18263 in AllocateChunk /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:193:15
    #2 0x7f8badf18263 in InternalAllocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:228
    #3 0x7f8badf18263 in Allocate /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:75
    #4 0x7f8badf18263 in mozilla::ArenaAllocator<8192ul, 8ul>::Allocate(unsigned long) /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/ArenaAllocator.h:80
    #5 0x7f8bb86fda7a in AllocateByFrameID /builds/worker/workspace/build/src/layout/base/nsPresArena.h:39:12
    #6 0x7f8bb86fda7a in AllocateFrame /builds/worker/workspace/build/src/layout/base/nsIPresShell.h:206
    #7 0x7f8bb86fda7a in operator new /builds/worker/workspace/build/src/layout/xul/nsBoxFrame.cpp:92
    #8 0x7f8bb86fda7a in NS_NewBoxFrame(nsIPresShell*, mozilla::ComputedStyle*) /builds/worker/workspace/build/src/layout/xul/nsBoxFrame.cpp:89
    #9 0x7f8bb7eeeca3 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:3859:7
    #10 0x7f8bb7efceaa in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6047:3
    #11 0x7f8bb7ed7fde in ConstructFramesFromItemList /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10124:5
    #12 0x7f8bb7ed7fde in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10317
    #13 0x7f8bb7ef0a51 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4037:9
    #14 0x7f8bb7efceaa in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6047:3
    #15 0x7f8bb7ed7fde in ConstructFramesFromItemList /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10124:5
    #16 0x7f8bb7ed7fde in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, mozilla::ComputedStyle*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10317
    #17 0x7f8bb7ee260f in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, mozilla::ComputedStyle*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11245:3
    #18 0x7f8bb7ef4857 in ConstructScrollableBlockWithConstructor /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4872:3
    #19 0x7f8bb7ef4857 in nsCSSFrameConstructor::ConstructScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4839
    #20 0x7f8bb7eeee13 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:3853:7
    #21 0x7f8bb7efceaa in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6047:3
    #22 0x7f8bb7ed6e7a in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, bool, nsFrameItems&) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10124:5
    #23 0x7f8bb7f0e557 in nsCSSFrameConstructor::ContentAppended(nsIContent*, nsCSSFrameConstructor::InsertionKind) /builds/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:7247:3
    #24 0x7f8bb7e7f297 in mozilla::RestyleManager::ProcessRestyledFrames(nsStyleChangeList&) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:1404:27
    #25 0x7f8bb7e8fd43 in mozilla::RestyleManager::DoProcessPendingRestyles(mozilla::ServoTraversalFlags) /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:2997:9
    #26 0x7f8bb7e35ebd in ProcessPendingRestyles /builds/worker/workspace/build/src/layout/base/RestyleManager.cpp:3074:3
    #27 0x7f8bb7e35ebd in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4285
    #28 0x7f8bb1e358c9 in FlushPendingNotifications /builds/worker/workspace/build/src/layout/base/nsIPresShell.h:575:5
    #29 0x7f8bb1e358c9 in nsIDocument::FlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/dom/base/nsDocument.cpp:7495
    #30 0x7f8bb6f98b44 in nsSMILAnimationController::DoSample(bool) /builds/worker/workspace/build/src/dom/smil/nsSMILAnimationController.cpp:439:15
    #31 0x7f8bb7e35b41 in Resample /builds/worker/workspace/build/src/obj-firefox/dist/include/nsSMILAnimationController.h:73:21
    #32 0x7f8bb7e35b41 in FlushResampleRequests /builds/worker/workspace/build/src/obj-firefox/dist/include/nsSMILAnimationController.h:89
    #33 0x7f8bb7e35b41 in mozilla::PresShell::DoFlushPendingNotifications(mozilla::ChangesToFlush) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:4271
    #34 0x7f8bb5332278 in FlushPendingNotifications /builds/worker/workspace/build/src/obj-firefox/dist/include/nsIPresShell.h:566:5
    #35 0x7f8bb5332278 in FlushPendingEvents /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:5512
    #36 0x7f8bb5332278 in mozilla::EventStateManager::PreHandleEvent(nsPresContext*, mozilla::WidgetEvent*, nsIFrame*, nsIContent*, nsEventStatus*, nsIContent*) /builds/worker/workspace/build/src/dom/events/EventStateManager.cpp:687
    #37 0x7f8bb7e6763d in mozilla::PresShell::HandleEventInternal(mozilla::WidgetEvent*, nsEventStatus*, bool, nsIContent*) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:7605:19
    #38 0x7f8bb7e62587 in mozilla::PresShell::HandleEvent(nsIFrame*, mozilla::WidgetGUIEvent*, bool, nsEventStatus*) /builds/worker/workspace/build/src/layout/base/PresShell.cpp:7250:17
    #39 0x7f8bb75fa3a1 in nsViewManager::DispatchEvent(mozilla::WidgetGUIEvent*, nsView*, nsEventStatus*) /builds/worker/workspace/build/src/view/nsViewManager.cpp:812:14
    #40 0x7f8bb75f9b76 in nsView::HandleEvent(mozilla::WidgetGUIEvent*, bool) /builds/worker/workspace/build/src/view/nsView.cpp:1141:9
    #41 0x7f8bb76981b5 in mozilla::widget::PuppetWidget::DispatchEvent(mozilla::WidgetGUIEvent*, nsEventStatus&) /builds/worker/workspace/build/src/widget/PuppetWidget.cpp:410:35
    #42 0x7f8bb106a99a in mozilla::layers::APZCCallbackHelper::DispatchWidgetEvent(mozilla::WidgetGUIEvent&) /builds/worker/workspace/build/src/gfx/layers/apz/util/APZCCallbackHelper.cpp:500:21
    #43 0x7f8bb6e1af17 in DispatchWidgetEventViaAPZ /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1799:10
    #44 0x7f8bb6e1af17 in mozilla::dom::TabChild::HandleRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1739
    #45 0x7f8bb6e1c2f6 in mozilla::dom::TabChild::RecvRealMouseButtonEvent(mozilla::WidgetMouseEvent const&, mozilla::layers::ScrollableLayerGuid const&, unsigned long const&) /builds/worker/workspace/build/src/dom/ipc/TabChild.cpp:1706:3

	Matt Woodrow (:mattwoodrow)
	Comment 2 • 6 years ago

I think this is an existing bug (unrelated to the other new DisplayItemData crashes), but is still interesting. I think Jamie will look at this once he's done with the other issues.

	Jamie Nicol [:jnicol] Assignee
	Comment 3 • 6 years ago

This is my understanding of the crash, from stepping through with rr:

We have display items A and B, with frames 1 and 2. Item A points to our DisplayItemData (holding a reference), which has frame 1 in mFrameList.

We then merge items A and B, creating item C.

We then call DisplayItemData::BeginUpdate() with item C. This adds frame 2 to mFrameList. Then we destroy the temporary item C.

Then we call BeginUpdate() with item B. Frame 2 remains in mFrameList, but frame 1 is removed.

Frame 2 is then destroyed, but there is a dangling pointer to it in mFrameList.

DisplayItem A is then destroyed, dropping the final reference to the DisplayItemData. It dereferences the dangling Frame 2 pointer, in order to grab the PresContext to free itself.

	Jamie Nicol [:jnicol] Assignee
	Comment 4 • 6 years ago

So on the first paint, items A and B are consecutive in the display list, so they get merged. A->mDisplayItemData == our display item data, B->mDisplayItemData == nullptr. The DisplayItemData gets added to both frames, so the FrameLayerBuilder::GetDisplayItemData lookup will find it for both of them.

On the next paint, there is another different type of display item in between them, so they are not merged. in FinishPaintedLayerData we iterate through mAssignedDisplayItems, which has separate entries for items B and A, both with the same DisplayItemData pointer. We call AddPaintedDisplayItem for each entry, so this calls BeginUpdate for item B, then for item A skips calling BeginUpdate, because the DisplayItemData has just been marked as used. Frame 2 is removed from the DisplayItemData, but item A still points to the DisplayItemData. I think that is the problem, if we unset the pointer at that time (or set it to a new DisplayItemData, because presumably we do want to paint this thing), the DisplayItemData will be destructed with Frame 2 rather than after it.

	Jamie Nicol [:jnicol] Assignee
	Comment 5 • 6 years ago

That should say "Frame *1* is removed from the DisplayItemData..."

	Jamie Nicol [:jnicol] Assignee
	Comment 7 • 6 years ago

Comment on attachment 8988168 [details] [diff] [review]
0001-Bug-1468738-Create-new-DisplayItemDatas-if-required-.patch

[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?

I think all. Bug 1436904 (which landed in 60) made it so that a DisplayItemData can outlive its Frame, by holding a reference in the nsDisplayItem.

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

Bug 1436904

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

Patch should apply cleanly, low risk.

How likely is this patch to cause regressions; how much testing does it need?

The patch is fairly simple but DisplayItemData lifetimes are quite scary

	Al Billings [:abillings - ex-MoCo]
	Comment 8 • 6 years ago

As a sec-low, this doesn't need sec-approval to land on trunk.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 9 • 6 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1683f72dac4366bbe7d07f80464aee31211d0e3
https://hg.mozilla.org/mozilla-central/rev/c1683f72dac4

	Ryan VanderMeulen [:RyanVM]
	Comment 10 • 6 years ago

Please request Beta/ESR60 approval on this when you get a chance.

	Jamie Nicol [:jnicol] Assignee
	Comment 11 • 6 years ago

Comment on attachment 8988168 [details] [diff] [review]
0001-Bug-1468738-Create-new-DisplayItemDatas-if-required-.patch

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1436904
[User impact if declined]: Crashes, potentially exploitable
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: Yes
[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?]: Slightly
[Why is the change risky/not risky?]: DisplayItemData lifetimes aren't completely understood, but this definitely fixes a problem with them, and seems to be good on nightly.
[String changes made/needed]: None

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This is only marked as sec low, but could be exploited and there seem to be crashes in the wild.
User impact if declined: Crashes, potentially exploitable
Fix Landed on Version: 63
Risk to taking this patch (and alternatives if risky): Risk of tinkering with DisplayItemData lifetimes, which are a cause of other crashes and aren't fully understood. But I think this patch is good, and it's the simplest and least risky fix for this crash.
String or UUID changes made by this patch: None

	Liz Henry (:lizzard) (relman/hg->git project)
	Comment 12 • 6 years ago

Comment on attachment 8988168 [details] [diff] [review]
0001-Bug-1468738-Create-new-DisplayItemDatas-if-required-.patch

Crash fix, may be a bit risky but it's been solid on Nightly for a week now. Let's take this for beta 8.   Not sure if this is a good idea for ESR uplift, though.

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 13 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/9f52abcf637e

	Ryan VanderMeulen [:RyanVM]
	Comment 14 • 6 years ago

Comment on attachment 8988168 [details] [diff] [review]
0001-Bug-1468738-Create-new-DisplayItemDatas-if-required-.patch

If this was later in the ESR cycle, I'd probably be more inclined to wontfix. Given that we're still early on, let's take it to fix the exploitable crashes. Approved for ESR 60.2.

	Ryan VanderMeulen [:RyanVM]
	Comment 15 • 6 years ago
uplift

https://hg.mozilla.org/releases/mozilla-esr60/rev/caa39737edde

	Cristian Fogel [:cfogel]
	Comment 16 • 6 years ago

Managed to reproduce using 62.0a1 (2018-06-03).
Have checked using the provided test case file with 60.1.1esr, 63.0a1 (2018-07-30), 62.0b13 on Windows10x64, macOS 10.13.4, Ubuntu  16.04LTS and can confirm the issue is no longer reproducible.