1348503 - Crash in mozilla::FrameLayerBuilder::WillEndTransaction

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[baffclan]

This bug was filed from the Socorro interface and is 
report bp-336c6eab-5049-404b-9458-802832170318.
=============================================================


Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::FrameLayerBuilder::WillEndTransaction() 	layout/painting/FrameLayerBuilder.cpp:2011
1 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) 	layout/painting/nsDisplayList.cpp:2213
2 	xul.dll 	nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) 	layout/base/nsLayoutUtils.cpp:3756
3 	xul.dll 	mozilla::PresShell::Paint(nsView*, nsRegion const&, unsigned int) 	layout/base/PresShell.cpp:6495
4 	xul.dll 	nsViewManager::ProcessPendingUpdatesPaint(nsIWidget*) 	view/nsViewManager.cpp:483
5 	xul.dll 	nsViewManager::ProcessPendingUpdatesForView(nsView*, bool) 	view/nsViewManager.cpp:415
6 	xul.dll 	nsViewManager::ProcessPendingUpdates() 	view/nsViewManager.cpp:1104
7 	xul.dll 	nsRefreshDriver::Tick(__int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:2031
8 	xul.dll 	mozilla::RefreshDriverTimer::TickDriver(nsRefreshDriver*, __int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:329
9 	xul.dll 	mozilla::RefreshDriverTimer::TickRefreshDrivers(__int64, mozilla::TimeStamp, nsTArray<RefPtr<nsRefreshDriver> >&) 	layout/base/nsRefreshDriver.cpp:299
10 	xul.dll 	mozilla::RefreshDriverTimer::Tick(__int64, mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:321
11 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:711
12 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:624
13 	xul.dll 	mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::NotifyVsync(mozilla::TimeStamp) 	layout/base/nsRefreshDriver.cpp:525
14 	xul.dll 	mozilla::layout::VsyncChild::RecvNotify(mozilla::TimeStamp const&) 	layout/ipc/VsyncChild.cpp:64
15 	xul.dll 	mozilla::layout::PVsyncChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PVsyncChild.cpp:155
16 	xul.dll 	mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) 	obj-firefox/ipc/ipdl/PBackgroundChild.cpp:1446
17 	xul.dll 	mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) 	ipc/glue/MessageChannel.cpp:1872
18 	xul.dll 	mozilla::ipc::MessageChannel::DispatchMessageW(IPC::Message&&) 	ipc/glue/MessageChannel.cpp:1807
19 	xul.dll 	mozilla::ipc::MessageChannel::RunMessage(mozilla::ipc::MessageChannel::MessageTask&) 	ipc/glue/MessageChannel.cpp:1680
20 	xul.dll 	mozilla::ipc::MessageChannel::MessageTask::Run() 	ipc/glue/MessageChannel.cpp:1713
21 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1269
22 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/threads/nsThreadUtils.cpp:389
23 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:124
24 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
25 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
26 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
27 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
28 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:263
29 	xul.dll 	XRE_RunAppShell() 	toolkit/xre/nsEmbedFunctions.cpp:854
30 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:269
31 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
32 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
33 	xul.dll 	XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/nsEmbedFunctions.cpp:686
34 	firefox.exe 	content_process_main(mozilla::Bootstrap*, int, char** const) 	ipc/contentproc/plugin-container.cpp:64
35 	firefox.exe 	NS_internal_main(int, char**, char**) 	browser/app/nsBrowserApp.cpp:286
36 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
37 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
38 	kernel32.dll 	BaseThreadInitThunk 	
39 	ntdll.dll 	RtlUserThreadStart 	



Application Basics: 
Name: Firefox
Version: 55.0a1
Build ID: 20170317111607
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:55.0) Gecko/20100101 Firefox/55.0

Comment 1

[Marcia Knous [:marcia]]

[Tracking Requested - why for this release]: Nominating since this is a new spike and a potential security issue.

This signature had a noticeable spike in 68.0b8, larger than what is shown currently on 67 release. Matt - Any ideas why we would have not a significant spike? This is what went in between b7 and b8: https://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=FIREFOX_68_0b7_RELEASE&tochange=b25d2ea5cf47fc4e0c01214acad7c30162b1b1d8&full=1

Also I am marking this as security sensitive since many of the 68.0b8 signatures look like potential UAFs.

Comment 2

[[:philipp]]

this first happened in 69.0a1 nightly build 20190527095441 and now in 68.0b8, which is pointing towards the changes from bug 1547624 as the source of the recent spike. urls in crash reports are commonly showing that this happened while attempting to print a page (about:printpreview).

Comment 3

[Matt Woodrow (:mattwoodrow)]

Yeah, this is almost certainly bug 1547624, which we should probably back out of beta.

Thanks for the printpreview tip, I have a working theory, digging in more.

Comment 4

[Matt Woodrow (:mattwoodrow)]

I looked at the crash dump, and this is definitely a UAF of the FrameLayerBuilder instance.

I think what's happening is we have duplicate display items (that both map to the same DisplayItemData, so we're configuring a single temporary LayerManager twice.

The second time we run CreateInactiveLayerData with the same LayerManager it creates a new FrameLayerBuilder (and deletes the old) one. The InactiveLayerData from the first run still has the raw pointer to the deleted FrameLayerBuilder.

I think we can do a quick fix to not hold the raw pointer, and look it up from the LayerManager when we need it.

That'll still be doing broken painting for the duplicate item case (which isn't a regression), so I want to try fix the underlying problem too.

Comment 5

[Julien Cristau [:jcristau]]

Bug 1547624 backed out of beta: https://hg.mozilla.org/releases/mozilla-beta/rev/34a9ba95e9085caa5dd46f7aa78b6a7c43897fcb

Comment 6

[Matt Woodrow (:mattwoodrow)]

Attachment: Bug 1348503 - Retrieve the FrameLayerBuilder pointer from the LayerManager when we need it. r?miko

Comment 7

[Matt Woodrow (:mattwoodrow)]

The patch there is just a workaround, doesn't fix the broken painting or the real underlying issue.

I've filed bug 1558937 to fix this properly, and will include a test there (that locally reproduces an ASAN failure because of this bug, and this patch fixes).

Comment 8

[Matt Woodrow (:mattwoodrow)]

Note that I'm not marking a dependency, since the testcase combined with this patch makes the problem pretty easy to reproduce. It's Nightly only now, but still probably worth avoiding.

Comment 9

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/integration/autoland/rev/6f11e76d382220103cb9b65b06964429aeeb9010
https://hg.mozilla.org/mozilla-central/rev/6f11e76d3822

Comment 10

[Julien Cristau [:jcristau]]

Do we want to uplift some combination of this / 1547624 / 1558937 to 68, or leave things as-is?

Comment 11

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 9071720 [details]
Bug 1348503 - Retrieve the FrameLayerBuilder pointer from the LayerManager when we need it. r?miko

Beta/Release Uplift Approval Request

• User impact if declined: We can't uplift bug 1547624 (an important scrolling regression) without this.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: Bug 1547624
• Risk to taking this patch: Medium
• Why is the change risky/not risky? (and alternatives if risky): This patch is fairly simple, but bug 1547624 already had this regression, the code is complicated.

Assuming there are no other new spiking crashes that I've missed, then I think it's worth uplifting and trying again.

• String changes made/needed:

Comment 12

[Julien Cristau [:jcristau]]

Comment on attachment 9071720 [details]
Bug 1348503 - Retrieve the FrameLayerBuilder pointer from the LayerManager when we need it. r?miko

approved for 68.0b14

Comment 13

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/4ae8f7ba9988