1387026 - Fix Android browserAction corrupting legacy Addon menu items label on overlapping menu item id

Status: VERIFIED FIXED

(WebExtensions :: Android, defect, P1)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

Unfortunately, besides the issue fixed by Bug 1375857, there is also another scenario that leads to the corruption of unrelated menu items added using the `NativeApp.menu` helpers (http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mobile/android/chrome/content/browser.js#2267-2308), e.g. used in the integrated Android WebcompatReporter: http://searchfox.org/mozilla-central/source/mobile/android/chrome/content/WebcompatReporter.js#62-71

The issue is due to the way Android keeps track of the added menu items using a unique integer id, which is assigned from the sender of the "Menu:Add" and "Menu:AddBrowserAction" (basically the privileged JS code) and the receiver (the Java side of Firefox for Android, in particular the BrowserApp class) adds an ADDON_MENU_OFFSET offset to this id:

- http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#257

which is unfortunately shared between "Menu:Add" and "Menu:AddBrowserAction":

- http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1841-1848
- http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1830-1831
- http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1852-1853
- http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#1855-1862

and the senders are using two different counters:

- http://searchfox.org/mozilla-central/rev/bbc1c59e460a27b20929b56489e2e55438de81fa/mobile/android/chrome/content/browser.js#2290-2291
- http://searchfox.org/mozilla-central/source/mobile/android/modules/BrowserActions.jsm#19,70

This obviously leads to the issue: 
because of the overlapping menu item ids, a browserAction updates the label of a menu item installed by the NativeWindow.menu.add helper (which includes at least the one installed by the integrated WebcompatReporter).

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1387026 - Fix Android browserAction corrupting legacy Addon menu items on overlapping menu item id.

Review commit: https://reviewboard.mozilla.org/r/164470/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/164470/

Comment 2

[Luca Greco [:rpl] [:luca] [:lgreco]]

Hi Sebastian,
In the attached patch there is an initial workaround (and I'm hating myself a little bit because of it :-), because I would really prefer a more complete solution, e.g. unify the NativeWindow.menu and BrowserActions.jsm, or change the menu item messages to just don't require a numeric id allocated on the sender side).

As briefly described in Comment 0 the current behavior in Firefox for Android 56 is broken and so I opted to attach the simplest mitigation possible as the first step, something as low risky as possible (e.g. for an uplift to beta), so that we can discuss about the preferred long term solution as a follow up.

How this sounds to you?

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8893371 [details]
Bug 1387026 - Fix Android browserAction corrupting legacy Addon menu items on overlapping menu item id.

https://reviewboard.mozilla.org/r/164470/#review169752

:)

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8893371 [details]
Bug 1387026 - Fix Android browserAction corrupting legacy Addon menu items on overlapping menu item id.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/164470/diff/1-2/

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2d15b3c8b58c
Fix Android browserAction corrupting legacy Addon menu items on overlapping menu item id. r=sebastian

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/2d15b3c8b58c

Comment 7

[Vasilica Tamas, QA [:vtamas]]

Attachment: Animation.gif

This issue is verified as fixed on Fennec 57.0a1 (2017-08-08) under Android 7.1.2. The corrupted menu item is no longer displayed. See attached screencast.

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8893371 [details]
Bug 1387026 - Fix Android browserAction corrupting legacy Addon menu items on overlapping menu item id.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1331742 - Add support for browserAction.onClicked
and
Bug 1351111 - Add support for browserAction.setTitle and browserAction.getTitle

which introduced the new browserActions.jsm helper and a related set of changes in the BrowserApp Java class.

[User impact if declined]:
If a user installs more than one WebExtension add-on that defines a browserAction, the menu ids assigned by the browserAction internals and the ones assigned by the internal menu items API (e.g. like the "Report Site" menu item integrated in Fennec) are going to overlap, and then an update of the browserAction title is going to update the wrong menu item because they are going to use the same id. 
(which will be very confusing for the user, e.g. clicking the menu item will actually have a unexpected behavior because the corrupted menu items will be executed instead of the action expected by the related browserAction).

[Is this code covered by automated tests?]:
No

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, the same STR used to verify the fix on Nightly should be used to verify it
on beta.

[List of other uplifts needed for the feature/fix]:
Bug 1375857 - Fix browserAction Android menu items corrupting addon menu items cache

fixes a similar issue which happens when a WebExtension add-on with a browserAction has been installed and then uninstalled.

Both Bug 1375857 and Bug 1387026 are needed so that the browserAction menu items would stop from corrupting the unrelated menu items added using the internal menu items API.

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
Bug 1387026 applies a very small change (2 lines change which change the offset used for assigning the browserAction menu item ids), these changes only impact on the browserAction Java internals and none of the other feature should be impacted by them.

[String changes made/needed]:
None

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8893371 [details]
Bug 1387026 - Fix Android browserAction corrupting legacy Addon menu items on overlapping menu item id.

Let's uplift this and the work in bug 1375857 to get the menus working correctly and looking good for beta 3.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0f875d25932a

Comment 11

[CosminB]

Attachment: FixMenu.gif

This issue is verified as fixed on Fennec 56.0b3 under Android 6.0.1. 

The corrupted menu item is no longer displayed.