1347646 - Crash in OOM | large | mozalloc_abort | mozalloc_handle_oom | moz_xmalloc | std::_Allocate | std::basic_stringbuf<T>::overflow

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[[:philipp]]

This bug was filed from the Socorro interface and is 
report bp-0405fef9-4a35-45c6-95fe-a98002170315.
=============================================================
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 	std::_Allocate(unsigned int, unsigned int, bool) 	vs2015u3/VC/include/xmemory0:69
4 	xul.dll 	std::basic_stringbuf<char, std::char_traits<char>, std::allocator<char> >::overflow(int) 	vs2015u3/VC/include/sstream:151
5 	msvcp140.dll 	std::basic_streambuf<char, std::char_traits<char> >::xsputn(char const*, __int64) 	f:\dd\vctools\crt\crtw32\stdhpp\streambuf:411
6 	msvcp140.dll 	std::basic_streambuf<char, std::char_traits<char> >::sputn(char const*, __int64) 	f:\dd\vctools\crt\crtw32\stdhpp\streambuf:208
7 	msvcp140.dll 	std::basic_ostream<unsigned short, std::char_traits<unsigned short> >::write(unsigned short const*, __int64) 	f:\dd\vctools\crt\crtw32\stdhpp\ostream:564
8 	xul.dll 	mozilla::gfx::RecordedSourceSurfaceCreation::RecordToStream(std::basic_ostream<char, std::char_traits<char> >&) 	gfx/2d/RecordedEvent.cpp:1315
9 	xul.dll 	mozilla::gfx::DrawEventRecorderPrivate::RecordEvent(mozilla::gfx::RecordedEvent const&) 	gfx/2d/DrawEventRecorder.cpp:33
10 	xul.dll 	mozilla::gfx::StoreSourceSurface 	gfx/2d/DrawTargetRecording.cpp:52
11 	xul.dll 	mozilla::gfx::DrawTargetRecording::OptimizeSourceSurface(mozilla::gfx::SourceSurface*) 	gfx/2d/DrawTargetRecording.cpp:599
12 	xul.dll 	nsLayoutUtils::SurfaceFromElement(nsIImageLoadingContent*, unsigned int, RefPtr<mozilla::gfx::DrawTarget>&) 	layout/base/nsLayoutUtils.cpp:7190
13 	xul.dll 	nsLayoutUtils::SurfaceFromElement(mozilla::dom::Element*, unsigned int, RefPtr<mozilla::gfx::DrawTarget>&) 	layout/base/nsLayoutUtils.cpp:7349
14 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, double, double, double, double, double, double, unsigned char, mozilla::ErrorResult&) 	dom/canvas/CanvasRenderingContext2D.cpp:4916
15 	xul.dll 	js::Allocate<JSObject, 1>(js::ExclusiveContext*, js::gc::AllocKind, unsigned int, js::gc::InitialHeap, js::Class const*) 	js/src/gc/Allocator.cpp:68
16 	xul.dll 	js::NewObjectWithGivenTaggedProto(js::ExclusiveContext*, js::Class const*, JS::Handle<js::TaggedProto>, js::gc::AllocKind, js::NewObjectKind, unsigned int) 	js/src/jsobj.cpp:708
17 	xul.dll 	js::ProxyObject::New(JSContext*, js::BaseProxyHandler const*, JS::Handle<JS::Value>, js::TaggedProto, js::ProxyOptions const&) 	js/src/vm/ProxyObject.cpp:77
18 	vcruntime140.dll 	__vcrt_FlsGetValue 	f:\dd\vctools\crt\vcruntime\src\internal\winapi_downlevel.cpp:390
19 	xul.dll 	js::jit::LUrshD::op() 	js/src/jit/shared/LIR-shared.h:3372
20 	xul.dll 	_cairo_gstate_set_matrix 	gfx/cairo/cairo/src/cairo-gstate.c:750
21 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, double, double, double, double, double, double, mozilla::ErrorResult&) 	obj-firefox/dist/include/mozilla/dom/CanvasRenderingContext2D.h:232
22 	xul.dll 	mozilla::dom::CanvasRenderingContext2D::DrawImage(mozilla::dom::HTMLImageElementOrHTMLCanvasElementOrHTMLVideoElementOrImageBitmap const&, double, double, double, double, double, double, double, double, mozilla::ErrorResult&) 	obj-firefox/dist/include/mozilla/dom/CanvasRenderingContext2D.h:232
23 	xul.dll 	mozilla::dom::CanvasRenderingContext2DBinding::drawImage 	obj-firefox/dom/bindings/CanvasRenderingContext2DBinding.cpp:2739

