1076910 - crash in gfxUtils::EncodeSourceSurface(mozilla::gfx::SourceSurface*, nsACString_internal const&, nsAString_internal const&, gfxUtils::BinaryOrData, _iobuf*)

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Tracy Walker [:tracy]]

This bug was filed from the Socorro interface and is 
report bp-82ad2874-158c-4654-8b3b-f71bb2141001.
=============================================================

Windows 7, 8 and 8.1 only.  Higher volume in 33.0b7 than in beta8, but is present still.  Should keep an eye on this to see if its volume is significant on beta8

Frame 	Module 	Signature 	Source
0 	xul.dll 	gfxUtils::EncodeSourceSurface(mozilla::gfx::SourceSurface*, nsACString_internal const&, nsAString_internal const&, gfxUtils::BinaryOrData, _iobuf*) 	gfx/thebes/gfxUtils.cpp
1 	xul.dll 	mozilla::widget::AsyncEncodeAndWriteIcon::Run() 	widget/windows/WinUtils.cpp
2 	xul.dll 	nsThread::ProcessNextEvent(bool, bool*) 	xpcom/threads/nsThread.cpp
3 	xul.dll 	NS_ProcessNextEvent(nsIThread*, bool) 	xpcom/glue/nsThreadUtils.cpp
4 	xul.dll 	mozilla::ipc::MessagePumpForNonMainThreads::Run(base::MessagePump::Delegate*) 	ipc/glue/MessagePump.cpp
5 	xul.dll 	_SEH_epilog4

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

This looks scary-ish.  Maybe we're making some assumptions that fail in the OMTC case?

Comment 2

[Robert Kaiser]

[Tracking Requested - why for this release]:

The stats in https://crash-stats.mozilla.com/report/list?product=Firefox&signature=gfxUtils%3A%3AEncodeSourceSurface%28mozilla%3A%3Agfx%3A%3ASourceSurface%2A%2C+nsACString_internal+const%26%2C+nsAString_internal+const%26%2C+gfxUtils%3A%3ABinaryOrData%2C+_iobuf%2A%29 say that b7 without OMTC was affected way more than b8, but still, 32 isn't affected at all.

This is in the top 10 on Aurora as well, so we might want to track for 34.

Comment 3

[Robert Kaiser]

[Tracking Requested - why for this release]: See previous comment

Comment 4

[Lawrence Mandel [:lmandel] (use needinfo)]

I'm tracking on both 33 and 34. If we find a safe enough fix, we can consider taking this in 33.1.0.

Comment 5

[Milan Sreckovic [:milan] (needinfo for best results)]

Bas, do you think we can wallpaper this at least?

Comment 6

[Jeff Muizelaar [:jrmuizel]]

It seems like this could have been caused by bug 986526

Comment 7

[Jonathan Watt [:jwatt]]

Attachment: don't use gfxPlatform::GetPlatform() off the main thread

Here's one thing that needs fixed. Not sure if this is the reason gfxUtils::EncodeSourceSurface() is being passed a null SourceSurface, but it's definitely something that should be fixed.

Comment 8

[Jonathan Watt [:jwatt]]

To be clear, the gfxPlatform pointer that gfxPlatform::GetPlatform() returns is not safe to access off the main thread, nor is the DrawTarget pointer returned from calling ScreenReferenceDrawTarget() on it, nor is the subsequent call to DrawTargetD2D::CreateSourceSurfaceFromData since that calls SourceSurfaceD2D::InitFromData passing mRT, and presumably the ID2D1RenderTarget::CreateBitmap() call on mRT in InitFromData is not thread safe either.

Comment 9

[Jonathan Watt [:jwatt]]

Attachment: add some error checks to gfxUtils::EncodeSourceSurface

Comment 10

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8512804 [details] [diff] [review]
add some error checks to gfxUtils::EncodeSourceSurface

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

