881861 - Quickshare: Activity name exposed in share menu; missing package icon when applications are updated or uninstalled

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (06/11) - Screenshot

Device: LG Nexus 4 (Android 4.2.2)
OS: Android 4.2.2
Build: Nightly (06/11)

Currently the share menu item list doesn't toss it's cache and 'rebuild' when applications get updated while Firefox is running.

What the user will see is a standard Android icon and the activity name. This didn't happen with the old share popup screen (Chrome uses this and is unaffected as a result).

See screenshot. In the case of my screenshot, Flipboard had been updated while Nightly was running.

Comment 1

[Aaron Train [:aaronmt]]

Attachment: Nightly (06/11) - Screenshot (#2)

This is also reproducible when applications are uninstalled while Firefox is running.

For example, install Google Drive. Run Firefox, see share menu. Uninstall Google Drive, return to Firefox and see the share menu (activity names remain).

Comment 2

[Sriram Ramasubramanian [:sriram]]

This is because we removed the ability to monitor add/remove package changes as it had more dependencies. To add this functionality, we need to monitor add/remove packages ourselves.

But here is the problem.
A dynamic BroadcastReceiver isn't listening for these changes. They have to be declared in the manifest. When declared in the manifest, there is no way to get the actual ActivityContextModel and ask it to reload activities.

Examples in stackoverflow dealing with dynamic receivers unregistered in onPause() won't work for us. When we are paused we want to be notified.

Comment 3

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

I don't suspect any performance overhead.

Comment 4

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

Comment on attachment 761181 [details] [diff] [review]
Patch

The Internet and StackOverflow seem to think getInstalledApplications should be very fast. The consensus seems to be that requesting metadata for an ApplicationInfo is more time consuming.

That said, I'd like to know how long the getInstalledApplications line takes to complete on some of our test hardware. Can you add some timing code to the line and do some tests?

Comment 5

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

Also, are we sure this code handles package replaces and uninstalls? It seems kind of fragile. If someone uninstalled AppA and installed AppB, we would be tricked into thinking nothing changed.

Comment 6

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #4)
> Comment on attachment 761181 [details] [diff] [review]
> Patch
> 
> The Internet and StackOverflow seem to think getInstalledApplications should
> be very fast. The consensus seems to be that requesting metadata for an
> ApplicationInfo is more time consuming.
> 
> That said, I'd like to know how long the getInstalledApplications line takes
> to complete on some of our test hardware. Can you add some timing code to
> the line and do some tests?

I tried it. It's about 19-25ms on the average. And at times spiking to 50-70ms (once in 20+ calls). Which seems like not a problem.

Also, the probability of someone running Firefox in the background, installing and uninstalling equal number of apps that can handle links is very very less. I feel like, if we have to find a solution for them, we are over optimizing stuff.

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Nightly (11/11) - Screenshot

Still broken.

Finkle going to review the patch?

Comment 9

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

Comment on attachment 761181 [details] [diff] [review]
Patch

>+        List<ApplicationInfo> applications = mContext.getPackageManager().getInstalledApplications(0);
>+        if (applications != null && applications.size() != mApplicationsCount) {
>+            mApplicationsCount = applications.size();
>+            mReloadActivities = true;
>+        }

Using differences in the size means QA knows how to break this fix: Remove one app and add another.

Doing anything more than this means we get slower, because getting the app metadata is the slow part.

Comment 10

[Sriram Ramasubramanian [:sriram]]

QA breaking is fine. What's the probability of this happening in the field?

Re-quoting my earlier comment:
Also, the probability of someone running Firefox in the background, installing and uninstalling equal number of apps that can handle links is very very less. I feel like, if we have to find a solution for them, we are over optimizing stuff.

Comment 11

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

Comment on attachment 761181 [details] [diff] [review]
Patch

I'll r+ this as a bandaid, but I think we should look for other ways to be more robust.

Can't we tell when the app is no longer installed (it shows it's package name) and just not show that menu item?

Comment 12

[Aaron Train [:aaronmt]]

