Closed Bug 1323759 Opened 7 years ago Closed 7 years ago

OTA distributions do not apply if browser is not used within a certain amount of time

Categories

(Firefox for Android Graveyard :: Android partner distribution, defect)

Product:
Firefox for Android Graveyard
Component:
Android partner distribution
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox51 fixed, firefox52 fixed, firefox53 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 53
Tracking Flags:
Tracking Status
firefox51 --- fixed
firefox52 --- fixed
firefox53 --- fixed

People

(Reporter: mkaply, Assigned: mkaply)

References

Details

Attachments

(1 file, 1 obsolete file)

Bug 1323759 - To ensure distribution is initialized
58 bytes, text/x-review-board-request
rnewman
: review+
jcristau
: approval-mozilla-aurora+
gchang
: approval-mozilla-beta+
Details 
+++ This bug was initially created as a clone of Bug #1320733 +++

I'm cloning so we can have a public bug without the partner document

If a referrer intent is used to stage a distribution, but the UI for the browser isn't brought for some period of time, the thread is killed and the OTA distro never happens.
Comment on attachment 8819164 [details]
Bug 1323759 - To ensure distribution is initialized

https://reviewboard.mozilla.org/r/99006/#review99566

I'm not convinced that this is worth the effort, but assuming you keep the behavior the same -- including not writing values back into SharedPreferences when read -- then I'm fine if you want to do this.

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:493
(Diff revision 1)
> +     * @param newState
> +     * @param inInitialization
> +     */
> +    private void updateDistributionState(int newState, boolean inInitialization) {
> +        if (newState != STATE_SET && newState != STATE_NONE) {
> +            Log.w(LOGTAG, "only accept STATE_SET or STATE_NONE");

Throw, don't log.

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:499
(Diff revision 1)
> +            return;
> +        }
> +
> +        // Persist our new state.
> +        this.state = newState;
> +        getSharedPreferences().edit().putInt(getKeyName(), this.state).apply();

Don't touch `SharedPreferences` unnecessarily. This is only necessary in two of the four call sites; the other two just loaded this value from `SharedPreferences`!
Attachment #8819164 - Flags: review?(rnewman) → review+
Comment on attachment 8819165 [details]
Bug 1323759 - Download distribution file by Install-Intent

https://reviewboard.mozilla.org/r/99008/#review99572

All of this distribution code was written under the assumption that an install referrer arrives some time after Firefox has started. Whenever Firefox starts it inits a distribution.

Assuming my understanding is correct, the only thing that changed in Android 5 is that we are handed this intent without first initializing any of the consumers of the distribution system. That means it's possible for the referrer intent to be handled by an uninitialized distribution.

The consequence of that is twofold: we don't know what state we're in (which triggers the early escape in `processDelayedReferrer`), and we also don't know if there's an on-disk distribution.

The only necessary fix, then, is to initialize the distribution prior to handling the referrer, just as we would have done implicitly in earlier versions of Android.

You need to do this anyway, to implement the check for an existing distribution. Once you've done so, `processDelayedReferrer` should act in the manner for which it was designed.

Note that `processDelayedReferrer` already has handling for `state != STATE_NONE`, so strictly speaking you don't even need the new warning on 298: you should just be able to trigger an init prior to enqueuing the background runnable.

Let me know if any of that doesn't make sense!

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:298
(Diff revision 1)
>          ThreadUtils.postToBackgroundThread(new Runnable() {
>              @Override
>              public void run() {
>                  final Distribution distribution = Distribution.getInstance(context);
> +                if (distribution.state == STATE_SET) {
> +                    Log.i(LOGTAG, "Got installation intent, however already set distribution");

Three changes here:

1. Warn, not info.
2. Minor language tweak: "Got INSTALL_REFERRER intent, but distribution already set."
3. The distribution you fetch here _is not initialized_, so in most cases `distribution.state` won't be set yet.

You need to make sure that the distribution is initialized, probably by fetching the instance via `Distribution.init(Context)` instead of using `getInstance`. `Distribution.init` posts the initialization to the background thread, returning the uninitialized `Distribution` instance, so call it _before_ you queue this runnable.

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:323
(Diff revision 1)
>              this.onLateReady.clear();
>              return;
>          }
>  
> +        if (this.state == STATE_UNKNOWN) {
> +            // The distribution instance have not been initialized, therefore stats is UNKNOWN.

And this clause should be unnecessary if you make sure you have an initialized distribution before calling `processDelayedReferrer`.
Attachment #8819165 - Flags: review?(rnewman) → review-
Attachment #8819165 - Attachment is obsolete: true
Attachment #8819165 - Flags: review?(s.kaspari)
Newman: Thanks for your patient on explaining this.

In previous patches I try to put some logic(what it is supposed to do in `doInit`) in Intent-handling. After reading your comments, I think to initialize Distribution instance then call `processDelayedReferrer` would be more consistent and easier to understand. So I dropped previous patches then create new one.

I use `getInstance` due to explicitly indicate which instance we are working on.

Please let me know if I miss anything, thank you!
Flags: needinfo?(rnewman)
Sounds like you have a good handle on what to do.

I'll be traveling for the next day or two, but I'll be online when I can; I'll watch for another review request.

Thank you for your patience!
Flags: needinfo?(rnewman)
Attachment #8819164 - Flags: review?(s.kaspari)
Comment on attachment 8819164 [details]
Bug 1323759 - To ensure distribution is initialized

https://reviewboard.mozilla.org/r/99006/#review103216

::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java:300
(Diff revision 3)
>              public void run() {
>                  final Distribution distribution = Distribution.getInstance(context);
>  
> +                // INSTALL_REFERRER intent triggers onReceivedReferrer callback, but app might have
> +                // not been launched. Need to make sure distribution is initialized.
> +                distribution.doInit();

I think this should happen after `processDelayedReferrer`, not before (in other words: move this to the end of the block). That would be the order of operation if the install intent arrived immediately prior to the app being opened.

The comment should then be something like:


```
// On Android 5+ we might receive the referrer intent and never
// actually launch the browser, which is the usual signal for
// the distribution init process to complete.
// Attempt to init here to handle that case.
// Profile setup that relies on the distribution will occur when
// the browser is eventually launched, via
// `addOnDistributionReadyCallback`.
```
Comment on attachment 8819164 [details]
Bug 1323759 - To ensure distribution is initialized

https://reviewboard.mozilla.org/r/99006/#review103216

> I think this should happen after `processDelayedReferrer`, not before (in other words: move this to the end of the block). That would be the order of operation if the install intent arrived immediately prior to the app being opened.
> 
> The comment should then be something like:
> 
> 
> ```
> // On Android 5+ we might receive the referrer intent and never
> // actually launch the browser, which is the usual signal for
> // the distribution init process to complete.
> // Attempt to init here to handle that case.
> // Profile setup that relies on the distribution will occur when
> // the browser is eventually launched, via
> // `addOnDistributionReadyCallback`.
> ```

OK I see the difference between two orders of operation. If `doInit` prior to `proessDelayedReferrer` and `state` is `STATE_NONE` (app launched), the `readyQueue` will be invoked in `doInit` before we process the referrer intent.
Attachment #8819164 - Flags: review?(s.kaspari)
I removed me from the list of reviewers. I have a long queue and if Richard has reviewed this then this should be okay.

However - is it correct that only the first patch survived and the second one is now marked as obsolete?
Sebastian: Yes, we only need to apply one(survived) patch.
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/11e5d90cbf4d
To ensure distribution is initialized r=rnewman
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/11e5d90cbf4d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
Comment on attachment 8819164 [details]
Bug 1323759 - To ensure distribution is initialized

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Approval Request Comment
[Feature/Bug causing the regression]:
[User impact if declined]:
[Is this code covered by automated tests?]:
[Has the fix been verified in Nightly?]:
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]:
[Why is the change risky/not risky?]:
[String changes made/needed]:

Bug 1323759 -  OTA distributions do not apply if browser is not used within a certain amount of time

Approval Request Comment
[Feature/regressing bug #]: To apply OTA distributions even if app not launch for a while.

[User impact if declined]: distribution file cannot be downloaded and applied correctly, if user install Fennec without launching it.

[Describe test coverage new/current, TreeHerder]: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c17a48c9926f0e171aa1d5c4b6e39ac8fd2ab62

[Risks and why]: Low. Only one line and just make sure it is initialized.

[String/UUID change made/needed]: -
Attachment #8819164 - Flags: approval-mozilla-beta?
Attachment #8819164 - Flags: approval-mozilla-aurora?
Comment on attachment 8819164 [details]
Bug 1323759 - To ensure distribution is initialized

fennec distribution fix, aurora52+
Attachment #8819164 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8819164 [details]
Bug 1323759 - To ensure distribution is initialized

Fix a distribution issue for fennec. Beta51+. Should be in 51 beta 14.
Attachment #8819164 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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: