1075077 - Crash during YUV to RGB conversion from GrallocImage::GetAsSourceSurface() on FxOS v2.1

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Diego Wilson [:diego]]

The crash happens during a WebRTC H.264 full duplex call.

https://mxr.mozilla.org/mozilla-aurora/source/gfx/ycbcr/yuv_convert_arm.cpp#220

Crash stack to come.

Comment 1

[Randell Jesup [:jesup] (needinfo me)]

Note: crash is in graphics-land, though the stack will tell us more

Comment 2

[Diego Wilson [:diego]]

Attachment: Crash stack

Comment 3

[Diego Wilson [:diego]]

Attachment: Crash extra

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

All the stuff on the stack is GFX and window drawing code -> GFX

Sotaro?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

doing the coversion for GrallocImage::GetAsSourceSurface()

Called from layers code

Comment 6

[Inder]

[Blocking Requested - why for this release]:

Comment 7

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_2.0.01.04.00.114.096
Moz BuildID: 20140924000243
B2G Version: 2.1
Gecko Version: 34.0a2
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=93a99bea0b40d81bd063f7d8b1964dc1ba35ba7b
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=c896bc5d04b8c08dcddd78195901503a3578a08f
Patches: bug 1055299, bug 1068978, bug 1065511, bug 1068394, bug 1067205, bug 1070431, bug 1073088, bug 1059573, bug 1051636, bug 1051633, bug 1063568

Comment 8

[bhavana bajaj [:bajaj]]

CAF mentioned they are seeing this very frequently during their stability testing so blocking on this.

Sotaro, please flag Tapas if you need any more information that will help with investigation here.

Comment 9

[Peter Chang[:pchang]]

Diego, are you able to check the patch of bug 1060811 to see this issue is still happened or not?

Comment 10

[Diego Wilson [:diego]]

This issue is no longer reproducible on the latest v2.1 gecko. Mystery fix?

Comment 11

[Diego Wilson [:diego]]

Woops! Looks like this issue is still reproducible in the latest v2.1 gecko/gaia. It may be a little more random than I thought.

I even applied the patch in bug 1060811 but it didn't fix it.

Comment 12

[Diego Wilson [:diego]]

Peter,

Do you have any other suggestions?

Comment 13

[Diego Wilson [:diego]]

STR:

1. Open https://apprtc.appspot.com on the Firefox Desktop Browser (needs a webcam connected).

2. Open the address displayed at the bottom "waiting for someone to join..." on the FFOS v2.1 phone.

3. After the call initiates, drag down the notification tray and select the settings button.

4. Disable wifi from settings.

5. After the call freezes re-enable wifi from settings.

6. Now go back to the apprtc app by either swiping or long press.

Result: apprtc crash 3/5 times with the crash stack in attachment 8497750 [details]

Comment 14

[Peter Chang[:pchang]]

(In reply to Diego Wilson [:diego] from comment #13)
> STR:
> 
> 1. Open https://apprtc.appspot.com on the Firefox Desktop Browser (needs a
> webcam connected).
> 
> 2. Open the address displayed at the bottom "waiting for someone to join..."
> on the FFOS v2.1 phone.
> 
> 3. After the call initiates, drag down the notification tray and select the
> settings button.
> 
> 4. Disable wifi from settings.
> 
> 5. After the call freezes re-enable wifi from settings.
> 
> 6. Now go back to the apprtc app by either swiping or long press.
> 
> Result: apprtc crash 3/5 times with the crash stack in attachment 8497750 [details]
> [details]

I still try to prepare environment to reproduce this issue on my side.

Based on the call stack and STR from comment 13, is it possible the image got from [1] became invalid because the WebRTC tried to deinitialize remote session when wifi was disabled?

[1]http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.cpp?rev=57e058be11ff#311

Comment 15

[Peter Chang[:pchang]]

Is there any module try to set the null image by calling [1]?

[1]http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.cpp?rev=57e058be11ff#207

Comment 16

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

Assigning to Peter as he's currently following up, so that it doesn't show up in the unassigned bugs query.

Comment 17

[bhargavg1]

(In reply to Milan Sreckovic [:milan] from comment #16)
> Assigning to Peter as he's currently following up, so that it doesn't show
> up in the unassigned bugs query.

any update on this, we are seeing consistently in our stability runs?

Comment 18

[Ken Chang[:kenkai]]

Hi Peter, please provide what you find for this bug. Thanks.

Comment 19

[Peter Chang[:pchang]]

Attachment: bug_1075077.patch

Diego, could you help to apply this patch to see this crash still happens or not?

I confirmed the buffer(graphic buffer) for remote session was still hold in content side but it caused crash when tried to convert the buffer content to RGB. Not sure the fd of this graphic buffer was closed by someone or caused by other root cause. By disabling graphic buffer allocation for WebRTC, I couldn't reproduce this problem now.

Comment 20

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang][:peter] from comment #15)
> Is there any module try to set the null image by calling [1]?
> 
> [1]http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ImageContainer.
> cpp?rev=57e058be11ff#207

