Closed
Bug 1387026
Opened 7 years ago
Closed 7 years ago
Fix Android browserAction corrupting legacy Addon menu items label on overlapping menu item id
Categories
(WebExtensions :: Android, defect, P1)
WebExtensions
Android
Tracking
(firefox55 unaffected, firefox56 verified, firefox57 verified)
VERIFIED
FIXED
mozilla57
Tracking | Status | |
---|---|---|
firefox55 | --- | unaffected |
firefox56 | --- | verified |
firefox57 | --- | verified |
People
(Reporter: rpl, Assigned: rpl)
References
Details
Attachments
(3 files)
59 bytes,
text/x-review-board-request
|
sebastian
:
review+
lizzard
:
approval-mozilla-beta+
|
Details |
9.62 MB,
image/gif
|
Details | |
603.94 KB,
image/gif
|
Details |
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•7 years ago
|
Comment hidden (mozreview-request) |
Assignee | ||
Comment 2•7 years 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•7 years 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) |
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•7 years ago
|
Flags: needinfo?(s.kaspari)
Comment 6•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2d15b3c8b58c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Comment 7•7 years ago
|
||
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.
Updated•7 years ago
|
Status: RESOLVED → VERIFIED
Assignee | ||
Comment 8•7 years 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?
Updated•7 years ago
|
status-firefox55:
--- → unaffected
status-firefox56:
--- → affected
Comment 9•7 years ago
|
||
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•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/0f875d25932a
Comment 11•7 years ago
|
||
This issue is verified as fixed on Fennec 56.0b3 under Android 6.0.1. The corrupted menu item is no longer displayed.
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•