746703 - ICS: Menu Button is displayed twice

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Patryk Adamczyk [:patryk] UX]

Attachment: 2 menu buttons

Using a HTC EVO X (ICS) it appears that device does not recognized that we have a menu button within the app, so it draws another one. See screenshot. 
*The HTC has physical buttons, unlike the Nexus Galaxy.

Comment 1

[Kevin Brosnan [Ex-Mozilla]]

We should discuss if this is bad/common enough to block.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Would be nice to know what the device market size is too.

Comment 3

[Patryk Adamczyk [:patryk] UX]

So this is will happen all the HTC ONE devices, they will likely be fairly popular as the last HTC device was Sprint's #1 selling phone so I hear.

More background:
It appears all apps that have 2.3 support show this issue (facebook, google docs, flickr). The newer 4.0 apps don't seem to exhibit this issue such as Chrome Beta, ICS native apps or Instagram.

Comment 4

[Matt Brubeck (:mbrubeck)]

I suspect this can be fixed by changing targetSdkVersion to 14 in our AndroidManifest.xml.

Comment 5

[Matt Brubeck (:mbrubeck)]

Attachment: patch

Could someone with an affected device please verify that the bug is fixed in this test build?
https://people.mozilla.com/~mbrubeck/fennec-746703.apk

Comment 6

[Matt Brubeck (:mbrubeck)]

http://android-developers.blogspot.com/2012/01/say-goodbye-to-menu-button.html explains, "if you set minSdkVersion to 10 or lower, set targetSdkVersion to 11, 12, or 13, and you do not use ActionBar, the system will add the legacy overflow button when running your app on a handset with Android 4.0 or higher.

"That exception might be a bit confusing, but it’s based on the belief that if you designed your app to support pre-Honeycomb handsets and Honeycomb tablets, it probably expects handset devices to include a Menu button (but it supports tablets that don’t have one).

"So, to ensure that the overflow action button never appears beside the system navigation, you should set the targetSdkVersion to 14. (You can leave minSdkVersion at something much lower to continue supporting older devices.)"

Comment 7

[Matt Brubeck (:mbrubeck)]

https://developer.android.com/sdk/android-4.0.html notes some other effects of setting targetSdkVersion to 14 or higher:

* Hardware acceleration is enabled by default.
* JNI uses indirect references to avoid GC-related bugs.
* Apps that don't specify a theme will use the device's default theme.

Comment 8

[Patryk Adamczyk [:patryk] UX]

Just loaded your apk. FIXED!!!!!

Comment 9

[Aaron Train [:aaronmt]]

Any possible side effects from targeting SDK 14?

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Aaron Train [:aaronmt] from comment #9)
> Any possible side effects from targeting SDK 14?

See comment 7 above for known side effects.  I don't think any of these *should* cause any user-visible changes, but we'd definitely need to watch for regressions from this patch, and be prepared to back it out if there are bad regressions that couldn't be fixed in time for the first release.

Comment 11

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7ecd07342540

Comment 12

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/7ecd07342540

Comment 13

[Joe Drew (not getting mail)]

Backed out for causing bug 747681: https://hg.mozilla.org/integration/mozilla-inbound/rev/9daee30907ea

Comment 14

[Joe Drew (not getting mail)]

Aaron points out in bug 747681 that this change hardware-accelerated our window by default. We probably don't handle that too well.

More info: http://android-developers.blogspot.ca/2011/11/android-40-graphics-and-animations.html

I am rather against this landing as-is again. Too many moving parts! It should be the first thing to land in a regular train model, IMO.

Comment 15

[Ed Morley [:emorley]]

(In reply to Joe Drew (:JOEDREW!) from comment #13)
> Backed out for causing bug 747681:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/9daee30907ea

Backout merged to m-c in time for 14:
https://hg.mozilla.org/mozilla-central/rev/9daee30907ea

Comment 16

[Matt Brubeck (:mbrubeck)]

I think we should consider *not* blocking on this because the only available fix is high-risk.  (See comment 10 and comment 14 for details.)  If we can fix this in Firefox 15 instead, it could still be released before buttony ICS phones are too widespread.

Comment 17

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

let's try bumping the SDK level to 14 and turning off accelerated surfaces. Either way, we need this fixed sooner rather than later so it can be well tested.

Comment 18

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

There's a test build at https://people.mozilla.com/~mbrubeck/fennec-746703-b.apk

Can someone with an affected phone verify that this fixes the menu button, while not causing crashes on rotate (bug 747681)?

Comment 19

[Matt Brubeck (:mbrubeck)]

Attachment: patch v2

Forgot hg qref.

Comment 20

[Aaron Train [:aaronmt]]

The build above in comment #18 specifying hardwareAccelerated with false is not valid overrode with the forced declaration of targetSDK 14 as it reintroduces bug 747681.

Comment 21

[Matt Brubeck (:mbrubeck)]

Comment on attachment 617989 [details] [diff] [review]
patch v2

(In reply to Aaron Train [:aaronmt] from comment #20)
> The build above in comment #18 specifying hardwareAccelerated with false is
> not valid overrode with the forced declaration of targetSDK 14 as it
> reintroduces bug 747681.

The targetSdk attribute is not supposed to override an explicit hardwareAccelerated="false" attribute.  According to the docs, "If necessary, you can manually disable hardware acceleration with the hardwareAccelerated attribute for individual <activity> elements or the <application> element." - https://developer.android.com/sdk/android-4.0.html

I think my patch was wrong.  Preparing a new build and patch.

Comment 22

[Matt Brubeck (:mbrubeck)]

Attachment: patch v3

The previous patch was missing an "android:" namespace prefix.  Oops!

New test build at https://people.mozilla.com/~mbrubeck/fennec-746703-c.apk

Comment 23

[Matt Brubeck (:mbrubeck)]

Comment on attachment 618711 [details] [diff] [review]
patch v3

Bug 747681 is apparently not caused by hardware acceleration.  If I just add hardwareAccelerated="true" to the manifest, there are no problems.  So it is caused by some other targetSdkVersion="14" change -- maybe the JNI changes.

Comment 24

[Matt Brubeck (:mbrubeck)]

Attachment: patch v4

The activity was restarting because of the new "screenSize" configuration change introduced in SDK version 13 and later.  This fixes the regression.

Comment 25

[Kevin Brosnan [Ex-Mozilla]]

Built with this patch. Rotation seems to be fixed. Though I'm crashing when using CTP Flash. Build at http://people.mozilla.com/~kbrosnan/tmp/fennec-746703-d.apk

Comment 26

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

(In reply to Kevin Brosnan [:kbrosnan] from comment #25)
> Built with this patch. Rotation seems to be fixed. Though I'm crashing when
> using CTP Flash. Build at
> http://people.mozilla.com/~kbrosnan/tmp/fennec-746703-d.apk

The suspicion was that the flash issues were caused by hardware accel. Can we try the current patch plus disabling hardware accel?

Comment 27

[Matt Brubeck (:mbrubeck)]

Attachment: part 2: disable hardware acceleration?

Here's an additional patch that disables HW acceleration, and a build with both patches:
https://people.mozilla.com/~mbrubeck/fennec-746703-e.apk

Comment 28

[Tony Chung [:tchung]]

qawanted: assigning to kevin to take a look.

Comment 29

[Kevin Brosnan [Ex-Mozilla]]

fennec-746703-e.apk is still crashing on the Galaxy Nexus when playing Flash. My Android 2.3 phone does not crash when playing Flash.

Comment 30

[Kevin Brosnan [Ex-Mozilla]]

I/GeckoApp(15116): Got message: DOMContentLoaded
I/GeckoTab(15116): Updated title: Unbelievably Delayed Reaction - YouTube for tab with id: 1
D/GeckoFavicons(15116): Creating LoadFaviconTask with URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo and favicon URL = http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico
D/GeckoFavicons(15116): Calling loadFavicon() with URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo and favicon URL = http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico (4)
I/GeckoApp(15116): URI - http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo, title - Unbelievably Delayed Reaction - YouTube
D/GeckoFavicons(15116): Favicon URL is now: http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico
D/GeckoFavicons(15116): Calling getFaviconUrlForPageUrl() for http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): Downloading favicon for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo with favicon URL = http://s.ytimg.com/yt/favicon-refresh-vfldLzJxy.ico
I/Gecko   (15116): Compositor: Composite took 50 ms.
D/GeckoFavicons(15116): Downloaded favicon successfully for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): Saving favicon on browser database for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
I/GeckoAppShell(15116): createSurface
E/dalvikvm(15116): JNI ERROR (app bug): accessed stale local reference 0x2590007d (index 31 in a table of size 30)
E/dalvikvm(15116): VM aborting
F/libc    (15116): Fatal signal 11 (SIGSEGV) at 0xdeadd00d (code=1)
D/GeckoFavicons(15116): Saving favicon URL for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): Calling setFaviconUrlForPageUrl() for http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
D/GeckoFavicons(15116): LoadFaviconTask finished for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo (4)
I/GeckoApp(15116): Favicon successfully loaded for URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
I/GeckoApp(15116): Favicon is for current URL = http://www.youtube.com/watch?v=QnEaeYpzDeI&feature=g-logo
I/GeckoTab(15116): Updated favicon for tab with id: 1
I/ActivityManager(  192): Process org.mozilla.fennec_mbrubeck (pid 15116) has died.
W/ActivityManager(  192): Force removing ActivityRecord{41b668d8 org.mozilla.fennec_mbrubeck/.App}: app died, no saved state
I/WindowManager(  192): WIN DEATH: Window{41aa0e90 SurfaceView paused=false}
I/WindowManager(  192): WIN DEATH: Window{41909ca0 org.mozilla.fennec_mbrubeck/org.mozilla.fennec_mbrubeck.App paused=false}
D/dalvikvm(  373): GC_CONCURRENT freed 2897K, 29% free 17013K/23687K, paused 2ms+3ms
I/DEBUG   (12519): debuggerd committing suicide to free the zombie!
D/Zygote  (  118): Process 15116 exited cleanly (11)
W/InputManagerService(  192): Got RemoteException sending setActive(false) notification to pid 15116 uid 10033
I/DEBUG   (15190): debuggerd: Apr 13 2012 21:29:36
D/dalvikvm(  373): GC_FOR_ALLOC freed 311K, 27% free 17426K/23687K, paused 30ms
D/dalvikvm(  373): GC_FOR_ALLOC freed 1359K, 28% free 17245K/23687K, paused 24ms
I/dalvikvm-heap(  373): Grow heap (frag case) to 18.469MB for 1644560-byte allocation