nullptr to ImageContainer::SetCurrentImage(Image *aImage) is a valid argument. Some codes set null pointer via VideoFrameContainer.

Comment 21

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang][:peter] from comment #19)
> Created attachment 8500357 [details] [diff] [review]
> bug_1075077.patch
> 
> Diego, could you help to apply this patch to see this crash still happens or
> not?
> 
> I confirmed the buffer(graphic buffer) for remote session was still hold in
> content side but it caused crash when tried to convert the buffer content to
> RGB. Not sure the fd of this graphic buffer was closed by someone or caused
> by other root cause. By disabling graphic buffer allocation for WebRTC, I
> couldn't reproduce this problem now.

I am not sure about an intent of attachment 8500357 [details] [diff] [review]. It does not actually fix the cause of the problem.

Comment 22

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #21)
> (In reply to peter chang[:pchang][:peter] from comment #19)
> > Created attachment 8500357 [details] [diff] [review]
> > bug_1075077.patch
> > 
> > Diego, could you help to apply this patch to see this crash still happens or
> > not?
> > 
> > I confirmed the buffer(graphic buffer) for remote session was still hold in
> > content side but it caused crash when tried to convert the buffer content to
> > RGB. Not sure the fd of this graphic buffer was closed by someone or caused
> > by other root cause. By disabling graphic buffer allocation for WebRTC, I
> > couldn't reproduce this problem now.
> 
> I am not sure about an intent of attachment 8500357 [details] [diff] [review]
> [review]. It does not actually fix the cause of the problem.
As I mentioned in comment 19, I suspect the crash of accessing graphic buffer via ConvertYCbCrToRGB is caused by fd of this graphic buffer was closed by others.

The attachment 8500357 [details] [diff] [review] doesn't fix the problem but tries to avoid this crash by disable graphic buffer allocation for WebRTC. Since this is a CAF issue and the due day is very close, therefore, I would suggest to land this patch first and then I could continue working to find the real root cause. Sotaro, do you have any suggestion or comment?

(In reply to bhargavg1 from comment #17)
> (In reply to Milan Sreckovic [:milan] from comment #16)
> > Assigning to Peter as he's currently following up, so that it doesn't show
> > up in the unassigned bugs query.
> 
> any update on this, we are seeing consistently in our stability runs?
bhargavg1, is there anyone could help to verify attachment 8500357 [details] [diff] [review]?

Comment 23

[bhargavg1]

(In reply to peter chang[:pchang][:peter] from comment #22)
> bhargavg1, is there anyone could help to verify attachment 8500357 [details] [diff] [review]

Passing the ni on to Diego

Comment 24

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang][:peter] from comment #22)
> (In reply to Sotaro Ikeda [:sotaro] from comment #21)
> > (In reply to peter chang[:pchang][:peter] from comment #19)
> > > Created attachment 8500357 [details] [diff] [review]
> > > bug_1075077.patch
> > > 
> > > Diego, could you help to apply this patch to see this crash still happens or
> > > not?
> > > 
> > > I confirmed the buffer(graphic buffer) for remote session was still hold in
> > > content side but it caused crash when tried to convert the buffer content to
> > > RGB. Not sure the fd of this graphic buffer was closed by someone or caused
> > > by other root cause. By disabling graphic buffer allocation for WebRTC, I
> > > couldn't reproduce this problem now.
> > 
> > I am not sure about an intent of attachment 8500357 [details] [diff] [review]
> > [review]. It does not actually fix the cause of the problem.
> As I mentioned in comment 19, I suspect the crash of accessing graphic
> buffer via ConvertYCbCrToRGB is caused by fd of this graphic buffer was
> closed by others.
> 
> The attachment 8500357 [details] [diff] [review] doesn't fix the problem but
> tries to avoid this crash by disable graphic buffer allocation for WebRTC.
> Since this is a CAF issue and the due day is very close, therefore, I would
> suggest to land this patch first and then I could continue working to find
> the real root cause. Sotaro, do you have any suggestion or comment?
> 

To improve the b2g quality, we need to analyze and fix the problem. GrallocImage holds GrallocTextureClientOGL and GrallocTextureClientOGL hold android::GraphicBuffer. Therefore, it code works correctly "fd of this graphic buffer was closed by others" should not happen. If it happens, it could be like Bug 1057220. If it happens on b2g, we have to fix it. Otherwise, it could cause other problems.

Comment 25

[Sotaro Ikeda [:sotaro]]

I could reproduce the problem in Comment 13. I am going to investigate about it.

Comment 26

[Sotaro Ikeda [:sotaro]]

The problem seems to happen because ConvertOmxYUVFormatToRGB565() uses stale y,cb,cr buffers' addresses.

Comment 27

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update buffer's addresses

The patch is created based on attachment 8482460 [details] [diff] [review] in Bug 1060811.

Comment 29

[Sotaro Ikeda [:sotaro]]

diego, can you check if attachment 8501221 [details] [diff] [review] works?

Comment 30

[Diego Wilson [:diego]]

(In reply to Sotaro Ikeda [:sotaro] from comment #29)
> diego, can you check if attachment 8501221 [details] [diff] [review] works?

Sotaro,

Can you please share a patch that applies cleanly to v2.1?

Comment 31

[Vincent Liu[:vliu]]

Attachment: patch_1075077_1-v2.1

(In reply to Diego Wilson [:diego] from comment #30)
> (In reply to Sotaro Ikeda [:sotaro] from comment #29)
> > diego, can you check if attachment 8501221 [details] [diff] [review] works?
> 
> Sotaro,
> 
> Can you please share a patch that applies cleanly to v2.1?

It seems the code conflict happens without the patch of bug 1060811. The attached file was attached including this part.

Comment 32

[Peter Chang[:pchang]]

Attachment: WIP

Sotaro, after checking your patch, I think it is better not to cache the yuv address from source image. With this patch, I couldn't reproduce the crash anymore.

Comment 33

[Peter Chang[:pchang]]

Diego, I think you can apply attachment 8501497 [details] [diff] [review] to verify the crash. It almost did the same thing with attachment 8501481 [details].

Comment 34

[Ken Chang[:kenkai]]

ni?me for reminding.

Comment 35

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #27)
> Created attachment 8501221 [details] [diff] [review]
> patch - Update buffer's addresses
> 
> The patch is created based on attachment 8482460 [details] [diff] [review]
> in Bug 1060811.
I would prefer not to include the patch of bug 1079251 in this bug because it belongs to 2.1.

Comment 36

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8501497 [details] [diff] [review]
WIP

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

I do not fun of caching gralloc buffer address when gralloc buffer is not locked.

The patch should work in flame device, because codeaurora's gralloc hal map gralloc buffer when allocated since JB. But from android::GraphicBuffer's api's definition, gralloc buffer could be mapped(assigned address) only when it is locked.

Comment 37

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v2.1 - Update buffer's addresses

Modify attachment 8501221 [details] [diff] [review] for b2g v2.1. attachment 8501221 [details] [diff] [review] is not cleanly applied to b2gv2.1. But it is soon uplifted to b2gv2.1.

Comment 38

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update buffer's addresses

Update nits.

Comment 39

[Peter Chang[:pchang]]

(In reply to Sotaro Ikeda [:sotaro] from comment #36)
> Comment on attachment 8501497 [details] [diff] [review]
> WIP
> 
> Review of attachment 8501497 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I do not fun of caching gralloc buffer address when gralloc buffer is not
> locked.
> 
> The patch should work in flame device, because codeaurora's gralloc hal map
> gralloc buffer when allocated since JB. But from android::GraphicBuffer's
> api's definition, gralloc buffer could be mapped(assigned address) only when
> it is locked.

Good point, I forgot to think about the timing for buffer locking.

Comment 40

[Peter Chang[:pchang]]

Sotaro, I just switch owner to you since you already have patch to review.

Comment 41

[Sotaro Ikeda [:sotaro]]

Diego, can you test if attachment 8501737 [details] [diff] [review] works? The peter's patch seems also have same effect on codearura's platform.

Comment 42

[Sotaro Ikeda [:sotaro]]

(In reply to peter chang[:pchang][:peter] from comment #40)
> Sotaro, I just switch owner to you since you already have patch to review.

Okey, I take this bug, thanks.

Comment 43

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update buffer's addresses

Update a comment.

Comment 44

[Sotaro Ikeda [:sotaro]]

nical, can you review the patch soon? This bug need to be fixed very soon. thanks!

Comment 45

[Sotaro Ikeda [:sotaro]]

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

Comment 46

[Nicolas Silva [:nical]]

Comment on attachment 8501746 [details] [diff] [review]
patch - Update buffer's addresses

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

Looks like something is missing (or I didn't quite understand the patch).

::: gfx/layers/GrallocImages.cpp
@@ +71,5 @@
> +  // gralloc hal could map gralloc buffer only when the buffer is locked,
> +  // though some gralloc hals implementation maps it when it is allocated.
> +  mData.mYChannel     = 0;
> +  mData.mCrChannel    = 0;
> +  mData.mCbChannel    = 0;

A few dozen lines below we use these variables with memcpy. Perhaps you want to null those members after the memcpy calls?

Comment 47

[Sotaro Ikeda [:sotaro]]

Sorry, the patch was incorrect. I am going to update the patch soon.

Comment 48

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update buffer's addresses

fix based on the comment.

Comment 49

[Sotaro Ikeda [:sotaro]]

I updated the patch. Can you review again? Thanks!

Comment 50

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v2.1 - Update buffer's addresses

Fix incorrect previous patch.

Comment 51

[Sotaro Ikeda [:sotaro]]

(In reply to Nicolas Silva [:nical] from comment #46)
> Comment on attachment 8501746 [details] [diff] [review]
> patch - Update buffer's addresses
> 
> Review of attachment 8501746 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks like something is missing (or I didn't quite understand the patch).
> 
> ::: gfx/layers/GrallocImages.cpp
> @@ +71,5 @@
> > +  // gralloc hal could map gralloc buffer only when the buffer is locked,
> > +  // though some gralloc hals implementation maps it when it is allocated.
> > +  mData.mYChannel     = 0;
> > +  mData.mCrChannel    = 0;
> > +  mData.mCbChannel    = 0;
> 
> A few dozen lines below we use these variables with memcpy. Perhaps you want
> to null those members after the memcpy calls?

This happens because current GrallocImage::SetData(const Data& aData) is incorrect. mData.mYChannel should not be used as data source. It should be a destination's address. It might be better to be handled as a different bug.

Comment 52

[Sotaro Ikeda [:sotaro]]

Bug 1079251 is removed from dup bug. It seems better to handle as a different bug.

Comment 53

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8501779 [details] [diff] [review]
patch - Update buffer's addresses

I am going to remove Bug 1079251 related parts.

Comment 54

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2gv2.1 - Update buffer's addresses

Remove Bug 1079251 related part.

Comment 55

[Sotaro Ikeda [:sotaro]]

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

Comment 56

[Sotaro Ikeda [:sotaro]]

I am going to check if attachment 8501793 [details] [diff] [review] fixes the problem.

Comment 57

[Sotaro Ikeda [:sotaro]]

Attachment: patch for b2g v2.1 - Update buffer's addresses

Fix a mistake.

Comment 58

[Sotaro Ikeda [:sotaro]]

diego, can you check if attachment 8501888 [details] [diff] [review] fixes the problem?

Comment 59

[Diego Wilson [:diego]]

Comment on attachment 8501888 [details] [diff] [review]
patch for b2g v2.1 - Update buffer's addresses

Yep, I can't reproduce the crash with this patch. Thanks

Comment 60

[Sotaro Ikeda [:sotaro]]

Good! Thanks for the confirmation :-)

Comment 61

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8501888 [details] [diff] [review]
patch for b2g v2.1 - Update buffer's addresses

v2.1 specific patch is not necessary. Bug 1075077 fix was already uplifted to v2.1.

Comment 62

[Sotaro Ikeda [:sotaro]]

Attachment: patch - Update buffer's addresses

Un-bitrot. Carry "r=nical".

Comment 63

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8501808 [details] [diff] [review]
patch for b2gv2.1 - Update buffer's addresses

By bug 1060811 uplift to b2g v2.1. The patch becomes a patch for b2gv2.1.

Comment 64

[Sotaro Ikeda [:sotaro]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e0662f559c63

Comment 65

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 66

[Ken Chang[:kenkai]]

Fixed.

Comment 67

[Diego Wilson [:diego]]

Sotaro,

Is attachment 8501808 [details] [diff] [review] still pending a v2.1 uplift then?

Comment 68

[cafbot (PoC: ggrisco)]

Observed on: 

Device: msm8226
Gonk Version: AU_LINUX_GECKO_B2G_KK_2.0.01.04.00.114.108
Moz BuildID: 20141011000201
B2G Version: 2.1
Gecko Version: 34.0a2
Gaia:  http://git.mozilla.org/?p=releases/gaia.git;a=commit;h=f5d4ff60ffed8961f7d0380ada9d0facfdfd56b1
Gecko: http://git.mozilla.org/?p=releases/gecko.git;a=commit;h=db7fce920e7d782d9f601384dc95924abcdaeeb8
Patches: bug 1070431, bug 1073419, bug 1042357, bug 1076327, bug 1074419

Comment 69

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8502520 [details] [diff] [review]
patch - Update buffer's addresses

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:

Approval Request Comment
[Feature/regressing bug #]:none
[User impact if declined]:Application that using WebRTC could cause crash.
[Describe test coverage new/current, TBPL]:locally tested.
[Risks and why]: low.
[String/UUID change made/needed]:

Comment 70

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/a41c0b832692