crashes with this signature have been around for a while and got fixed in bug 1279699 for firefox 51. the signature seems to return now in 53 though and in even bigger volume (when just focussing in on crashes on the beta channel:
https://crash-stats.mozilla.com/signature/?release_channel=beta&signature=OOM%20%7C%20large%20%7C%20mozalloc_abort%20%7C%20mozalloc_handle_oom%20%7C%20moz_xmalloc%20%7C%20std%3A%3A_Allocate%20%7C%20std%3A%3Abasic_stringbuf%3CT%3E%3A%3Aoverflow&date=%3E%3D2016-09-15T17%3A28%3A52.000Z#graphs

the reports are coming from 32bit firefox users on windows - the signature is accounting for ~0.6% of all crashes from 53.0b last week.

Correlations for Firefox Beta
(100.0% in signature vs 04.09% overall) moz_crash_reason = MOZ_CRASH()
(93.55% in signature vs 13.95% overall) os_arch = x86
(100.0% in signature vs 36.16% overall) ipc_fatal_error_msg = null
(100.0% in signature vs 40.02% overall) plugin_version = null
(39.59% in signature vs 00.62% overall) Module "prnfldr.dll" = true [49.53% vs 00.95% if platform_version = 6.1.7600]

Comment 1

[Vincent Liu[:vliu]]

Hi Bob,
It seems that this bug had similar back trace I saw in Bug 1279699. Can you please have a look to see they are relatives? Really thanks.

Comment 2

[Vincent Liu[:vliu]]

(In reply to Vincent Liu[:vliu] from comment #1)
> Hi Bob,
> It seems that this bug had similar back trace I saw in Bug 1279699. Can you
> please have a look to see they are relatives? Really thanks.

add ni? to Bob

Comment 3

[Bob Owen (:bobowen)]

(In reply to Vincent Liu[:vliu] from comment #2)
> (In reply to Vincent Liu[:vliu] from comment #1)
> > Hi Bob,
> > It seems that this bug had similar back trace I saw in Bug 1279699. Can you
> > please have a look to see they are relatives? Really thanks.
> 
> add ni? to Bob

Yes, this is the same problem that we were having with recording things for printing.
We switched printing to record to a file instead.

The stringstream (which I think is the only out of the box in-memory ostream we have) only allocates infallibly, so we crash. I think part of the problem is that during recording we sometimes convert images to DataSourceSurfaces to store them, where we wouldn't normally.

I think the only solution (if it needs to be in-memory) is to hand roll our own ostream (or probably iostream), to allocate fallibly.
We could also look into ways that we could reduce the size of what we record.

Comment 4

[Gerry Chang [:gchang]]

Track 53+ as the signature is accounting for ~0.6% of all crashes from 53.0b last week.

Comment 5

[Liz Henry (:lizzard) (relman/hg->git project)]

Bob and Vincent, is this a graphics issue or a printing issue? Can you help find an owner for this bug? Since it's a fairly high volume crash on beta and a recent regression it would be great to find some workaround if that's possible, even for the 54 timeframe if not for 53.

Comment 6

[Bob Owen (:bobowen)]

This is from when we started using the DrawTargetRecording for mozPrintCallback (used by PDF.js) in bug 1311512.

tobytailer - I'm not sure what that bug was trying to fix, but at least it seems to have fixed bug 1347626 in Fx52.

It might be worth trying to find out what caused that regression in the first place.

Comment 7

[[:philipp]]

this is one of the top crashes in 53.0b9 (1.9% of browser crashes, 1.55% of content crashes)

Comment 8

[[:philipp]]

sorry, beta 8

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Bob, should we back out whatever is causing this crash? At first glance we just traded off a "printing blurry in an edge case, with a workaround" situation for a top crash. Not a good trade! If you agree can you land a backout here or get things started so we might address this for today's beta 10 build?

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

Oh, it's actually coming from bug 1311512, so this is more complicated. I'm not sure what we should do here but would like you and toby's advice.

Comment 11

[Bob Owen (:bobowen)]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #9)
> Bob, should we back out whatever is causing this crash? At first glance we
> just traded off a "printing blurry in an edge case, with a workaround"
> situation for a top crash. Not a good trade! If you agree can you land a
> backout here or get things started so we might address this for today's beta
> 10 build?

I'm afraid I don't know too much about the changeset that caused this crash, other than it hit a similar issue to one I hit a while ago.

You'll need to get tobytailor to look into it.

Comment 12

[Tobias Schneider [:tobytailor]]

I will have a deeper look int this. I wouldn't recommend backing this out now, given the fact that tax session is approaching. One of the major reason for this fix was people complaining about their Form 1040 printouts being blurry when printing via PDF.js :).

Comment 13

[Lee Salzman [:lsalzman]]

Attachment: reuse DrawEventRecorderFile when replaying SourceSurfaceRecording

Here's a proposed patch to fix this while keeping the intent of the patch from bug 1311512. The basic idea is to allow reusing the underlying file or memory stream like before, but in a way that it can now also reuse the file, not just memory. It works via LockInputStream/UnlockInputStream to verify that no more than one owner can read it at a time, given the limitation of iostreams.

The previous code has what might be viewed as a bug, in that it always replayed the entire recording for each page, but this is not necessarily correct depending on where the recording was started (on DrawTargetRecording creation) and where it was stopped (on snapshotting) to produce the SourceSurfaceRecording. So this patch actually keeps track of that now so this can be passed in to the InlineTranslator to play only the correct subset of the file. This is also necessary since there is now only one DrawEventRecorderFile, despite being multiple pages.

I wasn't able to test this patch locally, so it would help if someone could verify this still preserves Tobias' original fix...

Comment 14

[Bob Owen (:bobowen)]

Comment on attachment 8855641 [details] [diff] [review]
reuse DrawEventRecorderFile when replaying SourceSurfaceRecording

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

This patch gives a blank page when I try to print the example on bug 1347626.

Comment 15

[Bob Owen (:bobowen)]

It looks like Bug 1280324 originally caused the rasterization issues in bug 1347626.

I think the sizing issue that causes the problem with the top of the fonts is a separate thing that happened around the same time.
Trying to bisect now, but it's tricky because one change came from inbound and the other from autoland I think.

Comment 16

[Bob Owen (:bobowen)]

OK I think I've found the offending line change in bug 1280324 [1].
This means that we don't get a DrawTargetRecording with our recorder in it, so I don't think this one should have been changed.

If I backout the patch for bug 1311512 and change that line back to call CreateRenderingContext, then the example in bug 1347626 prints fine for me.

lsalzman - what do you think?

[1] https://hg.mozilla.org/mozilla-central/file/22df0c5cfbf8/layout/generic/nsSimplePageSequenceFrame.cpp#l633

Comment 17

[Bob Owen (:bobowen)]

Attachment: Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Comment 18

[Bob Owen (:bobowen)]

Attachment: Part 2: Use CreateRenderingContext nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases

Comment 19

[Lee Salzman [:lsalzman]]

Comment on attachment 8855641 [details] [diff] [review]
reuse DrawEventRecorderFile when replaying SourceSurfaceRecording

(In reply to Bob Owen (:bobowen) from comment #16)
> OK I think I've found the offending line change in bug 1280324 [1].
> This means that we don't get a DrawTargetRecording with our recorder in it,
> so I don't think this one should have been changed.
> 
> If I backout the patch for bug 1311512 and change that line back to call
> CreateRenderingContext, then the example in bug 1347626 prints fine for me.
> 
> lsalzman - what do you think?
> 
> [1]
> https://hg.mozilla.org/mozilla-central/file/22df0c5cfbf8/layout/generic/
> nsSimplePageSequenceFrame.cpp#l633

I am going to agree with the idea of backing out bug 1311512 and not letting it ship in 53. Given what I was seeing of this code when trying to fix it up to play back from file, I believe that change was too fragile and possibly going to cause us other bugs/headaches, and only just masking an underlying bug as you have noted.

Comment 20

[Lee Salzman [:lsalzman]]

Since bug 1280324 landed in 52, this means 52 ESR is also affected by the original blurriness issue. So we should probably have a patch ready for 52 that that fixes the issue with CreateReferenceRenderingContext - but obviously since Toby's change landed in 53 we don't have to back that out for 52 ESR.

Comment 21

[Bob Owen (:bobowen)]

Comment on attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Flagging you both for this one as I'm not sure I've picked up some subsequent change in the DrawEventRecorderMemory.
(Although it shouldn't be getting used now anyway.)

Comment 22

[Lee Salzman [:lsalzman]]

Comment on attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Looks sane to me.

Comment 23

[Liz Henry (:lizzard) (relman/hg->git project)]

Backing out for 53 sounds good to me. It would be best if that lands today or over the weekend (to make it into the 53 RC build on Monday) .

Comment 24

[Bob Owen (:bobowen)]

Comment on attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Adding tobytailor on the request of jwatt

Comment 25

[Bob Owen (:bobowen)]

Try push, only one mochitest suite as we still don't have this covered anyway :-(
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e5d320a520f47732c9e13e254000152bff8bf7d

Comment 26

[Jonathan Watt [:jwatt]]

Comment on attachment 8855851 [details] [diff] [review]
Part 2: Use CreateRenderingContext nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases

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

Please expand on this review comment a bit since this isn't enough to make clear to future readers of the commit what the heck is going on here. Maybe say something like:

Bug 1347646 Part 2: Use CreateRenderingContext in nsSimplePageSequenceFrame::PrePrintNextPage so we record canvas drawing.

Calling CreateReferenceRenderingContext currently returns a non-recording DrawTarget
even when we pass a DrawEventRecorder, due to bug 1354624.  Since we've already
called BeginPage we can call CreateRenderingContext instead though, and that will
will give us a recording DrawTarget as we require to record the canvas drawing.

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73cf1505d813
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs. r=jwatt, r=lsalzman, r=tobytailor
https://hg.mozilla.org/integration/mozilla-inbound/rev/42dc50db2cdb
Part 2: Use CreateRenderingContext in nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases. r=jwatt

Comment 28

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

https://hg.mozilla.org/mozilla-central/rev/73cf1505d813
https://hg.mozilla.org/mozilla-central/rev/42dc50db2cdb

Comment 29

[Bob Owen (:bobowen)]

Comment on attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Sheriff note:
This backout doesn't apply quite cleanly to Beta, I'll upload another patch.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1311512 introduced this OOM trying to fix bug 1280324.

[User impact if declined]:
Fairly frequent OOM crash when printing PDFs.

[Is this code covered by automated tests?]:
No.

[Has the fix been verified in Nightly?]:
Don't think we have a straight forward STR.

[Needs manual test from QE? If yes, steps to reproduce]: 
It would be good to test printing of PDFs (from PDF.js).
It would also be good to reproduce the bug 1280324 in Fx52 and see that it is still fixed with this backout and one line fix.

[List of other uplifts needed for the feature/fix]:
Other patch from this bug.

[Is the change risky?]:
Slightly.

[Why is the change risky/not risky?]:
This is a fairly straight forward backout of a patch and then a one line fix to the original problem. However we don't have automated tests for printing.

[String changes made/needed]:
None.

Comment 30

[Bob Owen (:bobowen)]

Comment on attachment 8855851 [details] [diff] [review]
Part 2: Use CreateRenderingContext nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases

See comment 29.

Comment 31

[Bob Owen (:bobowen)]

Attachment: Beta - Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Patch for Beta just for part 1, because of a slight context change.

Comment 32

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8856277 [details] [diff] [review]
Beta - Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Backout for a bad crash on beta 53.

Comment 33

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

This is the aurora patch.

Comment 34

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8855851 [details] [diff] [review]
Part 2: Use CreateRenderingContext nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases

Part 2, ok for aurora and beta.

Comment 35

[Bob Owen (:bobowen)]

Comment on attachment 8855851 [details] [diff] [review]
Part 2: Use CreateRenderingContext nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases

Sheriff note:
We only require Part 2 for ESR52 as Part 1 backs out something that was landed in Fx53.

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration:
This is not sec, but results in very poor printing from PDF.

User impact if declined: 
Printing from PDF will be rasterized resulting in poor quality or incorrectly rendered text. Sometimes making it unreadable.

Fix Landed on Version:
55 (uplift to 53 already approved).

Risk to taking this patch (and alternatives if risky):
Low - this is a one line fix, which returns the code to a similar state as to Fx51. 

String or UUID changes made by this patch: 
None.

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/18daa17d013b
https://hg.mozilla.org/releases/mozilla-aurora/rev/f7f8d7181cfb

Comment 37

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9a2a544057ec
https://hg.mozilla.org/releases/mozilla-beta/rev/6e672a99997a

Comment 38

[Julien Cristau [:jcristau]]

Comment on attachment 8855851 [details] [diff] [review]
Part 2: Use CreateRenderingContext nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases

fix printing issue on esr52

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr52/rev/1b50711a46ce

Comment 40

[Andrei Vaida [:avaida]]

(In reply to Bob Owen (:bobowen) from comment #29)
> [Needs manual test from QE? If yes, steps to reproduce]: 
> It would be good to test printing of PDFs (from PDF.js).
> It would also be good to reproduce the bug 1280324 in Fx52 and see that it
> is still fixed with this backout and one line fix.

Flagging this for manual testing.

Comment 41

[Paul Silaghi, QA [:pauly]]

Reproduced bug 1280324 on 50.0a1 (2016-06-15), Win 7 x64.
Verified fixed Fx 55.0a1 (2017-05-07), 54b5, 52.1.1esr.