Closed Bug 1116473 Opened 6 years ago Closed 5 years ago

crash in PR_Lock | mozilla::gl::AndroidSurfaceTexture::Attach(mozilla::gl::GLContext*, unsigned int)

Categories

(Core :: Graphics: Layers, defect)

37 Branch
ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox35 --- affected
firefox36 --- affected
firefox37 --- affected
firefox38 --- affected
firefox45 --- affected
firefox46 + fixed
firefox47 + fixed

People

(Reporter: csuciu, Assigned: esawin)

References

Details

(Keywords: crash, reproducible)

Crash Data

Attachments

(4 files)

This bug was filed from the Socorro interface and is 
report bp-0ecf593d-8faa-46a4-b6a1-9978b2141230.
=============================================================

Nightly 37.0a1(2014-12-30)
Samsung Galaxy Tab (4.0.4)

Steps:
1. Go to www.imdb.com and play a movie trailer

Result: Firefox crashes.

Crashing Thread
Frame 	Module 	Signature 	Source
Ø 0 	libc.so 	libc.so@0x12334 	
1 	libnss3.so 	PR_Lock 	nsprpub/pr/src/pthreads/ptsynch.c
2 	libxul.so 	mozilla::gl::AndroidSurfaceTexture::Attach(mozilla::gl::GLContext*, unsigned int) 	xpcom/glue/Mutex.h
3 	libxul.so 	mozilla::layers::SurfaceTextureHost::Lock() 	gfx/layers/opengl/TextureHostOGL.cpp
4 	libxul.so 	mozilla::layers::ImageHost::Lock() 	gfx/layers/composite/ImageHost.cpp
5 	libxul.so 	mozilla::layers::AutoLockCompositableHost::AutoLockCompositableHost(mozilla::layers::CompositableHost*) 	gfx/layers/composite/CompositableHost.h
6 	libxul.so 	mozilla::layers::ImageHost::Composite(mozilla::layers::EffectChain&, float, mozilla::gfx::Matrix4x4 const&, mozilla::gfx::Filter const&, mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const&, nsIntRegion const*) 	gfx/layers/composite/ImageHost.cpp
7 	libxul.so 	mozilla::layers::ImageLayerComposite::RenderLayer(nsIntRect const&) 	gfx/layers/composite/ImageLayerComposite.cpp
8 	libxul.so 	void mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, mozilla::gfx::IntRectTyped<mozilla::RenderTargetPixel> const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
9 	libxul.so 	void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
10 	libxul.so 	void mozilla::layers::RenderLayers<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, mozilla::gfx::IntRectTyped<mozilla::RenderTargetPixel> const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
11 	libxul.so 	void mozilla::layers::ContainerRender<mozilla::layers::ContainerLayerComposite>(mozilla::layers::ContainerLayerComposite*, mozilla::layers::LayerManagerComposite*, nsIntRect const&) 	gfx/layers/composite/ContainerLayerComposite.cpp
12 	libxul.so 	mozilla::layers::LayerManagerComposite::Render() 	gfx/layers/composite/LayerManagerComposite.cpp
13 	libxul.so 	mozilla::layers::LayerManagerComposite::EndTransaction(void (*)(mozilla::layers::PaintedLayer*, gfxContext*, nsIntRegion const&, mozilla::layers::DrawRegionClip, nsIntRegion const&, void*), void*, mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/composite/LayerManagerComposite.cpp
14 	libxul.so 	mozilla::layers::LayerManagerComposite::EndEmptyTransaction(mozilla::layers::LayerManager::EndTransactionFlags) 	gfx/layers/composite/LayerManagerComposite.cpp
15 	libxul.so 	mozilla::layers::CompositorParent::CompositeToTarget(mozilla::gfx::DrawTarget*, nsIntRect const*) 	gfx/layers/ipc/CompositorParent.cpp
16 	libxul.so 	RunnableMethod<mozilla::layers::CompositorParent, void (mozilla::layers::CompositorParent::*)(mozilla::TimeStamp), Tuple1<mozilla::TimeStamp> >::Run() 	ipc/chromium/src/base/tuple.h
4 recorded crashes in a week
Component: General → Graphics: Layers
Product: Firefox for Android → Core
Version: Firefox 37 → 37 Branch
I can also reproduce this crash on all branches using Samsung Galaxy Tab 2 (Android 4.2.2)
Crash Signature: [@ PR_Lock | mozilla::gl::AndroidSurfaceTexture::Attach(mozilla::gl::GLContext*, unsigned int)] → [@ PR_Lock | mozilla::gl::AndroidSurfaceTexture::Attach(mozilla::gl::GLContext*, unsigned int)] [@ PR_Lock | mozilla::gl::AndroidSurfaceTexture::Attach]
This crash appears to happen more often in 44.0 and also current nightly builds.

It happened to me twice now when watching a YouTube playlist. I started the first music video in the playlist, turned off the screen, no further user interaction. Some videos play successfully, then it crashes. I think it crashes between two videos, so maybe related to ads.
Eugen, can you look?
Assignee: nobody → esawin
I don't see any crashes under this signature on current versions, original STR don't reproduce it for me.
Do we have a new signature for this? Otherwise, I'll mark it as invalid.
Did you check the second linked crash signature?
[@PR_Lock | mozilla::gl::AndroidSurfaceTexture::Attach]
No, I've overlooked that one, thanks. This one still shows activity.
I can't reproduce the crash, but based on the stack it looks like AndroidSurfaceTexture::Attach keeps lock/tries to acquire lock on a mutex, which is no longer valid. This can only be the case, if the VideoDataDecoder gets destructed and all references to the surface textures become invalid.

We should switch all AndroidSurfaceTexture references to RefPtr to avoid this scenario.
The texture instance mapping is accessed across decoder threads, it needs to be thread-safe.

PS: I think this class implements two common patterns: unique-id-mapping and thread-safe class access, both of which can be easily generalized. I'll run this through mfbt peers to see if that's something we want to add.
Attachment #8722156 - Flags: review?(snorp)
Const-correcteness.
Attachment #8722157 - Flags: review?(snorp)
Replace (almost) all raw AndroidSurfaceTexture pointers with RefPtr to avoid attaching to freed textures.
Attachment #8722158 - Flags: review?(snorp)
This is an example implementation of the two patterns applied in patch 1: unique-id-map and thread-safe-class-wrapper.

This would allow us to define sInstances from patch 1 as:
static ThreadSafeWrapper<UniqueIdMap<AndroidSurfaceTexture*>> sInstances;

Just leaving it here for future reference.
Comment on attachment 8722156 [details] [diff] [review]
0001-Bug-1116473-1.1-Handle-AndroidSurfaceTexture-mapping.patch

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

I hate this stupid map so much. We only need it to handle the frame available callback. Maybe we can use jchen's hot new JNI stuff to do that?
Attachment #8722156 - Flags: review?(snorp) → review+
Attachment #8722157 - Flags: review?(snorp) → review+
Comment on attachment 8722158 [details] [diff] [review]
0003-Bug-1116473-3.1-Use-RefPtr-for-AndroidSurfaceTexture.patch

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

Looks good with RefPtr arg fixes

::: gfx/layers/GLImages.cpp
@@ +95,5 @@
>  }
>  
>  #ifdef MOZ_WIDGET_ANDROID
> +SurfaceTextureImage::SurfaceTextureImage(
> +    const RefPtr<gl::AndroidSurfaceTexture>& aSurfTex,

You can just pass the raw ptr as we did before here. I don't think it's common to pass RefPtr& around.

::: gfx/layers/opengl/TextureClientOGL.cpp
@@ +68,5 @@
>  #ifdef MOZ_WIDGET_ANDROID
>  
>  already_AddRefed<TextureClient>
> +AndroidSurfaceTextureData::CreateTextureClient(
> +    const RefPtr<AndroidSurfaceTexture>& aSurfTex,

Same deal here

::: gfx/layers/opengl/TextureHostOGL.cpp
@@ +366,5 @@
>  #ifdef MOZ_WIDGET_ANDROID
>  
> +SurfaceTextureSource::SurfaceTextureSource(
> +    CompositorOGL* aCompositor,
> +    const RefPtr<AndroidSurfaceTexture>& aSurfTex,

Same thing here too
Attachment #8722158 - Flags: review?(snorp) → review+
Interesting logs from one of the reports:

--------- beginning of /dev/log/main
02-21 02:20:19.034 10209 10209 W google-breakpad: Bad file number
02-21 02:20:19.034 10209 10209 W google-breakpad: 
02-21 02:20:19.034 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.034 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.034 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.034 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.034 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.034 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.034 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.034 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.034 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.034 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.044 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.044 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
02-21 02:20:19.044 22722  9037 E MediaCodec: err = 0
02-21 02:20:19.044 22722 10208 E MediaCodec: err = 0
<this goes on for a while>
Duplicate of this bug: 1239162
Comment on attachment 8722156 [details] [diff] [review]
0001-Bug-1116473-1.1-Handle-AndroidSurfaceTexture-mapping.patch

Approval Request Comment
[Feature/regressing bug #]: media playback
[User impact if declined]: top crasher on Android, crash during media playback
[Describe test coverage new/current, TreeHerder]: just landed on Nightly, but fixed intermittent test failure in bug 1239162 
[Risks and why]: low, simple code change which only affects certain object lifetimes
[String/UUID change made/needed]: none
Attachment #8722156 - Flags: approval-mozilla-beta?
Attachment #8722156 - Flags: approval-mozilla-aurora?
Attachment #8722158 - Flags: approval-mozilla-beta?
Attachment #8722158 - Flags: approval-mozilla-aurora?
Duplicate of this bug: 1251157
Tracking for 46+ since this is the top android crash in 46 right now.
Comment on attachment 8722156 [details] [diff] [review]
0001-Bug-1116473-1.1-Handle-AndroidSurfaceTexture-mapping.patch

We are too late in beta to take this but happy to uplift for aurora. 
This may fix the 46 fennec #1 top crash.
Attachment #8722156 - Flags: approval-mozilla-beta?
Attachment #8722156 - Flags: approval-mozilla-beta-
Attachment #8722156 - Flags: approval-mozilla-aurora?
Attachment #8722156 - Flags: approval-mozilla-aurora+
Attachment #8722157 - Flags: approval-mozilla-beta?
Attachment #8722157 - Flags: approval-mozilla-beta-
Attachment #8722157 - Flags: approval-mozilla-aurora?
Attachment #8722157 - Flags: approval-mozilla-aurora+
Attachment #8722158 - Flags: approval-mozilla-beta?
Attachment #8722158 - Flags: approval-mozilla-beta-
Attachment #8722158 - Flags: approval-mozilla-aurora?
Attachment #8722158 - Flags: approval-mozilla-aurora+
Noting that there are barely any crashes showing with these signatures for 45 over the last month. But lots in release 44 versions and significant crashes in 46 aurora. 
I wonder what's different here between 46 and 45?
Ranking crashes on Aurora is a difficult thing. The population is skewed by the large number of Amazon Kindle Fire users and the relatively small number of users. One user crashing often can shoot a crash signature into a topcrash on Aurora. With the following query you can see that Amazon devices dominate the signature on Aurora. https://crash-stats.mozilla.com/signature/?date=%3E%3D2016-02-19T22%3A37%3A04.129574&date=%3C2016-02-26T22%3A37%3A04.129574&version=46.0a2&signature=PR_Lock+|+mozilla%3A%3Agl%3A%3AAndroidSurfaceTexture%3A%3AAttach
You need to log in before you can comment on or make changes to this bug.