967254 - Initialize Webapp uninstall listener on DelayedStartup

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wesley Johnston (:wesj)]

Now that we have delayed startup notifications in java, we should move our FHR startup code (and WebApp Uninstall code) there.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/GeckoApp.java#1331

Comment 1

[Wesley Johnston (:wesj)]

Attachment: Patch v1

I'm not sure if we need this locale code to run earlier or not.

Comment 2

[Wesley Johnston (:wesj)]

Attachment: Patch v1

Whoops. Last minute changes = patch that didn't build.

Comment 3

[Richard Newman [:rnewman]]

Comment on attachment 8369693 [details] [diff] [review]
Patch v1

Review of attachment 8369693 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/GeckoApp.java
@@ -1283,5 @@
> -
> -                // Wait until now to set this, because we'd rather throw an exception than 
> -                // have a caller of LocaleManager regress startup.
> -                LocaleManager.setContextGetter(GeckoApp.this);
> -                LocaleManager.initialize();

Yup, we want this to occur as early as possible. (Even earlier than this would be better, but I erred on the side of perf.)

@@ +2801,5 @@
> +            editor.commit();
> +
> +            // The lifecycle of mHealthRecorder is "shortly after onCreate"
> +            // through "onDestroy" -- essentially the same as the lifecycle
> +            // of the activity itself.

