1076825 - crash in mozilla::layers::RotatedContentBuffer::BorrowDrawTargetForPainting

Status: VERIFIED FIXED

(Core :: Graphics: Layers, defect)

Description

[Robert Kaiser]

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.

Comment 1

[Robert Kaiser]

[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.

Comment 2

[Nicolas Silva [:nical]]

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.

Comment 3

[Robert Kaiser]

https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3ABeginPaint%28mozilla%3A%3Alayers%3A%3AThebesLayer%2A%2C%20unsigned%20int%29 also is there with a 0x0 address on 33.0b8.

Comment 4

[Robert Kaiser]

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.

Comment 5

[Nicolas Silva [:nical]]

Nevermind, I got confused by the logic inside BorrowDrawTargetForQudrantUpdate

Comment 6

[Sylvestre Ledru [:Sylvestre]]

Nical, 33 rc is going to build today, I guess you won't be able to fix that today? Thanks

Comment 7

[Nicolas Silva [:nical]]

Attachment: 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).

Comment 8

[Bas Schouten (:bas.schouten)]

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.

Comment 9

[Nicolas Silva [:nical]]

fixed nit and landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2971c67412a

Comment 10

[Nicolas Silva [:nical]]

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]:

Comment 11

[Sylvestre Ledru [:Sylvestre]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0af2ec81ae6c
https://hg.mozilla.org/releases/mozilla-release/rev/d89ec5b69c01

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c2971c67412a

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Backed out from 33 at Sylvestre's request.
https://hg.mozilla.org/releases/mozilla-release/rev/1233c159ab6d

Comment 15

[Sylvestre Ledru [:Sylvestre]]

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)

Comment 16

[Robert Kaiser]

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.

Comment 17

[Ada [:adalucinet]]

No crashes present in Socorro in the last 2 weeks with [@ mozilla::layers::RotatedContentBuffer::BorrowDrawTargetForPainting(mozilla::layers::RotatedContentBuffer::PaintState&, mozilla::layers::RotatedContentBuffer::DrawIterator*)] signature for:

-- 34.0b: https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A34.0b&range_value=2&range_unit=weeks&date=10%2F20%2F2014+12%3A00%3A00&query_search=signature&query_type=contains&query=mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3ABorrowDrawTargetForPainting%28mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3APaintState%26%2C+mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3ADrawIterator*%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

-- 35.0a2: https://crash-stats.mozilla.com/query/?product=Firefox&version=Firefox%3A35.0a2&range_value=2&range_unit=weeks&date=10%2F20%2F2014+12%3A00%3A00&query_search=signature&query_type=contains&query=mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3ABorrowDrawTargetForPainting%28mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3APaintState%26%2C+mozilla%3A%3Alayers%3A%3ARotatedContentBuffer%3A%3ADrawIterator*%29&reason=&release_channels=&build_id=&process_type=any&hang_type=any

Since the crashes for 33 branch are tracked separatly (bug 1081171), marking this accordingly.