801627 - Fullscreen Flash broken

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Plugins, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Looks like something broke fullscreen support for Flash recently. I get the page showing through underneath instead of the Flash content, similar to past broken behavior.

Comment 1

[Cristian Nicolae (:xti)]

related to bug 789406?

Comment 2

[James Willcox (:snorp) ([e-mail]) (he/him)]

So there are two parts here:

1) For some reason, calling setVisiblity() on LayerView no longer has the appropriate effect on the underlying SurfaceView. Calling setVisibility(View.INVISIBLE) used to destroy the SurfaceView's surface and we would pause compositing, etc. It looks like we have to explicitly set the visibility on the SurfaceView and not just the parent (weird). However, after fixing that...

2) The patch for bug 797942 pauses and resumes the compositor on every resize event. While transitioning to fullscreen, we often change the orientation to landscape, so it triggers this path. The pause and resume events are both synchronous. Flash seems to hold a mutex that isn't released until the fullscreen SurfaceView appears on the screen, so we get a deadlock.

IMHO, the patch for bug 797942 is pretty questionable. Sending pause/resume on every resize is kind of expensive and introduces the opportunity for deadlocks like this to occur. I'm trying to find a workaround, but I think there are probably better solutions for bug 797942 too.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix fullscreen Flash on Android

Comment 5

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 673370 [details] [diff] [review]
Fix fullscreen Flash on Android

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

::: mobile/android/base/GeckoApp.java
@@ +1354,5 @@
>          mFullScreenPluginContainer.removeView(mFullScreenPluginView);
>  
>          // We need do do this on the next iteration in order to avoid
>          // a deadlock, see comment below in FullScreenHolder
>          mMainHandler.post(new Runnable() { 

please fix this trailing whitespace while you're here

@@ +1362,5 @@
>          });
>  
>          FrameLayout decor = (FrameLayout)getWindow().getDecorView();
>          decor.removeView(mFullScreenPluginContainer);
>          

please fix this whitespace

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 673370 [details] [diff] [review]
Fix fullscreen Flash on Android

[Approval Request Comment]
Low risk, fixes major Flash regression

Comment 8

[Matt Brubeck (:mbrubeck)]

Backed out on inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8101371a2a4e

because all Android tests were crashing with "Verifier rejected class Lorg/mozilla/gecko/gfx/LayerView":
https://tbpl.mozilla.org/php/getParsedLog.php?id=16344714&full=1&branch=mozilla-inbound

I clobbered and retriggered; if the clobber build is green then this can re-land (with another clobber when it does).

Comment 9

[Matt Brubeck (:mbrubeck)]

Unfortunately the clobber build failed with the same error.

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix fullscreen Flash on Android

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

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

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 673933 [details] [diff] [review]
Fix fullscreen Flash on Android

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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

Comment 15

[Cristian Nicolae (:xti)]

Full screen flash works fine on the latest Nightly. Setting flag as verified for Firefox 19 branch. 

--
Firefox for Android 19.0a1 (2012-10-23)
Device: Galaxy S2
OS: Android 4.0.3

Comment 16

[Aaron Train [:aaronmt]]

Verified Fixed on trunk; awaiting channel uplift

Comment 17

[Lukas Blakk [:lsblakk] use ?needinfo]

Please uplift to branches asap.

Comment 18

[James Willcox (:snorp) ([e-mail]) (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/33d62a2e0fe9
https://hg.mozilla.org/releases/mozilla-beta/rev/cfc343c55624

Comment 19

[Cristian Nicolae (:xti)]

This issue is fixed on the latest Aurora too.

--
Firefox 18.0a2 (2012-10-29)
Device: Galaxy R
OS: Android 2.3.4

Comment 20

[Paul Feher]

This issue is fixed on Firefox mobile 17 Beta 5 too

Firefox 17.0b5 (2012-11-07)
Device: Acer A500
OS: Android 3.2.1