Closed Bug 832990 Opened 11 years ago Closed 6 years ago

Menu items added by add-ons disappear if "Don't keep activities" is checked

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(fennec+, firefox57 wontfix, firefox58 wontfix, firefox59 verified, firefox60 verified)

Status:
RESOLVED FIXED
Milestone:
Firefox 60
Tracking Flags:
Tracking Status
fennec + ---
firefox57 --- wontfix
firefox58 --- wontfix
firefox59 --- verified
firefox60 --- verified

People

(Reporter: kats, Assigned: JanH)

References

Details

(Whiteboard: [add-ons])

Attachments

(3 files)

Bug 832990 - Part 0 - Improve comment wording.
59 bytes, text/x-review-board-request
Grisha
: review+
Details
Bug 832990 - Part 1 - Make MenuItemInfo classes parcelable.
59 bytes, text/x-review-board-request
Grisha
: review+
RyanVM
: approval-mozilla-beta+
Details
Bug 832990 - Part 2 - Save and restore menu item caches via savedInstanceState.
59 bytes, text/x-review-board-request
Grisha
: review+
RyanVM
: approval-mozilla-beta+
Details 
STR:
- Turn on the "don't keep activities" checkbox in the developer settings
- Install an add-on that has a menu item (I used capella's QuitNow addon)
- Start Fennec, ensure that the menu item shows up
- Open the awesomebar screen and load a page (you may have to do this twice, for some reason it didn't happen the first time for me)
- Check the menu to see if the QuitNow menu item is still there

Actual:
- It is not.

It looks like we keep the menu items in the mAddonMenuItemsCache until the first time we build the menu, and then clear that list. If the activity goes away, so do the menu items. We should probably keep that list around.
I think this is still an issue that add-on authors are running into. See for example [1] which says:

"NOT WORKING? There is a known Firefox bug that affects this and other add-ons.

If on your Android device, under Settings -> Developer Options, you have "Do not keep activities" selected / checked, Firefox will lose add-on menu items ... Uncheck that option, to solve that problem :-)"

[1] https://addons.mozilla.org/en-US/android/addon/cleanquit/
tracking-fennec: --- → ?
I added code to fix a similar situation, triggered by the locale switcher.

https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#2440

The trick with this bug is that we're persisting the menu items in BrowserApp itself, which is being thrown away each time. If we fix that -- persisting the menu items list outside of BrowserApp -- then everything else should work correctly.
We should fix this to make sure the menu add-on API works reliably, but it's not high priority for us right now.
tracking-fennec: ? → +
Whiteboard: [add-ons]
This happens to me even if I don't have "don't keep activities" checkbox in the developer settings selected. I disabled developer settings completly.

I can reproduce easily by:

- open Firefox, and check if menu entries are present (I have three, for uBlock, Cookie AutoDelete and Violentmonkey)
- go to system settings -> display -> font size
- change font size (medium to small or back)
- go back to Firefox
- menu entries are missing

This is on Android 4.4.2 with Firefox Nightly 58.0a1 (2017-10-22)
(In reply to gwarser from comment #4)
> This happens to me even if I don't have "don't keep activities" checkbox in
> the developer settings selected.

Yes, that option is intended to force this behaviour every time the app is backgrounded to make testing easier, but when resources are running low it can happen for real even without that option being active.
See Also: → 1396270
tracking-fennec: + → ?
status-firefox57: --- → affected
status-firefox58: --- → affected
status-firefox59: --- → affected
Version: unspecified → Trunk
Hi Joe, Wesly
Please help prioritize this.
Flags: needinfo?(wehuang)
Flags: needinfo?(jcheng)
tracking-fennec: ? → +
Joe, should we consider to fix add-on support in the next nightly cycle if no other priority? (looks similiar to 1396270, both are affecting the use of add-on)
Flags: needinfo?(wehuang)
See Also: → 1427255
Can we get a priority on this please? We think this is affecting add-ons and being mentioned in multiple other bugs.
See Also: 1427255 → 
Had a look at it - if this is actually as simple as it looks, I should have something within the next few days.
Assignee: nobody → jh+bugzilla
As a quick fix I'd simply make those menu item objects parcelable, so we can stick them into the savedInstanceState and restore them from there, but when I can find some more time we actually have to move them out of BrowserApp in order to solve bug 1414084.
See Also: → 1414084
I want to add that in standard, not PWA mode, browserAction items disappear similarly to menu items. For example "Close button" from this extension https://addons.mozilla.org/firefox/addon/close-tab-button/
For menu items the above approach seems to work fine. For URL bar page actions I need to see if I can get something similar working without too much hassle - if not, I'll punt it to bug 1414084.
Respectively PageActions also have a Drawable that needs to be saved somehow and I'm not going down that rabbit hole right now, so definitively leaving PageActions to bug 1414084.
Comment on attachment 8945608 [details]
Bug 832990 - Part 0 - Improve comment wording.

https://reviewboard.mozilla.org/r/215742/#review222800
Attachment #8945608 - Flags: review+
Comment on attachment 8945609 [details]
Bug 832990 - Part 1 - Make MenuItemInfo classes parcelable.

https://reviewboard.mozilla.org/r/215744/#review222806
Attachment #8945609 - Flags: review?(gkruglov) → review+
Comment on attachment 8945610 [details]
Bug 832990 - Part 2 - Save and restore menu item caches via savedInstanceState.

https://reviewboard.mozilla.org/r/215746/#review222808

Looks good, but I'd like to hear an answer to Q2.

Q1: Can you add a comment in code saying that this is a stop-gag measure, and link to 1414084?

Q2: Also, it'd be good to leave brief comments explaining when these caches are actually used, and why you think they'll be populated by the time we need them. AFAICT, at two points - whenever we create the menu (so, after a user action to open, obviously cache will be in good state), and whenever messages like "Menu:AddBrowserAction" come in. The latter isn't obvious to me: from android docs, onRestoreInstanceState is called "between onStart() and onPostCreate(Bundle)." But we register listeners for these messages in "onCreate", leaving a bit of a gap. I'm not sure what the other side of this message bridge looks like - are you confident that small gap won't be problematic?
Attachment #8945610 - Flags: review?(gkruglov) → review+
Comment on attachment 8945610 [details]
Bug 832990 - Part 2 - Save and restore menu item caches via savedInstanceState.

https://reviewboard.mozilla.org/r/215746/#review222808

Q1: Yes, of course.
Q2: That's why I'm restoring from within `onCreate()`, before those event listeners are even added.
(In reply to Jan Henning [:JanH] from comment #22)
> Q2: That's why I'm restoring from within `onCreate()`, before those event
> listeners are even added.

Ah, of course. Carry on then, I got things confused and thought you were going through 'onRestoreInstanceState'.
Pushed by mozilla@buttercookie.de:
https://hg.mozilla.org/integration/autoland/rev/8dee38a1aeec
Part 0 - Improve comment wording. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/3f03169f58a9
Part 1 - Make MenuItemInfo classes parcelable. r=Grisha
https://hg.mozilla.org/integration/autoland/rev/fd684a8d5206
Part 2 - Save and restore menu item caches via savedInstanceState. r=Grisha
Flags: needinfo?(jcheng)
Is this something we should consider for Beta backport or can it ride the 60 train?
status-firefox57: affected → wontfix
status-firefox58: affected → wontfix
Flags: needinfo?(jh+bugzilla)
Please please backport, extensions are really painful to use on Android: I need to go back and fort in the add-ons panel to disable and re-enable NoScript almost every time I need to unblock a script.
Comment on attachment 8945609 [details]
Bug 832990 - Part 1 - Make MenuItemInfo classes parcelable.

See below.
Attachment #8945609 - Flags: approval-mozilla-beta?
Comment on attachment 8945610 [details]
Bug 832990 - Part 2 - Save and restore menu item caches via savedInstanceState.

Approval Request Comment
[Feature/Bug causing the regression]: Add-on menu item support on Android
[User impact if declined]: (Main) menu items added by add-ons disappear if Android  destroys the activity while Firefox is in background
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: Not yet, but tested locally.
[Needs manual test from QE? If yes, steps to reproduce]: See comment 0, but instead of "Open the awesomebar screen" go into our settings or switch to some other app and then go back to Firefox.
[List of other uplifts needed for the feature/fix]: Part 1.
[Is the change risky?]: Not much.
[Why is the change risky/not risky?]: We're just passing the menu item data to Android for safekeeping when the activity is destroyed, so we can restore it later on if necessary. The data for a single menu item should take only around 100 bytes, so even for people with lots of add-ons we're very unlikely to exceed the maximum amount of data that can be stored that way (~500 kB).
[String changes made/needed]: none
Flags: needinfo?(jh+bugzilla)
Attachment #8945610 - Flags: approval-mozilla-beta?
Verified as fixed on Nightly 60.0a1, the add on is still displayed in Menu following the steps from comment 0 and comment 32.
Device: LG G4 (Android 5.1), Motorola Nexus 6 (Android 7.0).
status-firefox60: fixed → verified
Comment on attachment 8945609 [details]
Bug 832990 - Part 1 - Make MenuItemInfo classes parcelable.

Fixes a Fennec addon annoyance and verified on Nightly. Approved for 59b9.
Attachment #8945609 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8945610 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on the latest beta build, 59.0b9. This issue was verified on a Huawei P8 Lite - Android 8.0 and on a Pixel XL Android 8.0.0
status-firefox59: fixed → verified
See Also: 1396270 → 
This is still happening in Firefox 60 Nightly. When a page loads, uBlock Origin and Decentraleyes entries sometimes aren't visible in the menu. I have to disable and enable extensions from Addons area for their entries to appear in menu.
Are you using custom tabs or PWAs?
(In reply to smartfon.reddit from comment #38)
> This is still happening in Firefox 60 Nightly. When a page loads, uBlock
> Origin and Decentraleyes entries sometimes aren't visible in the menu. I
> have to disable and enable extensions from Addons area for their entries to
> appear in menu.

What device are you using? thanks
Flags: needinfo?(smartfon.reddit)
It's still happening for me too. 

LG G6, Android 7.0

I don't understand Jan's question about custom tabs, aren't they enabled by default? But I do have a PWA.
(In reply to Paul [pwd] from comment #41)
> I don't understand Jan's question about custom tabs, aren't they enabled by
> default? But I do have a PWA.

Even if custom tabs are enabled in Firefox, you might not use any app that actually wants to open a custom tab in Firefox.

The reason I'm asking is because a variety of this bug can still happen if the Firefox process is started through a custom tab or a PWA, i.e. bug 1414084.
LG L65 with Lineage OS Android 7.1.2. Firefox 59.0b9
Happened when I opened link from other application (InoReader) using "Tab queue".
(In reply to Jan Henning [:JanH] from comment #42)
> (In reply to Paul [pwd] from comment #41)
> > I don't understand Jan's question about custom tabs, aren't they enabled by
> > default? But I do have a PWA.
> 
> Even if custom tabs are enabled in Firefox, you might not use any app that
> actually wants to open a custom tab in Firefox.
> 
> The reason I'm asking is because a variety of this bug can still happen if
> the Firefox process is started through a custom tab or a PWA, i.e. bug
> 1414084.

Well I'm guessing that's it then. I have Twitter and Flamingo amongst others using Firefox's custom tabs, as well as Hive Home set up as a PWA.
Unfortunately I don't have an LG G6 to test. We do have a LG G4 with Android 5.1 and could't reproduce. I've  installed uBlock, Stylish, Cookie AutoDelete and the menu items was still displayed after following the steps from comments. Tried also opening from tab queue and Custom tab.
Flags: needinfo?(smartfon.reddit)
I'm using LG G5. This happened with stock ROM as well as LineageOS.
I open links on Firefox by tapping a link in Flamingo for Twitter app. Flamingo's settings have "Chrome custom tabs" disabled. Firefox opens a regular tab with all available settings, and not a stripped down tab like in Chrome Custom Tabs.
Firefox is configured as the default phone Assistant from Android Settings. I don't use Progressive Web Apps. Custom Tabs entry is enabled in Firefox settings.
The problem happens with Custom Tabs option disabled in Firefox settings.
Hmm,... please wait for bug 1414084 to be fixed, as that will rework that area of the code once more anyway.
If you can still reproduce any strange behaviour even after that bug lands, please open a new bug.
Opened #1505617 as I still have this behavior on Android 8.1 Nexus 6P with Firefox 62.0.3.
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: