crash in mozilla::layers::CairoImage::GetTextureClient(mozilla::layers::CompositableClient*)

VERIFIED FIXED in mozilla33

Status

()

--
critical
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: wsmwk, Assigned: nical)

Tracking

({crash, regression})

Trunk
mozilla33
x86
Windows NT
crash, regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
nightlies only.
Operating System	Percentage
Windows 8.1 	48.28 %
Windows 7 	40.30 %
Windows 8 	8.19 %

first crash from https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3ACairoImage%3A%3AGetTextureClient%28mozilla%3A%3Alayers%3A%3ACompositableClient*%29&product=Firefox&query_type=is_exactly&range_unit=weeks&process_type=any&hang_type=any&date=2014-06-02+12%3A00%3A00&range_value=4#tab-correlations is 
report bp-7160211f-77ef-499a-be88-35a612140505 
buildid 20140504030205
=============================================================
0	xul.dll	mozilla::layers::CairoImage::GetTextureClient(mozilla::layers::CompositableClient *)	gfx/layers/ImageContainer.cpp
1	xul.dll	mozilla::layers::ImageClientSingle::UpdateImageInternal(mozilla::layers::ImageContainer *,unsigned int,bool *)	gfx/layers/client/ImageClient.cpp
2	xul.dll	mozilla::layers::ImageClientSingle::UpdateImage(mozilla::layers::ImageContainer *,unsigned int)	gfx/layers/client/ImageClient.cpp
3	xul.dll	mozilla::layers::ClientImageLayer::RenderLayer()	gfx/layers/client/ClientImageLayer.cpp
4	xul.dll	mozilla::layers::ClientContainerLayer::RenderLayer()	gfx/layers/client/ClientContainerLayer.h
5	xul.dll	mozilla::layers::ClientContainerLayer::RenderLayer()	gfx/layers/client/ClientContainerLayer.h
6	xul.dll	mozilla::layers::ClientLayerManager::EndTransactionInternal(void (*)(mozilla::layers::ThebesLayer *,gfxContext *,nsIntRegion const &,mozilla::layers::DrawRegionClip,nsIntRegion const &,void *),void *,mozilla::layers::LayerManager::EndTransactionFlags)	gfx/layers/client/ClientLayerManager.cpp
7	xul.dll	mozilla::layers::ClientLayerManager::EndTransaction(void (*)(mozilla::layers::ThebesLayer *,gfxContext *,nsIntRegion const &,mozilla::layers::DrawRegionClip,nsIntRegion const &,void *),void *,mozilla::layers::LayerManager::EndTransactionFlags)	gfx/layers/client/ClientLayerManager.cpp
8	xul.dll	nsDisplayList::PaintForFrame(nsDisplayListBuilder *,nsRenderingContext *,nsIFrame *,unsigned int)	layout/base/nsDisplayList.cpp
9	xul.dll	nsLayoutUtils::PaintFrame(nsRenderingContext *,nsIFrame *,nsRegion const &,unsigned int,unsigned int)	layout/base/nsLayoutUtils.cpp
10	xul.dll	PresShell::Paint(nsView *,nsRegion const &,unsigned int)	layout/base/nsPresShell.cpp
11	xul.dll	nsViewManager::ProcessPendingUpdatesPaint(nsIWidget *)	view/src/nsViewManager.cpp
12	xul.dll	nsViewManager::ProcessPendingUpdatesForView(nsView *,bool)	view/src/nsViewManager.cpp
13	xul.dll	nsViewManager::ProcessPendingUpdates()	view/src/nsViewManager.cpp

Comment 1