This comment is no longer correct. But I'm not convinced that it's necessary to move the creation of mHealthRecorder; the only work it actually does up-front is reading the PIC on the background thread and triggering the ContentProvider to go live (if it isn't already).

Everything else is already delayed by two steps: firstly, a double chain of background runnables to do storage work; secondly, if we need to do any expensive work (like grabbing all of your add-ons and stuffing them into the DB), we _already_ block on Gecko as we wait for HealthReport:RequestSnapshot.

There's also code elsewhere in onResume that relies on mHealthRecorder having already been initialized, so as to accurately track sessions. There's a delicate lifecycle dance going on here.

I don't know how much you're going to get out of this change other than a bunch of FHR lifecycle bugs to fix :D


The way forward is to either put all this stuff back where it was, and only do the synthAPK stuff later, or to very carefully rejig the FHR initialization stages to occur in the same relative order, capturing the same session information but delaying its recording until mBHR comes on-line. The former is much easier, and recommended unless you can measure a huge perf improvement from this part of this change.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch v2

This just moves the UninstallListener stuff out. I also made some patches to move the FHR and Locale runnables into their respective classes at least, but I have a feeling there are strange lifecycle/order things there. I like the idea of us having more small runnable taks though. I'll throw them up anyway.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Patch v2 - Locale stuff

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Patch v3 - FHR stuff

Comment 7

[Richard Newman [:rnewman]]

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

Review of attachment 8369853 [details] [diff] [review]:
-----------------------------------------------------------------

::: mobile/android/base/webapp/UninstallListener.java
@@ +107,5 @@
> +        public void run() {
> +            ThreadUtils.assertOnBackgroundThread();
> +
> +            // Perform webapp uninstalls as appropiate.
> +            if (AppConstants.MOZ_ANDROID_SYNTHAPKS) {

I assume that this is always going to be true, and thus this will end up doing a bunch of work regardless?

Comment 8

[Richard Newman [:rnewman]]

Comment on attachment 8369854 [details] [diff] [review]
Patch v2 - Locale stuff

Review of attachment 8369854 [details] [diff] [review]:
-----------------------------------------------------------------

I suspect that the overhead of creating a new task and running it as another step, as well as the additional cost of another class, is enough to make us not want to take this. 

Additionally, it trades a fairly straightforward three lines (albeit a mismash of static method calls) for a more elegant but much less obvious chain of control flow -- the coupling is still present, and indeed you've coupled the independent LocaleManager class to GeckoApp and ThreadUtils, which I'll have to undo when LocaleManager moves into android-services. (I don't think it's worth introducing a delegate interface here.)

But quite apart from that: LocaleManager has to be initialized before FHR, because the two locales need to be initialized.

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8369855 [details] [diff] [review]
Patch v3 - FHR stuff

Review of attachment 8369855 [details] [diff] [review]:
-----------------------------------------------------------------

This is going to break when mcomella lands Bug 966143 -- each app will have its own HealthRecorder class. That means that including the LocaleManager invocation inside the health recorder init isn't going to work. (Also it doesn't seem right to do so.)

The OOM tracker and telemetry stuff also needs to move back, so probably this whole thing is a bit of f-.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +1014,5 @@
> +        @Override
> +        public void run() {
> +            ThreadUtils.assertOnBackgroundThread();
> +
> +            final SharedPreferences prefs = GeckoApp.getAppSharedPreferences();

I'd prefer to not accrue additional dependencies on GeckoApp itself here.

@@ +1018,5 @@
> +            final SharedPreferences prefs = GeckoApp.getAppSharedPreferences();
> +
> +            SessionInformation previousSession = SessionInformation.fromSharedPrefs(prefs);
> +            if (previousSession.wasKilled()) {
> +                Telemetry.HistogramAdd("FENNEC_WAS_KILLED", 1);

This (and the OOM stuff below) isn't part of the FHR code; probably best left where it was.

@@ +1038,5 @@
> +            final EventDispatcher dispatcher = GeckoAppShell.getEventDispatcher();
> +            Log.i(LOG_TAG, "Creating BrowserHealthRecorder.");
> +
> +            final String osLocale = Locale.getDefault().toString();
> +            String appLocale = LocaleManager.getAndApplyPersistedLocale();

This should occur in BrowserApp/GeckoApp, not inside BHR. (See overview comment, too.)

Comment 10

[Wesley Johnston (:wesj)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/60651784715f

Comment 12

[Wesley Johnston (:wesj)]

backed out for startup regressions:
https://hg.mozilla.org/integration/fx-team/rev/d45483106791

Comment 13

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

Even with this in the delayed startup and in a background thread, I see this taking ~100ms. I wonder if we should use a background service for this, or find idle time.

Comment 14

[Richard Newman [:rnewman]]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> Even with this in the delayed startup and in a background thread, I see this
> taking ~100ms. I wonder if we should use a background service for this, or
> find idle time.

I've been thinking about having a background service to which we send a broadcast intent *onPause*. Have that do all of the dirty work.

Comment 15

[Wesley Johnston (:wesj)]

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

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or IDL/UUID changes made by this patch:

[Approval Request Comment]
Bug caused by (feature/regressing bug #): This caused a startup regression. Requesting uplift of the backout.
User impact if declined: Startup regression
Testing completed (on m-c, etc.): Been on mc for a week.
Risk to taking this patch (and alternatives if risky): low risk
String or IDL/UUID changes made by this patch: none.

Comment 16

[Myk Melez [:myk] [@mykmelez]]

Martyn and I have been digging into this over the last few days.  The uninstall receiver <http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#174> handles most uninstalls, but it doesn't work if Fennec is killed via the "force stop" button in Android settings or a third-party process manager.  And the uninstall package scanner is designed to handle those cases.  But those cases are unlikely enough that it seems preferable to delay the scan until well after startup to fix any startup performance regression it causes.

Waiting until well after startup does introduce a potential race: if the user kills Fennec, uninstalls an app, then restarts Fennec and immediately loads a page that checks the app's install status (like the Marketplace page from which they installed the app or Fennec's own about:apps page), then the page will display an incorrect status.  But that unlikely circumstance still seems better than a startup regression at this point.

So we think we should reintroduce the delay, making it as long as needed to ensure it fixes any startup regression.  And then continue to iterate on a further improvement that addresses the potential race.  Regarding that further improvement, one thought we had is to delay the scan until the first mozApps API call, which we would then delay pending completion of the scan.  But that requires further investigation.

Comment 17

[Wesley Johnston (:wesj)]

Great. I relanded this:
https://hg.mozilla.org/integration/fx-team/rev/f87a3c18196b

Lets watch eideticker closely and make sure this doesn't make things worse :)

Comment 18

[Ryan VanderMeulen [:RyanVM]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/859a9b3b09de