It'd be nice to be a bit more explicit about this, use gfxWarning() (or gfxCriticalError?) to send a message out that something bad happened, then return the error code.  The NS_ macros are convenient, but they're tagged as deprecated and NS_WARN_IF is suggested instead (though using gfxWarning/gfxCriticalError has it's benefits)

Comment 11

[Jonathan Watt [:jwatt]]

(In reply to Milan Sreckovic [:milan] from comment #10)
> It'd be nice to be a bit more explicit about this, use gfxWarning() (or
> gfxCriticalError?) to send a message out that something bad happened, then
> return the error code.

With this patch there are now 14 error return points from this function. There's not really any reason to treat this one as more special than the others, and adding gfxWarning() et. al. to all return points of functions like this would be...verbose (crazy?).

> The NS_ macros are convenient, but they're tagged as
> deprecated and NS_WARN_IF is suggested instead (though using
> gfxWarning/gfxCriticalError has it's benefits)

The deprecation of the NS_ENSURE_* macros is contentious, and I am one of the people who disagrees with it based on how bloated the source would become without them, and how that would discourage people from adding warnings. (I think renaming these macros to include the word "return" in their names - e.g. MOZ_WARN_AND_RETURN_IF - would be a good thing though.)

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Jonathan, are you planning to land that in 33.1? Thanks
If yes, it will have to land very soon.

Comment 13

[Jonathan Watt [:jwatt]]

Still waiting for the second review from Bas.

Comment 14

[Jonathan Watt [:jwatt]]

Comment on attachment 8512745 [details] [diff] [review]
don't use gfxPlatform::GetPlatform() off the main thread

https://hg.mozilla.org/integration/mozilla-inbound/rev/920fc43b861f

Comment 15

[Jonathan Watt [:jwatt]]

Comment on attachment 8512804 [details] [diff] [review]
add some error checks to gfxUtils::EncodeSourceSurface

https://hg.mozilla.org/integration/mozilla-inbound/rev/0e1a08d24467

This is a top crash which has probably missed 33.1, and definitely will if I wait any longer for review. This is a trivial and obviously ok change, so I landed this patch along with the r+'ed one. Review can come after the fact.

Comment 16

[Jonathan Watt [:jwatt]]

Comment on attachment 8512745 [details] [diff] [review]
don't use gfxPlatform::GetPlatform() off the main thread

Approval Request Comment
[Feature/regressing bug #]: probably bug 986526
[User impact if declined]: crashes continue
[Describe test coverage new/current, TBPL]: on m-c and ready to merge to m-c
[Risks and why]: virtually zero risk - just makes us return early instead of crashing
[String/UUID change made/needed]: none

Comment 17

[Jonathan Watt [:jwatt]]

Comment on attachment 8512804 [details] [diff] [review]
add some error checks to gfxUtils::EncodeSourceSurface

Approval Request Comment
See previous comment

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8512804 [details] [diff] [review]
add some error checks to gfxUtils::EncodeSourceSurface

We already built 33.1 and I don't think it is a driver for a new build.
So, let's take in 34

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/920fc43b861f
https://hg.mozilla.org/mozilla-central/rev/0e1a08d24467

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/98a8465147f7
https://hg.mozilla.org/releases/mozilla-aurora/rev/d65826010b63

https://hg.mozilla.org/releases/mozilla-beta/rev/8977f5061773
https://hg.mozilla.org/releases/mozilla-beta/rev/3c329a6fd0cb

Comment 21

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8512804 [details] [diff] [review]
add some error checks to gfxUtils::EncodeSourceSurface

We are going to make a build 3... So, taking it.
Please land it in the GECKO331_2014103013_RELBRANCH branch too.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

GECKO331_2014103013_RELBRANCH:
https://hg.mozilla.org/releases/mozilla-release/rev/68711234765e
https://hg.mozilla.org/releases/mozilla-release/rev/40c1f0342edf

default:
https://hg.mozilla.org/releases/mozilla-release/rev/aa636b6addb1
https://hg.mozilla.org/releases/mozilla-release/rev/75c2f18b0020

Comment 23

[Robert Kaiser]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #22)
> GECKO331_2014103013_RELBRANCH:


Apparently that's not the relbranch we built 33.0.3 from, we seem to have built from GECKO330_2014101104_RELBRANCH which does not have this patch landed. Fun times.

33.1 should have the patches, though.

Comment 24

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Latest Socorro [1] stats for the past 4 weeks show the following:
- Firefox 33.1 - 3 crashes
- Firefox 34.0b6 and newer - 0 crashes
- Firefox 35 - 0 crashes
- Firefox 36 - 1 crash

[1] - https://crash-stats.mozilla.com/report/list?product=Firefox&range_unit=days&range_value=28&signature=gfxUtils%3A%3AEncodeSourceSurface%28mozilla%3A%3Agfx%3A%3ASourceSurface%2A%2C+nsACString_internal+const%26%2C+nsAString_internal+const%26%2C+gfxUtils%3A%3ABinaryOrData%2C+_iobuf%2A%29

Comment 25

[Bas Schouten (:bas.schouten)]

Comment on attachment 8512804 [details] [diff] [review]
add some error checks to gfxUtils::EncodeSourceSurface

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

Argh, I'm sorry.