1347632 - Crash in OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | mozilla::gfx::RecordedSourceSurfaceCreation::RecordedSourceSurfaceCreation

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-34d25e7e-1076-4481-b44b-bc66c2170315.
=============================================================
Crashing Thread (0)
Frame 	Module 	Signature 	Source
0 	mozglue.dll 	mozalloc_abort(char const* const) 	memory/mozalloc/mozalloc_abort.cpp:33
1 	mozglue.dll 	mozalloc_handle_oom(unsigned int) 	memory/mozalloc/mozalloc_oom.cpp:46
2 	mozglue.dll 	moz_xmalloc 	memory/mozalloc/mozalloc.cpp:85
3 	xul.dll 	mozilla::gfx::RecordedSourceSurfaceCreation::RecordedSourceSurfaceCreation(std::basic_istream<char, std::char_traits<char> >&) 	gfx/2d/RecordedEvent.cpp:1325
4 	xul.dll 	mozilla::gfx::RecordedEvent::LoadEventFromStream(std::basic_istream<char, std::char_traits<char> >&, mozilla::gfx::RecordedEvent::EventType) 	gfx/2d/RecordedEvent.cpp:70
5 	xul.dll 	mozilla::gfx::InlineTranslator::TranslateRecording(std::basic_istream<char, std::char_traits<char> >&) 	gfx/2d/InlineTranslator.cpp:50
6 	xul.dll 	mozilla::layers::FillRectWithMask(mozilla::gfx::DrawTarget*, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::SourceSurface*, mozilla::gfx::SamplingFilter, mozilla::gfx::DrawOptions const&, mozilla::gfx::ExtendMode, mozilla::gfx::SourceSurface*, mozilla::gfx::Matrix const*, mozilla::gfx::Matrix const*) 	gfx/layers/basic/BasicLayersImpl.cpp:168
7 	xul.dll 	mozilla::layers::FillRectWithMask(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::gfx::SourceSurface*, mozilla::gfx::SamplingFilter, mozilla::gfx::DrawOptions const&, mozilla::layers::Layer*) 	gfx/layers/basic/BasicLayersImpl.cpp:199
8 	xul.dll 	mozilla::layers::BasicCanvasLayer::Paint(mozilla::gfx::DrawTarget*, mozilla::gfx::PointTyped<mozilla::gfx::UnknownUnits, float> const&, mozilla::layers::Layer*) 	gfx/layers/basic/BasicCanvasLayer.cpp:117
9 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp:716
10 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp:896
11 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp:736
12 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp:896
13 	xul.dll 	mozilla::layers::BasicLayerManager::PaintSelfOrChildren(mozilla::layers::PaintLayerContext&, gfxContext*) 	gfx/layers/basic/BasicLayerManager.cpp:736
14 	xul.dll 	mozilla::layers::BasicLayerManager::PaintLayer(gfxContext*, mozilla::layers::Layer*, void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*) 	gfx/layers/basic/BasicLayerManager.cpp:896
15 	xul.dll 	mozilla::layers::BasicLayerManager::EndTransactionInternal(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/basic/BasicLayerManager.cpp:622
16 	xul.dll 	nsDisplayList::PaintRoot(nsDisplayListBuilder*, nsRenderingContext*, unsigned int) 	layout/painting/nsDisplayList.cpp:2055
17 	xul.dll 	nsLayoutUtils::PaintFrame(nsRenderingContext*, nsIFrame*, nsRegion const&, unsigned int, nsDisplayListBuilderMode, nsLayoutUtils::PaintFrameFlags) 	layout/base/nsLayoutUtils.cpp:3675
18 	xul.dll 	nsSimplePageSequenceFrame::PrintNextPage() 	layout/generic/nsSimplePageSequenceFrame.cpp:772
19 	xul.dll 	nsPrintEngine::PrintPage(nsPrintObject*, bool&) 	layout/printing/nsPrintEngine.cpp:2745
20 	xul.dll 	nsPagePrintTimer::Run() 	layout/printing/nsPagePrintTimer.cpp:89
21 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp:1240
22 	xul.dll 	mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:96
23 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:301
24 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
25 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
26 	xul.dll 	nsBaseAppShell::Run() 	widget/nsBaseAppShell.cpp:156
27 	xul.dll 	nsAppShell::Run() 	widget/windows/nsAppShell.cpp:262
28 	xul.dll 	XRE_RunAppShell() 	toolkit/xre/nsEmbedFunctions.cpp:924
29 	xul.dll 	mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp:269
30 	xul.dll 	MessageLoop::RunHandler() 	ipc/chromium/src/base/message_loop.cc:231
31 	xul.dll 	MessageLoop::Run() 	ipc/chromium/src/base/message_loop.cc:211
32 	xul.dll 	XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/nsEmbedFunctions.cpp:756
33 	xul.dll 	mozilla::BootstrapImpl::XRE_InitChildProcess(int, char** const, XREChildData const*) 	toolkit/xre/Bootstrap.cpp:65
34 	firefox.exe 	content_process_main(mozilla::Bootstrap*, int, char** const) 	ipc/contentproc/plugin-container.cpp:115
35 	firefox.exe 	wmain 	toolkit/xre/nsWindowsWMain.cpp:115
36 	firefox.exe 	__scrt_common_main_seh 	f:/dd/vctools/crt/vcstartup/src/startup/exe_common.inl:253
37 	kernel32.dll 	BaseThreadInitThunk 	
38 	ntdll.dll 	__RtlUserThreadStart 	
39 	ntdll.dll 	_RtlUserThreadStart

this signature is apparently on the rise in 53.0b2 - maybe related to the patch for bug 1345815?
it's happening for windows users on 32builds of firefox so far.

Comment 1

[Lee Salzman [:lsalzman]]

I don't think bug 1345815 actually caused this. It is that both OOMs would occur under the same usage scenario (printing), and it is possible that bug 1345815, being a more common OOM, masked this one. I will do a bit of poking around and see if I can either fit it or forward to the relevant people for deeper investigation.

Comment 2

[[:philipp]]

another printing related crash that apparently got worse in 53 would be bug 1347646. not sure if that might be related as well...

Comment 3

[Lee Salzman [:lsalzman]]

So bug 1345815 was not causative, just masking a potentially nastier underlying issue.

Toby, it looks like this is trying to play back very large embedded recorded source surfaces. Any idea what would have introduced a change in this lately?

Comment 4

[Lee Salzman [:lsalzman]]

Attachment: make RecordedSourceSurfaceCreation fallible

It turns out this allocation happening here is non-negotiable, in that it is a regular SourceSurface that is needed for the recording playback to proceed.

The best we can do is make this fallible such that it will just fail instead of crash.

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to [:philipp] from comment #2)
> another printing related crash that apparently got worse in 53 would be bug
> 1347646. not sure if that might be related as well...

Bug 1347646 is a separate bug, and one that is even nastier than this one, so should remain separate. Basically, this bug is regarding playback, which we can at least easily mitigate via fallibility since we control allocation. But bug 1347646 is about actually writing to the std::stringstream, which is unfortunately not easy to make fallible in the way we are doing here or possibly at all.

Comment 6

[Tobias Schneider [:tobytailor]]

Comment on attachment 8847736 [details] [diff] [review]
make RecordedSourceSurfaceCreation fallible

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

r+

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b06d7097e4fc
make RecordedSourceSurfaceCreation fallible. r=tobytailor

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b06d7097e4fc

Comment 9

[Lee Salzman [:lsalzman]]

Comment on attachment 8847736 [details] [diff] [review]
make RecordedSourceSurfaceCreation fallible

Approval Request Comment
[Feature/Bug causing the regression]: bug 1311512
[User impact if declined]: OOMs during printing.
[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]:
[Is the change risky?]: no
[Why is the change risky/not risky?]: Prevents crash during printing on OOM and replaces it with a warning.
[String changes made/needed]: None

Comment 10

[Gerry Chang [:gchang]]

Comment on attachment 8847736 [details] [diff] [review]
make RecordedSourceSurfaceCreation fallible

Fix a crash. Aurora54+ & Beta53+.

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e2024a4ad34f

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/5403a55bd3e9

Comment 13

[Andrei Vaida [:avaida]]

This crash seems to be happening mostly on Windows 7 with 32-bit Firefox builds. Let's check if it's now handled as intended.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

crash-stats is showing hits on releases older than 53, which is what bug 1311512 shipped in. Would this patch be expected to be effective on ESR52 too as well or is there another issue at play there?

Comment 15

[Lee Salzman [:lsalzman]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #14)
> crash-stats is showing hits on releases older than 53, which is what bug
> 1311512 shipped in. Would this patch be expected to be effective on ESR52
> too as well or is there another issue at play there?

The draw target recording code is older than 53, however it was only in 53 that it was used for printing, which causes it to allocate much bigger chunks of memory than it has ever done previously and also to get more frequent usage. So while it may occur in versions earlier than 53 due to other older uses of the code, the question would be whether the OOMs there are of sufficient magnitude and frequency to justify mitigating it. I did a cursory look previously and it did not appear as such, but I could be wrong.

Comment 16

[Lee Salzman [:lsalzman]]

Comment on attachment 8847736 [details] [diff] [review]
make RecordedSourceSurfaceCreation fallible

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
User impact if declined: OOM crashes during printing.
Fix Landed on Version: 55
Risk to taking this patch (and alternatives if risky): Some content may fail to show up in printed output during printing where Firefox would normally just crash instead because of OOM.
String or UUID changes made by this patch: None

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 17

[Julien Cristau [:jcristau]]

Comment on attachment 8847736 [details] [diff] [review]
make RecordedSourceSurfaceCreation fallible

prevent crash on oom when printing, esr52+

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/7d8f7a52a108

Comment 19

[Bogdan Maris, Desktop Test Engineering]

I was unable to reproduce the initial crash on Windows 7 32bit using the build with the most crashes recorded in Socorro (53 beta 6). Based on Socorro there are no crashes on any build newer than 53 beta 6 or Dev Edition from 2017-03-23 in the last 7 days so I think we can call this verified fixed across builds.