Comment 31

[Matt Brubeck (:mbrubeck)]

It looks like the flash crash is due to the JNI changes mentioned in comment 18. This may be a latent bug in our code that we can fix. (I hope it's not a bug in the Flash plugin's code...)

Comment 32

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

that jni bug is fuxed in bug 749750

Comment 33

[Kevin Brosnan [Ex-Mozilla]]

Do we want to do one more build to test this or are we all set?

Comment 34

[Matt Brubeck (:mbrubeck)]

Now that I'm back home I was able to test this locally and verified that bug 749750 fixes the Flash crash.  Pushed to the targetSdkVersion change to inbound:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3ddb0ce899b

I'll wait until this has made it into Nightly before requesting Aurora approval.  There's also a new test build here if anyone wants to start testing early:
https://people.mozilla.com/~mbrubeck/fennec-746703-f.apk

Comment 35

[Matt Brubeck (:mbrubeck)]

Comment on attachment 619009 [details] [diff] [review]
part 2: disable hardware acceleration?

Not checking in part 2 (disable hardware acceleration) because it did not end up having any effect on the regressions we found.

Comment 36

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/9c0d42c3ae49

Comment 37

[Matt Brubeck (:mbrubeck)]

Comment on attachment 618786 [details] [diff] [review]
patch v4

[Approval Request Comment]

User impact if declined: Incorrect main UI layout on many ICS devices.

Testing completed (on m-c, etc.): Landed on m-c 5/1

Risk to taking this patch (and alternatives if risky):  Android-only blocker fix.  This patch is somewhat risky since it has various effects on how Android configures our app.  (See discussion above for details.)  Two regressions were already found and fixed.  However, the only alternative as far as I know is to leave this bug unfixed.

String changes made by this patch: None.

Comment 38

[(no longer active)]

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

Comment 39

[Alex Keybl [:akeybl]]

Comment on attachment 618786 [details] [diff] [review]
patch v4

[Triage Comment]
While a bit risky, this is mobile only and blocks Fennec 1.0. Approved for Aurora 14.

Comment 40

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/88982d73f135

Comment 41

[Cristian Nicolae (:xti)]

There is a single Menu Button on the latest Nightly and Aurora builds.
Closing bug as verified fixed on:

Firefox 15.0a1 (2012-05-29)
Firefox 14.0a2 (2012-05-29)

Device: Galaxy Nexus
OS: Android 4.0.2