(In reply to Sriram Ramasubramanian [:sriram] from comment #10)
> QA breaking is fine. What's the probability of this happening in the field?

I powered on a device I had not powered on in a week. Dozens of updates going on. I opened Nightly, I opened the share menu.

Seems easy enough to hit but that's just me.

Comment 14

[Aaron Train [:aaronmt]]

Can the 28+ be forwarded over from the bug duped?

Comment 15

[Aaron Train [:aaronmt]]

Can we land this to see if the related crasher I just filed in bug 956789 is fixed?

Comment 17

[Aaron Train [:aaronmt]]

Well?

Comment 18

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/f4c5e883482a

Comment 19

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

Note: This is the "less than ideal" way of fixing this bug. I still want to look into backing this out and using Package install watchers.

Comment 20

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #19)
> Note: This is the "less than ideal" way of fixing this bug. I still want to
> look into backing this out and using Package install watchers.

Filed Bug 963363. Please set tracking flags accordingly!

Comment 21

[Wesley Johnston (:wesj)]

Comment on attachment 761181 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Quickshare
User impact if declined: Strange/wrong app icons in quickshare menus
Testing completed (on m-c, etc.): landed on mc today
Risk to taking this patch (and alternatives if risky): We have long term plans to back this out and do something nicer. The alternatives involve listening for installs and updating the list when they happen. Lack of time is the only reason it hasn't been written yet.
String or IDL/UUID changes made by this patch: None.

Comment 22

[Carsten Book [:Tomcat]]

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

Comment 23

[Aaron Train [:aaronmt]]

This should be backed out of m-c as per comment in bug 964585

Comment 24

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This dynamically registers a listener for installs/uninstalls that basically forces a clobber of the list. It also updates the history list when things are uninstalled.

Still some more work to do here to make the actual quickshare buttons aware of these changes.

Comment 25

[Wesley Johnston (:wesj)]

Attachment: Patch part 2

I'm still trying to find the best/right way to do this, but here's one approach. We need our controller (GeckoMenuItem) to be notified when the dataset (an ActivityChooserModel wrapped in a GeckoActionProvider) xhanges and then notify the view (MenuItemActionView) that it needs to update.

This would work for normal menu items, which are driven by an Adapter filling a ListView, but for these things in the secondaryActionbar, we don't invalidate when the menu is shown.

This gives us a chance to invalidate by notifying the GeckoMenu when the menu is about to be shown and letting it query the provider.

Alternatively, we could let the menu register a listener on the GeckoActionProvider (which would in turn register it on the ActivityChooserModel), and update the menu when its called...

Comment 26

[Wesley Johnston (:wesj)]

backed out the original patch:
https://hg.mozilla.org/integration/fx-team/rev/12ac1dced3d0

Comment 27

[Ed Morley [:emorley]]

(In reply to Wesley Johnston (:wesj) from comment #26)
> backed out the original patch:
> https://hg.mozilla.org/integration/fx-team/rev/12ac1dced3d0

Merge of backout:
https://hg.mozilla.org/mozilla-central/rev/12ac1dced3d0

Comment 28

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/integration/fx-team/rev/8e7ac592bcbb

leave open for part 2

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8e7ac592bcbb

Comment 30

[Aaron Train [:aaronmt]]

Close?

This is verified fixed on trunk. This also fixed my crash accessing removed applications in the menu while the browser is still open.

Comment 31

[Wesley Johnston (:wesj)]

Renoming. I'm more nervous about this second part and would prefer to just keep it on 30. Any strong opinions?

Comment 32

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

(In reply to Wesley Johnston (:wesj) from comment #31)
> Renoming. I'm more nervous about this second part and would prefer to just
> keep it on 30. Any strong opinions?

Is the first part enough to fix the crash and *some* of the package name issues?

Comment 33

[Wesley Johnston (:wesj)]

Yes. The remaining patch just fixes things if you uninstall an app that's already in the quickshare list. Even then its not a crash. Just odd looking, and should fix itself next start.

Comment 34

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

So the first patch might be uplift-fx28 worthy?

Comment 35

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

tracking+, nominate for uplift it the patch winds up looking safe

Comment 36

[Wesley Johnston (:wesj)]

Comment on attachment 8366944 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Quickshare bug from forever
User impact if declined: Uninstalling apps while Fennec is running can get you in a funny state.
Testing completed (on m-c, etc.): Been on mc for awhile. One bug found and fixed in bug 971735, which I'll also nom for uplift.
Risk to taking this patch (and alternatives if risky): Relatively low risk. No good alternatives I can think of.
String or IDL/UUID changes made by this patch: None.

Comment 37

[Wesley Johnston (:wesj)]

Aurora already seems to have this:
https://hg.mozilla.org/releases/mozilla-beta/rev/846372de1a93
https://hg.mozilla.org/releases/mozilla-beta/rev/19a78a94f24c

Comment 38

[Ryan VanderMeulen [:RyanVM]]

Might be easier for tracking purposes at this point if a follow-up bug was filed for part 2.

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3/rev/846372de1a93

Comment 40

[Wesley Johnston (:wesj)]

pulled the other bug into 975525