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

VERIFIED FIXED in Firefox -esr52

Status

()

Core
Graphics
--
critical
VERIFIED FIXED
10 months ago
9 months ago

People

(Reporter: philipp, Assigned: bobowen)

Tracking

({crash, regression})

53 Branch
mozilla55
x86
Windows
crash, regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox52 wontfix, firefox-esr52 verified, firefox53+ fixed, firefox54 verified, firefox55 verified)

Details

(crash signature)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

10 months ago
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]

Updated

10 months ago
Blocks: 1311512
See Also: → bug 1347632, bug 1345815

Comment 1

10 months ago
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

10 months ago
(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
Flags: needinfo?(bobowencode)
(Assignee)

Comment 3

10 months ago
(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.
Flags: needinfo?(bobowencode)

Comment 4

10 months ago
Track 53+ as the signature is accounting for ~0.6% of all crashes from 53.0b last week.
tracking-firefox53: ? → +
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.
Flags: needinfo?(vliu)
Flags: needinfo?(bobowencode)
(Assignee)

Comment 6

10 months ago
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.
Flags: needinfo?(bobowencode) → needinfo?(tschneider)
(Reporter)

Comment 7

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

Comment 8

10 months ago
sorry, beta 8
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?
Flags: needinfo?(bobowencode)
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.
(Assignee)

Comment 11

10 months ago
(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.
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 :).
Flags: needinfo?(tschneider)
Created attachment 8855641 [details] [diff] [review]
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...
Attachment #8855641 - Flags: review?(tschneider)
Attachment #8855641 - Flags: feedback?(bobowencode)
(Assignee)

Comment 14

10 months ago
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.
Attachment #8855641 - Flags: feedback?(bobowencode)
(Assignee)

Comment 15

10 months ago
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.
Flags: needinfo?(bobowencode)
(Assignee)

Comment 16

10 months ago
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
Flags: needinfo?(lsalzman)
(Assignee)

Comment 17

10 months ago
Created attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs
(Assignee)

Updated

10 months ago
Assignee: nobody → bobowencode
Status: NEW → ASSIGNED
(Assignee)

Comment 18

10 months ago
Created attachment 8855851 [details] [diff] [review]
Part 2: Use CreateRenderingContext nsSimplePageSequenceFrame::PrePrintNextPage to ensure recorder is used for canvases
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.
Attachment #8855641 - Attachment is obsolete: true
Flags: needinfo?(lsalzman)
Attachment #8855641 - Flags: review?(tschneider)
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.
status-firefox52: unaffected → affected
status-firefox-esr52: --- → affected
See Also: → bug 1280324
(Assignee)

Comment 21

10 months ago
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.)
Attachment #8855848 - Flags: review?(lsalzman)
Attachment #8855848 - Flags: review?(jwatt)
(Assignee)

Updated

10 months ago
Attachment #8855851 - Flags: review?(jwatt)
Comment on attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

Looks sane to me.
Attachment #8855848 - Flags: review?(lsalzman) → review+
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) .
(Assignee)

Comment 24

10 months ago
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
Attachment #8855848 - Flags: review?(tschneider)
Attachment #8855848 - Flags: review?(tschneider) → review+
(Assignee)

Comment 25

10 months ago
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 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.
Attachment #8855851 - Flags: review?(jwatt) → review+
Attachment #8855848 - Flags: review?(jwatt) → review+

Comment 27

10 months ago
Pushed by bobowencode@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/73cf1505d813
https://hg.mozilla.org/mozilla-central/rev/42dc50db2cdb
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 29

10 months ago
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.
Attachment #8855848 - Flags: approval-mozilla-beta?
Attachment #8855848 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 30

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

See comment 29.
Attachment #8855851 - Flags: approval-mozilla-beta?
Attachment #8855851 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 31

10 months ago
Created attachment 8856277 [details] [diff] [review]
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 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.
Attachment #8856277 - Flags: approval-mozilla-beta+
Comment on attachment 8855848 [details] [diff] [review]
Part 1: Backout 4849ef8c9a34 for causing OOM issues when printing PDFs

This is the aurora patch.
Attachment #8855848 - Flags: approval-mozilla-beta?
Attachment #8855848 - Flags: approval-mozilla-beta-
Attachment #8855848 - Flags: approval-mozilla-aurora?
Attachment #8855848 - Flags: approval-mozilla-aurora+
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.
Attachment #8855851 - Flags: approval-mozilla-beta?
Attachment #8855851 - Flags: approval-mozilla-beta+
Attachment #8855851 - Flags: approval-mozilla-aurora?
Attachment #8855851 - Flags: approval-mozilla-aurora+

Updated

10 months ago
Flags: needinfo?(vliu)
(Assignee)

Updated

10 months ago
Blocks: 1350738
(Assignee)

Comment 35

10 months ago
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.
Attachment #8855851 - Flags: approval-mozilla-esr52?
(Assignee)

Updated

10 months ago
Blocks: 1355011
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
Attachment #8855851 - Flags: approval-mozilla-esr52? → approval-mozilla-esr52+

Comment 39

10 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr52/rev/1b50711a46ce
status-firefox-esr52: affected → fixed
status-firefox52: affected → wontfix
(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.
Flags: qe-verify+
Reproduced bug 1280324 on 50.0a1 (2016-06-15), Win 7 x64.
Verified fixed Fx 55.0a1 (2017-05-07), 54b5, 52.1.1esr.
Status: RESOLVED → VERIFIED
status-firefox54: fixed → verified
status-firefox55: fixed → verified
status-firefox-esr52: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.