823236 - crash in mozilla::layers::ShmemYCbCrImage::IsValid on ICS and above

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Scoobidiver (away)]

It's #19 top crasher in 19.0a2, #18 in 20.0a1 and first showed up in 19.0a1/20121110.

Signature 	mozilla::layers::ShmemYCbCrImage::IsValid() More Reports Search
UUID	415d3d68-3984-4f68-93ff-9f5b22121216
Date Processed	2012-12-16 11:36:29
Uptime	710
Install Age	11.8 minutes since version was first installed.
Install Time	2012-12-16 11:24:28
Product	FennecAndroid
Version	20.0a1
Build ID	20121215030840
Release Channel	nightly
OS	Android
OS Version	0.0.0 Linux 3.0.15-CM-gf76e4eb #1 SMP PREEMPT Wed Dec 12 10:00:19 CET 2012 armv7l samsung/GT-N7000/GT-N7000:4.0.3/IML74K/ZCLP6:user/release-keys
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x52105000
App Notes 	
AdapterDescription: 'ARM -- Mali-400 MP -- OpenGL ES 2.0 -- Model: GT-N7000, Product: GT-N7000, Manufacturer: samsung, Hardware: smdk4210'
EGL? EGL+ GL Context? GL Context+ GL Layers? GL Layers+ 
samsung GT-N7000
samsung/GT-N7000/GT-N7000:4.0.3/IML74K/ZCLP6:user/release-keys
Processor Notes 	Priority Job; /data/socorro/stackwalk/bin/exploitable: ERROR: unable to analyze dump
EMCheckCompatibility	True
Adapter Vendor ID	ARM
Adapter Device ID	Mali-400 MP
Device	samsung GT-N7000
Android API Version	16 (REL)
Android CPU ABI	armeabi-v7a

Frame 	Module 	Signature 	Source
0 	libxul.so 	mozilla::layers::ShmemYCbCrImage::IsValid 	ShmemYCbCrImage.cpp:145
1 	libxul.so 	mozilla::layers::ShmemYCbCrImage::Open 	ShmemYCbCrImage.cpp:101
2 	libxul.so 	mozilla::layers::ImageContainerChild::GetSharedImageFor 	ShmemYCbCrImage.h:39
3 	libxul.so 	mozilla::layers::ImageContainerChild::SendImageNow 	ImageContainerChild.cpp:314
4 	libxul.so 	mozilla::layers::ImageBridgeCopyAndSendTask::Run 	ImageContainerChild.cpp:344
5 	libxul.so 	MessageLoop::RunTask 	message_loop.cc:333
6 	libxul.so 	MessageLoop::DeferOrRunPendingTask 	message_loop.cc:341
7 	libxul.so 	MessageLoop::DoWork 	message_loop.cc:441
8 	libxul.so 	base::MessagePumpDefault::Run 	message_pump_default.cc:23
9 	libxul.so 	MessageLoop::RunInternal 	message_loop.cc:215
10 	libxul.so 	MessageLoop::Run 	message_loop.cc:208
11 	libxul.so 	base::Thread::ThreadMain 	thread.cc:156
12 	libxul.so 	ThreadFunc 	platform_thread_posix.cc:39
13 	libc.so 	libc.so@0x12c96 	
14 	libc.so 	libc.so@0x123ee

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Alayers%3A%3AShmemYCbCrImage%3A%3AIsValid%28%29

Comment 1

[Scoobidiver (away)]

It's #8 top crasher in 19.0b1.

Comment 2

[Robert Kaiser]

Scoobidiver, let's be careful with setting topcrash keywords when we have so little data as we do right now on 19.0b1 (we don't even have data from yesterday due to a Socorro bug I still need to file).

Comment 3

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #2)
> Scoobidiver, let's be careful with setting topcrash keywords when we have so
> little data as we do right now on 19.0b1.
Beta 1 was released more than 2 days ago. It's 30% of its short life. In addition, there are few duplicates.

