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

VERIFIED FIXED in Firefox 33

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: tracy, Assigned: jwatt)

Tracking

({crash, topcrash-win})

33 Branch
mozilla36
x86
Windows NT
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox32 unaffected, firefox33+ verified, firefox34+ verified, firefox35 verified, firefox36 verified)

Details

(crash signature)

Attachments

(2 attachments)

Reporter

Description

5 years ago
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
This looks scary-ish.  Maybe we're making some assumptions that fail in the OMTC case?
Flags: needinfo?(bas)

Comment 2

5 years ago
[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

5 years ago
[Tracking Requested - why for this release]: See previous comment
I'm tracking on both 33 and 34. If we find a safe enough fix, we can consider taking this in 33.1.0.
Bas, do you think we can wallpaper this at least?
It seems like this could have been caused by bug 986526
Assignee

Comment 7

5 years ago
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.
Attachment #8512745 - Flags: review?(bas)
Assignee

Comment 8

5 years ago
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 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)
Attachment #8512745 - Flags: review?(bas) → review+
Assignee

Comment 11

5 years ago
(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.)
Jonathan, are you planning to land that in 33.1? Thanks
If yes, it will have to land very soon.
Flags: needinfo?(jwatt)
Assignee

Comment 13

5 years ago
Still waiting for the second review from Bas.
Flags: needinfo?(jwatt)
Assignee

Comment 14

5 years ago
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
Attachment #8512745 - Flags: checkin+
Assignee

Comment 15

5 years ago
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.
Assignee

Comment 16

5 years ago
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
Attachment #8512745 - Flags: approval-mozilla-release?
Attachment #8512745 - Flags: approval-mozilla-beta?
Attachment #8512745 - Flags: approval-mozilla-aurora?
Assignee

Comment 17

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

Approval Request Comment
See previous comment
Attachment #8512804 - Flags: checkin+
Attachment #8512804 - Flags: approval-mozilla-release?
Attachment #8512804 - Flags: approval-mozilla-beta?
Attachment #8512804 - Flags: approval-mozilla-aurora?
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
Attachment #8512804 - Flags: approval-mozilla-release?
Attachment #8512804 - Flags: approval-mozilla-release-
Attachment #8512804 - Flags: approval-mozilla-beta?
Attachment #8512804 - Flags: approval-mozilla-beta+
Attachment #8512804 - Flags: approval-mozilla-aurora?
Attachment #8512804 - Flags: approval-mozilla-aurora+
Attachment #8512745 - Flags: approval-mozilla-release?
Attachment #8512745 - Flags: approval-mozilla-release-
Attachment #8512745 - Flags: approval-mozilla-beta?
Attachment #8512745 - Flags: approval-mozilla-beta+
Attachment #8512745 - Flags: approval-mozilla-aurora?
Attachment #8512745 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/920fc43b861f
https://hg.mozilla.org/mozilla-central/rev/0e1a08d24467
Assignee: nobody → jwatt
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
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.
Attachment #8512804 - Flags: approval-mozilla-release- → approval-mozilla-release+
Attachment #8512745 - Flags: approval-mozilla-release- → approval-mozilla-release+

Comment 23

5 years ago
(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.
Depends on: 1101195
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.
Attachment #8512804 - Flags: review?(bas) → review+
Flags: needinfo?(bas)
You need to log in before you can comment on or make changes to this bug.