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

VERIFIED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: Android
P1
normal
VERIFIED FIXED
3 months ago
2 months ago

People

(Reporter: rpl, Assigned: rpl)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---

Firefox Tracking Flags

(firefox55 unaffected, firefox56 verified, firefox57 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

(Assignee)

Description

3 months ago
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).
(Assignee)

Updated

3 months ago
Assignee: nobody → lgreco
Blocks: 1330159
Status: NEW → ASSIGNED
Priority: -- → P1
See Also: → bug 1351111
Comment hidden (mozreview-request)
(Assignee)

Comment 2

3 months ago
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?
Flags: needinfo?(s.kaspari)

Comment 3

3 months ago
mozreview-review
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

:)
Attachment #8893371 - Flags: review+
Comment hidden (mozreview-request)

Comment 5

2 months ago
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/2d15b3c8b58c
Fix Android browserAction corrupting legacy Addon menu items on overlapping menu item id. r=sebastian
(Assignee)

Updated

2 months ago
Flags: needinfo?(s.kaspari)
https://hg.mozilla.org/mozilla-central/rev/2d15b3c8b58c
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Created attachment 8895395 [details]
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.
Status: RESOLVED → VERIFIED
status-firefox57: fixed → verified
(Assignee)

Comment 8

2 months ago
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
Attachment #8893371 - Flags: approval-mozilla-beta?
status-firefox55: --- → unaffected
status-firefox56: --- → affected
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.
Attachment #8893371 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 10

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/0f875d25932a
status-firefox56: affected → fixed

Comment 11

2 months ago
Created attachment 8898702 [details]
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.

Updated

2 months ago
status-firefox56: fixed → verified
You need to log in before you can comment on or make changes to this bug.