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

VERIFIED FIXED in Firefox 34

Status

()

Core
Graphics: Layers
--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: Robert Kaiser, Assigned: nical)

Tracking

({crash})

Trunk
mozilla35
x86
Windows NT
crash
Points:
---
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox33+ wontfix, firefox34 verified, firefox35 verified)

Details

(crash signature)

Attachments

(1 attachment)

(Reporter)

Description

4 years ago
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.
(Reporter)

Comment 1

4 years ago
[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.
tracking-firefox33: --- → ?
tracking-firefox33: ? → +
(Assignee)

Comment 2

4 years ago
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)
(Reporter)

Comment 4

4 years ago
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.
(Assignee)

Comment 5

4 years ago
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)
(Assignee)

Comment 7

4 years ago
Created attachment 8500339 [details] [diff] [review]
Don't crash in RotatedBuffer if we failed to allocate the buffer on white.

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+
(Assignee)

Comment 10

4 years ago
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/releases/mozilla-aurora/rev/0af2ec81ae6c
https://hg.mozilla.org/releases/mozilla-release/rev/d89ec5b69c01
status-firefox33: --- → fixed
status-firefox34: --- → fixed
status-firefox35: --- → fixed
https://hg.mozilla.org/mozilla-central/rev/c2971c67412a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify+
Backed out from 33 at Sylvestre's request.
https://hg.mozilla.org/releases/mozilla-release/rev/1233c159ab6d
status-firefox33: fixed → affected
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)
(Reporter)

Comment 16

4 years ago
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.
status-firefox33: affected → wontfix
You need to log in before you can comment on or make changes to this bug.