> (we don't even have data from yesterday due to a Socorro bug I still need to file)
I don't see a hole in crashes. Maybe it's only ADU that is missing.

Comment 4

[Robert Kaiser]

(In reply to Scoobidiver from comment #3)
> > (we don't even have data from yesterday due to a Socorro bug I still need to file)
> I don't see a hole in crashes. Maybe it's only ADU that is missing.

No, all aggregations are missing, including the ones in the topcrash lists, AFAIK.

Comment 5

[Scoobidiver (away)]

(In reply to Robert Kaiser (:[e-mail]) from comment #4)
> No, all aggregations are missing, including the ones in the topcrash lists,
> AFAIK.
I usually use the real-time query (https://crash-stats.mozilla.com/query/query?product=FennecAndroid&version=FennecAndroid%3A19.0b1&do_query=1), not the topcrasher one built once a day at 9H UTC (https://crash-stats.mozilla.com/topcrasher/byversion/FennecAndroid/19.0b1) which seems right.

Comment 6

[Robert Kaiser]

(In reply to Scoobidiver from comment #5)
> I usually use the real-time query

OK, that puts a lot more load on the servers, but OTOH is more current and doesn't run into the problems of aggregations missing, so your assessment here is probably good (to get back to the actual topic of this bug). :)

Comment 7

[Alex Keybl [:akeybl]]

KaiRo - can you find URLs and correlations for this bug, so that Aaron can attempt to reproduce?

Milan - can you help find an assignee for this bug?

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Bas, is it too much to hope that this shmem issue, on Android, is somehow helped by the shmem issue you had to deal with on Windows?

Comment 9

[Robert Kaiser]

URLs:
1 	http://theweek.com/article/index/238410/4-reasons-the-government-wont-mint-a-trillion-dollar-coin-to-prevent-a-debt-ceiling-crisis
1 	http://www.replaytambayan.com/2013/01/a-beautiful-affair-january-11-2013.html
1 	http://jeffwalkerpresents.kajabi.com/fe/39192-jeff-walker-presents-brendon-burch
plus an adult page.

Those few could point to Flash being involved. Which wouldn't surprise me as it seems to be the mother of all mobile instability, or something close to that.

We don't have correlations for mobile.

Comment 10

[Bas Schouten (:bas.schouten)]

(In reply to Milan Sreckovic [:milan] from comment #8)
> Bas, is it too much to hope that this shmem issue, on Android, is somehow
> helped by the shmem issue you had to deal with on Windows?

It would be incredibly unlikely I'm afraid.

Comment 11

[Robert Kaiser]

This is the #4 topcrash on 19.0b2 now.

Comment 12

[Milan Sreckovic [:milan] (needinfo for best results)]

BenWa, can you take a look?  bjacob, Jeff and Joe are all chasing high priority issues at this time.

Comment 13

[Benoit Girard (:BenWa)]

This sounds a bit like the code nical is familiar with. Any ideas nical?

Comment 14

[Nicolas Silva [:nical]]

It could be that the shmem has been destroyed somewhere else and sent back to the child side.

I need to look into a few more things before I can say more about this.

Comment 15

[bhavana bajaj [:bajaj]]

(In reply to Nicolas Silva [:nical] from comment #14)
> It could be that the shmem has been destroyed somewhere else and sent back
> to the child side.
> 
> I need to look into a few more things before I can say more about this.

Hi Nicolas, do we have any updates here ? This is #4 on our top-crash list for Fennecandroid as of today and we are very close to FF19's Release with 19.0b4 going to build tomorrow.Any help here to get this resolved within FF19 timeframe will be great .Thanks !

Comment 16

[Nicolas Silva [:nical]]

I am very busy with the layers refactoring and its approaching deadlines, so it would be best if someone else can take this bug.

What I see is that ShmemYCbCr and Shmem are allocated on the stack so the segfault should be in accessing the shmem's buffer (not on the stack), but since we check for the shmem's buffer size (stored on the stack in Shmem) before touching the data, it looks like we are doing something silly like deallocating the shmem buffer on the parent process and sending it back to the child side. there maybe some methods of shmem that we could use, like isValid or isAllocated or something like this, i don't know. It is worth looking at what's going on in ImageBridgeParent and ImageContainerParent and see if we are passing around the Shmem by copy in places where we should be passing it by reference.

That would be my guess but I don't have time to reproduce and work on that at the moment, sorry.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

Putting Matt on the list in case BenWa comes up with something that could be continued in the other timezone and expedite this.

Comment 18

[Benoit Girard (:BenWa)]

This may be related to bug 792252.

I can't find a reproducible test case. It would likely have to do with HTML5 video.

Comment 19

[Benoit Girard (:BenWa)]

I got ted to pull up register info from a random crash report:
https://crash-stats.mozilla.com/report/index/7ee7bca1-853c-4707-804e-3e9452130130

Thread 26 (crashed)
 0  libxul.so + 0xbc2674
     r4 = 0x00000000    r5 = 0x0002fd1c    r6 = 0x648701c4    r7 = 0x60effcf4
     r8 = 0x659c0a80    r9 = 0x64870190   r10 = 0x659c0aa8    fp = 0x60effd08
     sp = 0x60effcc0    lr = 0x6224b6bd    pc = 0x6224b674

Comment 20

[Benoit Girard (:BenWa)]

$ wget ftp://ftp.mozilla.org/pub/mobile/candidates/19.0b3-candidates/build1/android/en-US/fennec-19.0b3.en-US.android-arm.apk
$ unzip fennec-19.0b3.en-US.android-arm.apk
$ arm-eabi-objdump -d libxul.so | grep -C 100 bc2674

  bc2640:	3403      	adds	r4, #3
  bc2642:	f024 0403 	bic.w	r4, r4, #3	; 0x3
  bc2646:	191b      	adds	r3, r3, r4
  bc2648:	6083      	str	r3, [r0, #8]
  bc264a:	680b      	ldr	r3, [r1, #0]
  bc264c:	60c3      	str	r3, [r0, #12]
  bc264e:	684b      	ldr	r3, [r1, #4]
  bc2650:	6103      	str	r3, [r0, #16]
  bc2652:	6813      	ldr	r3, [r2, #0]
  bc2654:	6143      	str	r3, [r0, #20]
  bc2656:	6853      	ldr	r3, [r2, #4]
  bc2658:	6183      	str	r3, [r0, #24]
  bc265a:	bd30      	pop	{r4, r5, pc}
  bc265c:	b530      	push	{r4, r5, lr}
  bc265e:	6803      	ldr	r3, [r0, #0]
  bc2660:	b085      	sub	sp, #20
  bc2662:	b90b      	cbnz	r3, bc2668 <NS_InvokeByIndex_P+0x82b80>
  bc2664:	68c3      	ldr	r3, [r0, #12]
  bc2666:	b1d3      	cbz	r3, bc269e <NS_InvokeByIndex_P+0x82bb6>
  bc2668:	6885      	ldr	r5, [r0, #8]
  bc266a:	2d1b      	cmp	r5, #27
  bc266c:	d917      	bls.n	bc269e <NS_InvokeByIndex_P+0x82bb6>
  bc266e:	6904      	ldr	r4, [r0, #16]
  bc2670:	6842      	ldr	r2, [r0, #4]
  bc2672:	1913      	adds	r3, r2, r4
> bc2674:	5912      	ldr	r2, [r2, r4]
  bc2676:	2a1c      	cmp	r2, #28
  bc2678:	d111      	bne.n	bc269e <NS_InvokeByIndex_P+0x82bb6>

Comment 21

[Benoit Girard (:BenWa)]

Added missing registers:
    Crash reason:  SIGSEGV
    Crash address: 0x5c668000
     
    Thread 26 (crashed)
     0  libxul.so + 0xbc2674
         r0 = 0x60effcf4    r1 = 0x60fb9970    r2 = 0x5c668000    r3 = 0x5c668000
         r4 = 0x00000000    r5 = 0x0002fd1c    r6 = 0x648701c4    r7 = 0x60effcf4
         r8 = 0x659c0a80    r9 = 0x64870190   r10 = 0x659c0aa8    fp = 0x60effd08
         sp = 0x60effcc0    lr = 0x6224b6bd    pc = 0x6224b674
        Found by: given as instruction pointer in context

Comment 22

[Benoit Girard (:BenWa)]

The crash is trying to dereference mYOffset in 'GetYCbCrBufferInfo(mShmem, mOffset)->mYOffset'. Right now my theory is that we either have an invalid image getting into the poll somehow.

With this theory in mind this code seems to fit this description:
http://hg.mozilla.org/mozilla-central/annotate/cbeab7da0e3a/gfx/layers/ipc/ImageContainerChild.cpp#l133

If we're stopping we inserting something into the pool and we release it. If we try to send that image before we close the image bridge we risk crashing.

Comment 23

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 24

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=04d2caaba7a7

Comment 25

[Nicolas Silva [:nical]]

The first thing AddSharedImageToPool does is check for mStop, so

if (!AddSharedImageToPool(img) || mStop)
if (mStop || !AddSharedImageToPool(img))
and
if (!AddSharedImageToPool(img))

are actually equivalent. It's probably worth removing the misleading mStop here, even though it won't change anything.

One thing that has been worrying me for a while is the way we push the images we send in an image queue, and we pop the image queue when we receive something back (in order to maintain ref counting). If for some reason we deallocate one image on the compositor process or the images come back in the wrong order it could lead to a crash like this.

Maybe you could store a unique id each the SharedImage that is sent and in the image queue to check that it matches just to validate or invalidate this theory (if you can reproduce the crash).

Comment 26

[Benoit Girard (:BenWa)]

Comment on attachment 708801 [details] [diff] [review]
patch

You're right. This mStop isn't the problem.

Comment 27

[Benoit Girard (:BenWa)]

Attachment: Sanity check

Well let's start by taking this in. This will tell us if an image interesting the poll is invalid and catch the error earlier much closer to the source.

Comment 28

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3c4205be7191

Comment 29

[Benoit Girard (:BenWa)]

The crash reproduces on inbound.
https://hg.mozilla.org/integration/mozilla-inbound/rev/60d155f8b54b

Comment 30

[Alex Keybl [:akeybl]]

Comment on attachment 709273 [details] [diff] [review]
Sanity check

Pre-approving for uplift to Aurora, given the fact that we're still considering a fix for this FF19 regression, for FF19.

Comment 31

[Benoit Girard (:BenWa)]

(In reply to Alex Keybl [:akeybl] from comment #30)
> Comment on attachment 709273 [details] [diff] [review]
> Sanity check
> 
> Pre-approving for uplift to Aurora, given the fact that we're still
> considering a fix for this FF19 regression, for FF19.

I wont be uplifting this because our test cause the assertion I added. This is wonderful news because it's mean that we can catch the problem earlier and reproduce this on try.

Comment 32

[Alex Keybl [:akeybl]]

Comment on attachment 709273 [details] [diff] [review]
Sanity check

Whoops sorry Benoit, clearing the flag

Comment 33

[Benoit Girard (:BenWa)]

Alright here is the bug:
1) After some time we call SetIdleNow() which calls ClearSharedImagePool:
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ImageContainerChild.cpp#306
2) This deallocs all the shmem
3) At some point in the near future the main thread release the last reference to SharedPlanarYCbCrImage which in it's destructor tries to recycle it's shared image for which we forced a deallocation in step 2

To me it's appears that the lifetime of the Shmem are very poorly defined. We use ref counted SharedImage to manage them but sometimes we explicitly delete them regardless of how its state or reference count.

Perhaps I'm just misunderstanding their lifetimes but the code makes it unnecessarily complicated.

Comment 34

[Nicolas Silva [:nical]]

If a shmem that is in the pool is used by a SharedPlanarYCbCrImage, it is a bug (I'd be a bit surprised though).

on the other hand, a shmem that is in the queue can be use by a SharedPlanarYCbCrImage since the queue is only meant to keep the refcount > 0 while the Shmem is only used/referenced in another process.

What I intended to do originally, was to send a uintptr_t in SharedImage that would be equal to the address of the SharedPlanarYCbCrImage/gralloc and manually addref/release on sending/receiving the data. it's not pretty but it would make it work if for some silly reason we receive back the image in a different order than what we sent (unlikely but it is hard to prove that it will never happen). It turned out that the queue hack was implemented by someone else before i did the uintptr_t hack.

My gut feeling is that the queue (not the pool) is likely to be related to the crash here, but it's because it's the part of async video that seems the most fragile to me (I can't prove that it is not correct though).

Comment 35

[Benoit Girard (:BenWa)]

Pushed my latest theory to try:
https://tbpl.mozilla.org/?tree=Try&rev=037ef408cee8

Comment 36

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=543c165ff223

Comment 37

[Benoit Girard (:BenWa)]

Attachment: patch

Comment 38

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/38b92d3080b9

Comment 39

[Benoit Girard (:BenWa)]

Comment on attachment 711398 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): unknown
User impact if declined: potential crash when stopping/closing video
Testing completed (on m-c, etc.): on inbound and fixed some assertion failures
Risk to taking this patch (and alternatives if risky): Low. Removed 1 line that was clearly wrong as confirmed by myself and nical.
String or UUID changes made by this patch: none

I think we should co-land this on aurora to see if it lowers the crash volume. If we get positive results we can uplift to beta.

Comment 40

[bhavana bajaj [:bajaj]]

Comment on attachment 711398 [details] [diff] [review]
patch

Low risk risk for a top-crasher.Approving on aurora.

Comment 41

[Robert Kaiser]

Here's some stats on the 19.0 beta crashes and number of installations:

 date       | crashes | installations 
------------+---------+---------------
 2013-02-05 |      82 |            62
 2013-02-06 |      70 |            53
 2013-02-07 |      89 |            66

Used queries to find this were as follows:
SELECT version,COUNT(*) as crashes,COUNT(DISTINCT client_crash_date - install_age  * interval '1 second') as installations FROM reports WHERE product='FennecAndroid' AND signature='mozilla::layers::ShmemYCbCrImage::IsValid()' AND utc_day_is(date_processed, '2013-02-07') GROUP BY version;

Comment 42

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/38b92d3080b9

Comment 43

[Scoobidiver (away)]

There are still crashes after the patch landed.

Comment 44

[Benoit Girard (:BenWa)]

Yes indeed. I can spend a bit more time trying to debug the problem but without reproducing dropping patches is just running out the clock. I imagine the patch landed in Comment 42 helps but it not sufficient.

I think the safest solution is to disable image recycling on everywhere to stop the crashing and do a proper, i.e. invasive, fix of how we recycle images on central and let it ride the trains. I need to do a bit of testing to see how big the performance is for not recycling images. Note this can only be done for Fennec from what I hear because gralloc buffers likely need to be recycled.

Comment 45

[Benoit Girard (:BenWa)]

Comment on attachment 711398 [details] [diff] [review]
patch

https://hg.mozilla.org/releases/mozilla-aurora/rev/a0acbdd7d52f

Comment 46

[Alex Keybl [:akeybl]]

While it's a shame to take this regression in FF19, we have run out of time. Beta 6 (our final) is going to build today. If a low risk fix is found, it'll be first present in FF20.

Comment 47

[Scoobidiver (away)]

(In reply to Alex Keybl [:akeybl] from comment #46)
> While it's a shame to take this regression in FF19, we have run out of time.
> Beta 6 (our final) is going to build today. If a low risk fix is found,
> it'll be first present in FF20.
Backing out bug 792252 would be a safe fix for Beta 6. Isn't it?

Comment 48

[Benoit Girard (:BenWa)]

bug 792252 is only a suspect change. I can't say with certitude that it's caused by that bug.

Comment 49

[Lukas Blakk [:lsblakk] use ?needinfo]

Benoit - in comment 44 you mention disabling image recycling - is that something you can put together soon so we can get testing around it in an early FF20 beta?

Comment 50

[Benoit Girard (:BenWa)]

(In reply to Lukas Blakk [:lsblakk] from comment #49)
> Benoit - in comment 44 you mention disabling image recycling - is that
> something you can put together soon so we can get testing around it in an
> early FF20 beta?

I'm working on the patch. I'll see how it goes.

Note to self (because TBPL logs expire), relevant test to look into is:
tests/content/media/test/test_bug493187.html

Comment 51

[Benoit Girard (:BenWa)]

mach.sh mochitest-plain content/media/test/test_streams_element_capture_reset.html

Thread 22 Crashed:: ImageBridgeChild
0   XUL                           	0x0000000102f0a9d1 mozilla::layers::ShmemYCbCrImage::IsValid() + 65 (ShmemYCbCrImage.cpp:146)
1   XUL                           	0x0000000102f0a989 mozilla::layers::ShmemYCbCrImage::Open(mozilla::ipc::Shmem&, unsigned long) + 41 (ShmemYCbCrImage.cpp:101)
2   XUL                           	0x0000000102f01ebd mozilla::layers::ImageContainerChild::AddSharedImageToPool(mozilla::layers::SharedImage*) + 141 (ShmemYCbCrImage.h:41)
3   XUL                           	0x0000000102f024dd mozilla::layers::ImageContainerChild::RecycleSharedImageNow(mozilla::layers::SharedImage*) + 77 (ImageContainerChild.cpp:231)
4   XUL                           	0x0000000102e4399b MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) + 107 (message_loop.cc:334)
5   XUL                           	0x0000000102e43bf7 MessageLoop::DoWork() + 295 (message_loop.cc:442)
6   XUL                           	0x0000000102e46611 base::MessagePumpDefault::Run(base::MessagePump::Delegate*) + 145 (message_pump_default.cc:23)
7   XUL                           	0x0000000102e43354 MessageLoop::Run() + 180 (message_loop.cc:503)
8   XUL                           	0x0000000102e49212 base::Thread::ThreadMain() + 194 (thread.cc:159)
9   XUL                           	0x0000000102e5213a ThreadFunc(void*) + 10 (platform_thread_posix.cc:39)
10  libsystem_c.dylib             	0x00007fff8f7ac742 _pthread_start + 327
11  libsystem_c.dylib             	0x00007fff8f799181 thread_start + 13

Comment 52

[Robert Kaiser]

Interestingly, I don't see this signature in Aurora 21 so far. On Nightly 22 it exists, and on 19.0 as well as 20.0b1 it's still pretty high.

Comment 53

[Benoit Girard (:BenWa)]

Just a quick update I've been working primarily on this bug this week and found several bugs:
- We sometimes allocate shmem twice and leak one
- We add the same shmem twice to the current pool
- a shmem appears both in the pool and the active queue (still investigating)

Comment 54

[Benoit Girard (:BenWa)]

Attachment: patch

This patch is the bare minimum I think is required to stop the crashing and thus is our best shot at uplifting to beta.

Comment 55

[Nicolas Silva [:nical]]

Comment on attachment 720151 [details] [diff] [review]
patch

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

Looks good to me except one thing:

::: gfx/layers/ipc/ImageContainerChild.cpp
@@ +472,5 @@
>      return false;
>    }
>    if (InImageBridgeChildThread()) {
>      AllocUnsafeShmem(aBufSize, aType, aShmem);
> +    return;

you need to return the result of AllocUnsafeShmem here (bool)

Comment 56

[Nicolas Silva [:nical]]

Comment on attachment 720151 [details] [diff] [review]
patch

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

No need to go through another review, please fix the return value I mentioned and land the patch.

Comment 57

[Benoit Girard (:BenWa)]

https://tbpl.mozilla.org/?tree=Try&rev=cbbc889163ed

Comment 58

[Benoit Girard (:BenWa)]

This is looking good on Try. I'm not convinced this will fix IsValid but it certainly fixes several major issues in the most beta-uplift-friendly way.

https://tbpl.mozilla.org/?tree=Try&rev=8c97bdae2c32

Comment 59

[Benoit Girard (:BenWa)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/13974e5104c9

Comment 60

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/13974e5104c9

Comment 61

[Scoobidiver (away)]

The Leave Open whiteboard was about the previous patch.

Comment 62

[Lukas Blakk [:lsblakk] use ?needinfo]

(In reply to Benoit Girard (:BenWa) from comment #58)
> This is looking good on Try. I'm not convinced this will fix IsValid but it
> certainly fixes several major issues in the most beta-uplift-friendly way.
> 
Can you nominate this for Beta uplift then? :)

Comment 63

[Benoit Girard (:BenWa)]

(In reply to Lukas Blakk [:lsblakk] from comment #62)
> (In reply to Benoit Girard (:BenWa) from comment #58)
> > This is looking good on Try. I'm not convinced this will fix IsValid but it
> > certainly fixes several major issues in the most beta-uplift-friendly way.
> > 
> Can you nominate this for Beta uplift then? :)

I wanted to wait to see the crash go away before uplifting. I'm keeping this on my radar.

Comment 64

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

I think it may need to be pushed to beta for us to really tell :

https://crash-stats.mozilla.com/report/list?product=FennecAndroid&query_search=signature&query_type=contains&query=mozilla%3A%3Alayers%3A%3AShmemYCbCrImage%3A%3AIsValid&reason_type=contains&date=03%2F06%2F2013%2017%3A32%3A50&range_value=1&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=mozilla%3A%3Alayers%3A%3AShmemYCbCrImage%3A%3AIsValid%28%29

Last crashes show on Build ID : 20130228083210

Comment 65

[Benoit Girard (:BenWa)]

The crash is low volume (1 crash per many days) on Nightly so you're right we wont really get much data. How about we let this patch bake for side effects and I'll uplift on Friday?

Comment 66

[Lukas Blakk [:lsblakk] use ?needinfo]

Since we've already gone to build on beta 3, we have no choice but to wait to get this into beta 4 (landed prior to Tues Mar 12th) so we can look at the volume on the beta channel.

Comment 67

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

:BenWa, considering that what lsblakk stated, that sounds great.  As long as it's in beta before tuesday preferably, it should be fine.  :)

Comment 68

[Benoit Girard (:BenWa)]

Comment on attachment 720151 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Async video image pool
User impact if declined: IsValid topcrasher will continue
Testing completed (on m-c, etc.): on m-c for several days. Signature hasn't been seen since and no fall out has been observed.
Risk to taking this patch (and alternatives if risky): The patch isn't without its risk but I have done local testing with custom asserts to convince myself that this patch made things better.
String or UUID changes made by this patch: none

Comment 69

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f8f786cff577
https://hg.mozilla.org/releases/mozilla-beta/rev/f3384068c5e7

Comment 70

[Benoit Girard (:BenWa)]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #69)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/f8f786cff577
> https://hg.mozilla.org/releases/mozilla-beta/rev/f3384068c5e7

Thanks!