4 years ago
(In reply to Wayne Mery (:wsmwk) from comment #0)
> nightlies only.

I suspect OMTC Windows as the cause/trigger. When did this start?
(Reporter)

Comment 2

4 years ago
from comment 0, approx buildid 20140504030205
(Assignee)

Comment 3

4 years ago
This code is OMTC-only so it at least cannot have happenned on windows before we enabled OMTC. What's happenning is that we just successfully locked a TextureClient, but failed to get a DrawTarget out of it, which is a bit worrying because in a lot of places we don't check that the DrawTarget can be null if the TextureClient was sucessfully locked. This crash should be easy to fix (just a null-check) but I would like to know what caused us to fail to create a DrawTarget out of a valid and locked TextureClient.
Assignee: nobody → nical.bugzilla
(Assignee)

Comment 4

4 years ago
Created attachment 8437884 [details] [diff] [review]
null-check the DrawTarget in ImageClient::GetTextureClient

It's kinda bad that we need to null-check here even if the TextureClient was locked sucessfully. I have ideas to properly ensure that a locked TextureClient will always provide a good DrawTarget, but in the mean time let's add a null-check here.
Attachment #8437884 - Flags: review?(matt.woodrow)
(Assignee)

Comment 5

4 years ago
Created attachment 8437891 [details] [diff] [review]
Make sure that D3D TextureClients can provide access to a DrawTarget if they are locked in write-mode.

D3D TextureClients are the only ones I've seen fail to provide DrawTargets while being locked so far, so here's a patch that makes sure they either can provide a DrawTarget ot fail to lock (if OPEN_WRITE). I'll refactor TextureClient to handle that at a higher level rather than doing that for all of the other TextureClient implementations.
In the mean time let's assume that GetAsDrawTarget can (rarely) fail.
Attachment #8437891 - Flags: review?(matt.woodrow)
Comment on attachment 8437884 [details] [diff] [review]
null-check the DrawTarget in ImageClient::GetTextureClient

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

::: gfx/layers/ImageContainer.cpp
@@ +653,5 @@
>    }
>  
>    textureClient->Unlock();
>  
> +  if (failed) {

Do we have an AutoLockTextureClient helper? Would make this code cleaner, since we could just immediately return when GetAsDrawTarget() fails.
Attachment #8437884 - Flags: review?(matt.woodrow) → review+
Comment on attachment 8437891 [details] [diff] [review]
Make sure that D3D TextureClients can provide access to a DrawTarget if they are locked in write-mode.

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

::: gfx/layers/d3d9/TextureD3D9.cpp
@@ +587,5 @@
>    mIsLocked = true;
>  
> +  // Make sure that successful write-lock means we will have a DrawTarget to
> +  // write into.
> +  if (aMode && OpenMode::OPEN_WRITE) {

Why is it ok to be locked in READ_WRITE or READ and not be able to return a DT?
(Assignee)

Comment 8

4 years ago
(In reply to Matt Woodrow (:mattwoodrow) from comment #7)
> Comment on attachment 8437891 [details] [diff] [review]
> Make sure that D3D TextureClients can provide access to a DrawTarget if they
> are locked in write-mode.
> 
> Review of attachment 8437891 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/d3d9/TextureD3D9.cpp
> @@ +587,5 @@
> >    mIsLocked = true;
> >  
> > +  // Make sure that successful write-lock means we will have a DrawTarget to
> > +  // write into.
> > +  if (aMode && OpenMode::OPEN_WRITE) {
> 
> Why is it ok to be locked in READ_WRITE or READ and not be able to return a
> DT?

creating/destroying Draw targets can have a non-negligible cost, depending on the backend, There isn't really a good reason to get a DrawTarget out of a TextureClient that you don't want to write into.


(In reply to Matt Woodrow (:mattwoodrow) from comment #6)
> Do we have an AutoLockTextureClient helper? Would make this code cleaner,
> since we could just immediately return when GetAsDrawTarget() fails.

Right, I'll update the patch.
(Assignee)

Comment 9

4 years ago
Addressed the comment and landed the first patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/337592467861
(Assignee)

Updated

4 years ago
Whiteboard: [leave-open]
Comment on attachment 8437891 [details] [diff] [review]
Make sure that D3D TextureClients can provide access to a DrawTarget if they are locked in write-mode.

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

::: gfx/layers/d3d11/TextureD3D11.cpp
@@ +185,5 @@
> +  }
> +
> +  // Make sure that successful write-lock means we will have a DrawTarget to
> +  // write into.
> +  if (aMode && OpenMode::OPEN_WRITE) {

One '&' here, not two. Same for the other file.

I think we should aim to make GetAsDrawTarget only valid when OPEN_WRITE is specified too, if that isn't already the case. Maybe assert the lock mode in that function.
Attachment #8437891 - Flags: review?(matt.woodrow) → review+
(Assignee)

Comment 12

4 years ago
fixed the 2nd patch and landed it:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb0089cfb28
Whiteboard: [leave-open]

Comment 13

4 years ago
(In reply to Nicolas Silva [:nical] from comment #12)
> fixed the 2nd patch and landed it:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/3fb0089cfb28

After this, when omtc is on, h264 playing is broken.
https://hg.mozilla.org/mozilla-central/rev/3fb0089cfb28
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Could this patch of caused HTML5 play on Youtube to present audio only, and no video ?

Updated

4 years ago
Depends on: 1035101
Keywords: verifyme

Comment 16

4 years ago
3 crashes in Socorro with [@ mozilla::layers::CairoImage::GetTextureClient(mozilla::layers::CompositableClient*)] signature on 33.0b in the last month - 2 for 33 beta 1 and 1 crash for beta 3:

https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3ACairoImage%3A%3AGetTextureClient%28mozilla%3A%3Alayers%3A%3ACompositableClient%2A%29&product=Firefox&query_type=contains&range_unit=weeks&process_type=any&version=Firefox%3A33.0b&hang_type=any&date=2014-09-23+08%3A00%3A00&range_value=4#tab-reports

No crash reports for 34.0a2 and 35.0a1 versions.
Also, I see no issues (as pointed in comment 13 and comment 15) with Firefox 33 beta 6 (Build ID: 20140922173023) on Windows 7 64-bit and Windows 8 32-bit.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.