Closed Bug 1076825 Opened 6 years ago Closed 6 years ago

crash in mozilla::layers::RotatedContentBuffer::BorrowDrawTargetForPainting

Categories

(Core :: Graphics: Layers, defect)

x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox33 + wontfix
firefox34 --- verified
firefox35 --- verified

People

(Reporter: kairo, Assigned: nical)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug was filed from the Socorro interface and is 
report bp-81b10ccc-be35-4f46-ae6f-034432141002.
=============================================================

Stack frames:
0 	xul.dll 	mozilla::layers::RotatedContentBuffer::BorrowDrawTargetForPainting(mozilla::layers::RotatedContentBuffer::PaintState&, mozilla::layers::RotatedContentBuffer::DrawIterator*) 	gfx/layers/RotatedBuffer.cpp
1 	xul.dll 	mozilla::layers::ContentClientRemoteBuffer::BorrowDrawTargetForPainting(mozilla::layers::RotatedContentBuffer::PaintState&, mozilla::layers::RotatedContentBuffer::DrawIterator*) 	obj-firefox/dist/include/mozilla/layers/ContentClient.h
2 	xul.dll 	mozilla::layers::ClientThebesLayer::PaintThebes() 	gfx/layers/client/ClientThebesLayer.cpp
3 	xul.dll 	mozilla::layers::ClientThebesLayer::RenderLayer() 	gfx/layers/client/ClientThebesLayer.cpp
4 	xul.dll 	mozilla::layers::ClientContainerLayer::RenderLayer() 	gfx/layers/client/ClientContainerLayer.h


This happens mostly in 33.0b8 and Nightly 35, with lower volume also elsewhere. The crash is seen on all Windows versions, and it's a EXCEPTION_ACCESS_VIOLATION_READ at 0x0, so that could smell like a null deref.
The sample I looked at all have D3DLayers+ from what I see. The URLs are not helpful, mostly Facebook.
[Tracking Requested - why for this release]:
We may want to track this given it's only spiking into the top 20 now in b8 data.
Matt do you know why we call BorrowDrawTargetForQudrantUpdate with BUFFER_BOTH regardless of whether the paint state says component alpha or not, here? http://hg.mozilla.org/releases/mozilla-beta/annotate/de4652503fb7/gfx/layers/RotatedBuffer.cpp#l702 (hg blame points to you)

looks like this is allocating a texture on white even when we aren't using component alpha.
Flags: needinfo?(matt.woodrow)
On Aurora 34, this started with the 2014-09-30 build so a checkin from 9/29 on that branch seems likely. Bug 1061699 is about BorrowDrawTarget and also landed for 33.0b8, where this spiked, so it sounds connected to me.
Nevermind, I got confused by the logic inside BorrowDrawTargetForQudrantUpdate
Flags: needinfo?(matt.woodrow)
Nical, 33 rc is going to build today, I guess you won't be able to fix that today? Thanks
Flags: needinfo?(nical.bugzilla)
Bas, can I haz an express review? It'd be good to have this in beta today for the RC.

In some of the patches we landed for OMTC Windows stability, we made it so that release builds would not crash in some of the texture allocation failure cases. BorrowDrawTargetForPainting still expected the allocations to be successful so some of the crashes we fixed moved there. I checked that with this one fixed we are not moving the crash to another place (although we do end up with some broken rendering which is sad).
Assignee: nobody → nical.bugzilla
Attachment #8500339 - Flags: review?(bas)
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8500339 [details] [diff] [review]
Don't crash in RotatedBuffer if we failed to allocate the buffer on white.

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

::: gfx/layers/RotatedBuffer.cpp
@@ +727,5 @@
>    }
>  
>    if (aPaintState.mMode == SurfaceMode::SURFACE_COMPONENT_ALPHA) {
>      MOZ_ASSERT(mDTBuffer && mDTBufferOnWhite);
> +    if (!mDTBuffer || !mDTBufferOnWhite) {

nit: Remove the assert above here and make it MOZ_ASSERT(false) or something in here.
Attachment #8500339 - Flags: review?(bas) → review+
Comment on attachment 8500339 [details] [diff] [review]
Don't crash in RotatedBuffer if we failed to allocate the buffer on white.

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Crashes on Windows
[Describe test coverage new/current, TBPL]:
[Risks and why]: low, simply null-check and replaces a crash by a glitch. 
[String/UUID change made/needed]:
Attachment #8500339 - Flags: approval-mozilla-beta?
Attachment #8500339 - Flags: approval-mozilla-aurora?
Comment on attachment 8500339 [details] [diff] [review]
Don't crash in RotatedBuffer if we failed to allocate the buffer on white.

beta has been merged to release a few minutes ago.
Attachment #8500339 - Flags: approval-mozilla-release+
Attachment #8500339 - Flags: approval-mozilla-beta?
Attachment #8500339 - Flags: approval-mozilla-beta-
Attachment #8500339 - Flags: approval-mozilla-aurora?
Attachment #8500339 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/mozilla-central/rev/c2971c67412a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify+
Attachment #8500339 - Flags: approval-mozilla-release+
Attachment #8500339 - Flags: approval-mozilla-beta-
For some context, Kairo noticed that the number of crashes increased with RC. Since we are going to build a new 33.0, we backout the three patches (bug 1074378, bug 1076825 and bug 1044975) to go back to the beta 9 levels (we were happy with them). We are unsure which one causes the increase.
https://crash-analysis.mozilla.com/rkaiser/2014-10-09/2014-10-09.firefox.33.explosiveness.html

Leaving the status flag as affected, we could take it back for 33.1 once we have more information (with 34 in beta)
See bug 1081171 on new the crashes we saw in RC. Bas said it's probable that bug 1074378 is actually at fault, but we backed out all gfx changed between b9 and RC to be on the safe side.
You need to log in before you can comment on or make changes to this bug.