939647 - Launch Video app causes crash based on Android JB (4.2.2)

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Video, defect)

Description

[Vincent Liu[:vliu]]

STR to reproduce this issue.

1. Launches video app
2. Video crashes.

Comment 1

[Vincent Liu[:vliu]]

Attachment: bug-939647-fix.patch

It should be the missing part when the patch of bug 922510 is landed. I Checked SurfaceTextureClient::dequeueBuffer(...) on Android JB(4.2.2) and its shouldn't call |fence->isValid()| instead of |fence.get()|.

Comment 2

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8333639 [details] [diff] [review]
bug-939647-fix.patch

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

::: widget/gonk/nativewindow/GonkNativeWindowClientJB.cpp
@@ +221,2 @@
>      if (fence->isValid()) {
> +#endif

I want to prevent ifdef as less as possible. If the code is like following, we do not need ifdef.

>   if (fence.get() && fence->isValid()) {

Comment 3

[Vincent Liu[:vliu]]

Attachment: bug-939647-v2-fix.patch

(In reply to Sotaro Ikeda [:sotaro] from comment #2)
> Comment on attachment 8333639 [details] [diff] [review]
> bug-939647-fix.patch
> 
> Review of attachment 8333639 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gonk/nativewindow/GonkNativeWindowClientJB.cpp
> @@ +221,2 @@
> >      if (fence->isValid()) {
> > +#endif
> 
> I want to prevent ifdef as less as possible. If the code is like following,
> we do not need ifdef.
> 
> >   if (fence.get() && fence->isValid()) {

Cool! It seems the suggestion is better then my first patch. Can you please review again? Thanks.

Comment 4

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8333668 [details] [diff] [review]
bug-939647-v2-fix.patch

Looks good. It seems better to ask a review also to mwu. I add mwu as a reviewer.

Comment 5

[Michael Wu [:mwu]]

Comment on attachment 8333668 [details] [diff] [review]
bug-939647-v2-fix.patch

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

Looks reasonable.

Comment 6

[Vincent Liu[:vliu]]

The result of try-server

https://tbpl.mozilla.org/?tree=Try&rev=60102d715e28
https://tbpl.mozilla.org/?tree=Try&rev=1247a84be328

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/facc5fa097c8

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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