Closed Bug 881861 Opened 7 years ago Closed 6 years ago

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

Categories

(Firefox for Android :: General, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 29
Tracking Status
firefox22 --- unaffected
firefox23 --- unaffected
firefox24 --- wontfix
firefox25 --- wontfix
firefox26 --- wontfix
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.3 --- fixed
fennec + ---

People

(Reporter: aaronmt, Assigned: wesj)

References

(Blocks 1 open bug)

Details

(Keywords: polish, reproducible)

Attachments

(5 files, 1 obsolete file)

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.
Summary: Quickshare: Activity name exposed in share menu; missing package icon when applications are updated → Quickshare: Activity name exposed in share menu; missing package icon when applications are updated or uninstalled
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).
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.
Attached patch Patch (obsolete) — Splinter Review
I don't suspect any performance overhead.
Attachment #761181 - Flags: review?(mark.finkle)
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?
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.
Flags: needinfo?(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.
Flags: needinfo?(sriram)
Duplicate of this bug: 888255
Still broken.

Finkle going to review the patch?
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.
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 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?
Attachment #761181 - Flags: review?(mark.finkle) → review+
(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.
Duplicate of this bug: 941743
Can the 28+ be forwarded over from the bug duped?
Assignee: nobody → sriram
Status: NEW → ASSIGNED
tracking-fennec: --- → 28+
Assignee: sriram → nobody
Status: ASSIGNED → NEW
Keywords: polish
Hardware: ARM → All
Can we land this to see if the related crasher I just filed in bug 956789 is fixed?
Duplicate of this bug: 956789
Assignee: nobody → wjohnston
Well?
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 29
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.
Blocks: 963363
(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 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.
Attachment #761181 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/f4c5e883482a
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 964585
Attachment #761181 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
This should be backed out of m-c as per comment in bug 964585
Attached patch Patch v2Splinter Review
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.
Attachment #761181 - Attachment is obsolete: true
Attachment #8366944 - Flags: review?(mark.finkle)
Attached patch Patch part 2Splinter Review
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...
Attachment #8366999 - Flags: review?(mark.finkle)
Attachment #8366944 - Flags: review?(mark.finkle) → review+
(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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/integration/fx-team/rev/8e7ac592bcbb

leave open for part 2
Whiteboard: [leave-open]
Attachment #8366999 - Flags: review?(mark.finkle) → review+
Close?

This is verified fixed on trunk. This also fixed my crash accessing removed applications in the menu while the browser is still open.
Renoming. I'm more nervous about this second part and would prefer to just keep it on 30. Any strong opinions?
tracking-fennec: 28+ → ?
(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?
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.
So the first patch might be uplift-fx28 worthy?
Depends on: 969743
Depends on: 971735
tracking+, nominate for uplift it the patch winds up looking safe
tracking-fennec: ? → +
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.
Attachment #8366944 - Flags: approval-mozilla-beta?
Attachment #8366944 - Flags: approval-mozilla-aurora?
Attachment #8366944 - Flags: approval-mozilla-beta?
Attachment #8366944 - Flags: approval-mozilla-beta+
Attachment #8366944 - Flags: approval-mozilla-aurora?
Attachment #8366944 - Flags: approval-mozilla-aurora+
Might be easier for tracking purposes at this point if a follow-up bug was filed for part 2.
Flags: needinfo?(wjohnston)
Target Milestone: Firefox 29 → ---
pulled the other bug into 975525
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Flags: needinfo?(wjohnston)
Resolution: --- → FIXED
Whiteboard: [leave-open]
Target Milestone: --- → Firefox 29
You need to log in before you can comment on or make changes to this bug.