Closed
Bug 1330714
Opened 7 years ago
Closed 7 years ago
Showing start wizard can't be overridden via distribution
Categories
(Firefox for Android Graveyard :: First Run, defect)
Firefox for Android Graveyard
First Run
Tracking
(firefox52 wontfix, firefox53 fixed, firefox54 fixed)
RESOLVED
FIXED
Firefox 54
People
(Reporter: mkaply, Assigned: mkaply)
References
Details
Attachments
(1 file, 5 obsolete files)
9.45 KB,
patch
|
rnewman
:
review+
gchang
:
approval-mozilla-aurora+
jcristau
:
approval-mozilla-beta-
|
Details | Diff | Splinter Review |
Because distributions are read on a callback, by the time they are actually read, the firstrun pref has been checked and we have shown the wizard. In cases where the distribution is physically on the device, we should read the distributon preferences as soon as possible (before firstrun). enqueueInitilization happens before checking firstrun, but setting the distribution preferences happens after. Note my testing is done on release and some of this code has been refactored, but the problem persists.
Comment 1•7 years ago
|
||
This is a tricky one. We want to show the app UI (and with that the first run tour) as soon as possible - We do not want to delay app start. However applying a distribution requires disk I/O and sometimes a bunch of files need to be copied around - This is slow and should not block the UI.
Assignee | ||
Comment 2•7 years ago
|
||
The only thing we need from the distribution at this point are the AndroidPreferences. My suggestion that we handle those independent of the rest of the distribution. So we can grab them as early as possible.
Assignee | ||
Comment 3•7 years ago
|
||
Could we delay firstrun until the distribution callback similar to suggestedsites? https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/db/SuggestedSites.java#284
Comment 4•7 years ago
|
||
Sebastian, let us know if/how we can proceed here and/or if you can delegate somebody from the Taipei team to assist here.
Flags: needinfo?(s.kaspari)
Comment 5•7 years ago
|
||
Mh. Maybe we could try to trigger the first run screen from a Distribution.ReadyCallback (found/notFound)? But we need to make sure that this is not happening too late (especially with OTAs). There could be edge cases that make this a bit more complicated. Julian (CC) has been working on some distribution related patches recently. But he might be busy with custom tabs now. Let's add it to the core browser roadmap and prioritize?
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 6•7 years ago
|
||
OTAs aren't an issue in this case. I'm only interested in distributions on the device. So we might want to have the distribution code distinguish between the two.
Assignee | ||
Comment 7•7 years ago
|
||
FYI, this is really high priority. We could end up on a lot more devices if we can get this solved.
Comment 8•7 years ago
|
||
Checking for a system distribution is file IO, but it's very bounded. I see no reason why we couldn't do the following when deciding whether to trigger first run: - Check whether a system distribution exists. (Pretty much just call Distribution.checkSystemDistribution().) - If found, it means that Distribution will be working with a system distribution, which will be purely local file copies. Chain first-run off the main distro ready callback. - If not, show first run as normal. I'm happy to advise.
Assignee | ||
Comment 9•7 years ago
|
||
Based on your idea, I was able to get this working, but in the process discovered that the preference name for start pane was named "wrong" (No NON_PREF_PREFIX) so it wasn't being set by the distribution anyway. When I moved it to a NON_PREF_PREFIX, it worked without this code change, but I think there is a timing issue so we should do it anyway. So patch coming.
Assignee | ||
Comment 10•7 years ago
|
||
There were two core problems here. 1. The pref wasn't named with the "not an android pref" so it couldn't be set from a distribution. 2. We need to delay first run if there is a system distro. I thought doing 1 might be enough, but it doesn't fix it every time, so it's a race condition. So we need 2. I have no idea if there is a more clever way to do the code I've written.
Attachment #8836221 -
Flags: review?(rnewman)
Comment 11•7 years ago
|
||
Comment on attachment 8836221 [details] [diff] [review] Fix preference name and do firstrun on callback Review of attachment 8836221 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java @@ +941,5 @@ > try { > final SharedPreferences prefs = GeckoSharedPrefs.forProfile(this); > > + if (prefs.contains(FirstrunAnimationContainer.PREF_FIRSTRUN_ENABLED_OLD)) { > + // Migrate the old pref and remove it I hesitate to add a bit of rot to the world, but it would be much cheaper and simpler to simply check both prefs for a few releases -- no need to kick off a write, and the read is essentially free. ::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java @@ +685,5 @@ > > /** > * @return true if we found a system distribution. Sets distributionDir in that case. > */ > + public boolean checkSystemDistribution() { This method isn't thread-safe, because it sets `distributionDir` as a side-effect. It's private for that reason, among others. You should keep this method private and introduce a new method, `public static boolean hasSystemDistribution(Context)`, that checks the same directories but doesn't have that side effect. If when you were testing you noticed that sometimes we'd be racing with real distro init, then you can do less work at runtime: do the `getInstance` flow you're using and check `.state`, which is volatile and has a predictable state machine. If it's STATE_NONE, there's no distro; proceed with normal first run. If it's STATE_SET, use that distro (just as you do in this patch). If it's STATE_UNKNOWN, use the new `Distribution.hasSystemDistribution` method to decide whether to wait.
Attachment #8836221 -
Flags: review?(rnewman) → review-
Updated•7 years ago
|
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
OS: Unspecified → All
Hardware: Unspecified → All
Assignee | ||
Comment 12•7 years ago
|
||
> I hesitate to add a bit of rot to the world, but it would be much cheaper and simpler to simply check both prefs for a few releases -- no need to kick off a write, and the read is essentially free.
I thought about that, but because it's a boolean, I couldn't come up with a good way to know which one was the value I should use. I can't query one right after the other because of the default value.
Assignee | ||
Comment 13•7 years ago
|
||
Better patch. 1. I can use && to check the two preferences. If 1 or the other is set, don't do first run, if both are false, do it. 2. Encapsulated everything into one function. We only need to do the distiribution callback if we are in the unknown state. In the other two cases, we can go down the normal path because we can access the distribution (or we don't need to).
Attachment #8836256 -
Flags: review?(rnewman)
Comment 14•7 years ago
|
||
Comment on attachment 8836256 [details] [diff] [review] Better patch Review of attachment 8836256 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java @@ +1027,5 @@ > + if (distribution.shouldWaitForDistribution()) { > + distribution.addOnDistributionReadyCallback(new Distribution.ReadyCallback() { > + @Override > + public void distributionNotFound() { > + checkFirstrun(getApplicationContext(), new SafeIntent(getIntent())); I think in all of these cases you should use `BrowserApp.this` instead of `getApplicationContext()`. @@ +1041,5 @@ > + checkFirstrun(getApplicationContext(), new SafeIntent(getIntent())); > + } > + }); > + } else { > + checkFirstrun(this, new SafeIntent(getIntent())); Four-space indent. ::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java @@ +690,5 @@ > return checkDirectories(getSystemDistributionDirectories(context)); > } > > /** > + * @return true if we should wait for a distribution to load Note the limitations: this doesn't reliably check for an APK or OTA distribution, so this is not as general as it looks! @@ +694,5 @@ > + * @return true if we should wait for a distribution to load > + */ > + public boolean shouldWaitForDistribution() { > + if (state == STATE_NONE || state == STATE_SET) { > + return true; If it's `STATE_NONE` there is no distribution, so you might as well return false. That is: we should do this work after the distro is loaded either: 1. If there's a distro (STATE_SET) 2. If there's a system distribution directory. @@ +697,5 @@ > + if (state == STATE_NONE || state == STATE_SET) { > + return true; > + } > + > + String[] directories = getSystemDistributionDirectories(context); final. @@ +699,5 @@ > + } > + > + String[] directories = getSystemDistributionDirectories(context); > + for (String path : directories) { > + File directory = new File(path); final. ::: mobile/android/base/java/org/mozilla/gecko/firstrun/FirstrunAnimationContainer.java @@ +24,5 @@ > * A container for the pager and the entire first run experience. > * This is used for animation purposes. > */ > public class FirstrunAnimationContainer extends LinearLayout { > + public static final String PREF_FIRSTRUN_ENABLED_OLD = "startpane_enabled"; Add a brief comment pointing back to this bug.
Assignee | ||
Comment 15•7 years ago
|
||
> If it's `STATE_NONE` there is no distribution, so you might as well return false. > That is: we should do this work after the distro is loaded either: > 1. If there's a distro (STATE_SET) > 2. If there's a system distribution directory. If we have STATE_SET, we should return false too, right? Because we should already have a distribution at that point, so we can check the first run preference. The only case where we need to do the work is when we are STATE_UNKNOWN with a system directory, right?
Comment 16•7 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #15) > If we have STATE_SET, we should return false too, right? Because we should > already have a distribution at that point, so we can check the first run > preference. No. STATE_SET just means that we know we have a distro, and where it is. It doesn't mean that we've run all of the distro callbacks… and the distro callbacks are where the prefs you're looking at are extracted (GeckoApplication.java#264). Indeed, we're relying here that GeckoApplication's own onProfileCreate handler is queued up before the first run code you're touching here… otherwise we could end up checking for prefs that haven't been extracted yet!
Assignee | ||
Comment 17•7 years ago
|
||
Thanks for your patience on this. This should fix remaining issues.
Attachment #8836221 -
Attachment is obsolete: true
Attachment #8836256 -
Attachment is obsolete: true
Attachment #8836256 -
Flags: review?(rnewman)
Attachment #8836302 -
Flags: review?(rnewman)
Comment 18•7 years ago
|
||
Comment on attachment 8836302 [details] [diff] [review] One more time Review of attachment 8836302 [details] [diff] [review]: ----------------------------------------------------------------- LGTM!
Attachment #8836302 -
Flags: review?(rnewman) → review+
Comment 19•7 years ago
|
||
Comment on attachment 8836302 [details] [diff] [review] One more time Note that we flip the first run pref the first time we go through checkFirstRun: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#965 I think you should move your new code *inside* checkFirstRun, not around it, and do the distribution check *inside* the existing short-circuit check. That'll avoid touching the distribution code at all on subsequent activity resumes. Additionally -- and this is why I'm flipping the review flag -- I realized that the first-run code needs to run on the main thread, and distribution callbacks are run on the background thread. In summary: - Don't touch distribution stuff if the first-run pref is already false. - The distribution callbacks you've added need to re-dispatch to the UI thread. Sorry for the churn, mkaply!
Flags: needinfo?(mozilla)
Attachment #8836302 -
Flags: review+ → review-
Assignee | ||
Comment 20•7 years ago
|
||
> I realized that the first-run code needs to run on the main thread, and distribution callbacks are run on the background thread. I'm confused by this. Earlier in this file (the init), a distribution callback is run on the main thread. isn't it? https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#776 Is doing this as simple as putting all the code inside this: ThreadUtils.postToUiThread(new Runnable() { @Override public void run() { // DO DISTRIBUTION CODE HERE } });
Flags: needinfo?(mozilla)
Assignee | ||
Comment 21•7 years ago
|
||
This moves the code into the pref check. I don't have the thread dispatch done yet.
Assignee | ||
Comment 22•7 years ago
|
||
Addresses review comments by moving all checks into the pref checks and posting to the UI thread. Also fixes a crash I found where you have a partial distribution directory with no preferences.json.
Attachment #8836302 -
Attachment is obsolete: true
Attachment #8837327 -
Attachment is obsolete: true
Attachment #8837350 -
Flags: review?(rnewman)
Comment 23•7 years ago
|
||
(In reply to Mike Kaply [:mkaply] from comment #20) > I'm confused by this. Earlier in this file (the init), a distribution > callback is run on the main thread. isn't it? No. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java#957 and the code: https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java#1004
Comment 24•7 years ago
|
||
Comment on attachment 8837350 [details] [diff] [review] Patch addressing review comments Review of attachment 8837350 [details] [diff] [review]: ----------------------------------------------------------------- ::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java @@ +920,5 @@ > /** > + * Code to actually show the first run pager, separated > + * for distribution purposes. > + */ > + private void checkFirstrunInternal() { You'll want a @UiThread annotation on this. import android.support.annotation.UiThread; at the top. @@ +963,2 @@ > if (!Intent.ACTION_VIEW.equals(intent.getAction())) { > + // Check to see if a distribution has turned off the first run pager Nit: closing period. @@ +963,4 @@ > if (!Intent.ACTION_VIEW.equals(intent.getAction())) { > + // Check to see if a distribution has turned off the first run pager > + final Distribution distribution = Distribution.getInstance(BrowserApp.this); > + if (distribution.shouldWaitForSystemDistribution()) { Invert this conditional, so the really short and common `else` moves up here. @@ +976,5 @@ > + } > + > + @Override > + public void distributionFound(final Distribution distribution) { > + // Check preference again in case distribution turned it off Nit: closing period. @@ +988,5 @@ > + } > + } > + > + @Override > + public void distributionArrivedLate(final Distribution distribution) { I'm not sure you want to do this. If a distro arrives 10 seconds after launching the browser, do you want to open the tour? I believe distributionNotFound will take care of this case, so you can just leave this method empty. Test and see! ::: mobile/android/base/java/org/mozilla/gecko/distribution/Distribution.java @@ +694,5 @@ > } > > /** > + * @return true if we should wait for a system distribution to load. > + * Not applicable to APK or OTA distributions Nit: closing period. @@ +706,5 @@ > + } > + > + final String[] directories = getSystemDistributionDirectories(context); > + for (String path : directories) { > + Log.d("Kaply", path); Ahem. @@ +708,5 @@ > + final String[] directories = getSystemDistributionDirectories(context); > + for (String path : directories) { > + Log.d("Kaply", path); > + final File directory = new File(path); > + if (directory != null && directory.exists()) { You just used a constructor. `directory` will never be null.
Attachment #8837350 -
Flags: review?(rnewman) → feedback+
Assignee | ||
Comment 25•7 years ago
|
||
New patch with comments addressed. I've tested the indicated scenarios with system and OTA distributions and everything is working as expected.
Attachment #8837350 -
Attachment is obsolete: true
Attachment #8837776 -
Flags: review?(rnewman)
Comment 26•7 years ago
|
||
Comment on attachment 8837776 [details] [diff] [review] Review comments addressed Review of attachment 8837776 [details] [diff] [review]: ----------------------------------------------------------------- LGTM; thanks for your patience. Run lint on this if you can; you might prepare for it to bounce due to linter issues.
Attachment #8837776 -
Flags: review?(rnewman) → review+
Assignee | ||
Comment 27•7 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/20feb033b19b569aa876dc980727c1505ef8a0a1 Bug 1330714 - Allow distributions to override firstrun wizard. r=rnewman
Comment 28•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/20feb033b19b
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox54:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 54
Assignee | ||
Comment 29•7 years ago
|
||
Comment on attachment 8837776 [details] [diff] [review] Review comments addressed Approval Request Comment [Feature/Bug causing the regression]: Provide ability for system distributions to turn off the startup wizard. [User impact if declined]: No user impact. Requested by partners. [Is this code covered by automated tests?]: No [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: Lost a system distribution on a rooted device with "startpane_enabled": false in AndroidPreferences. (See https://wiki.mozilla.org/Mobile/Distribution_Files) [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Extremely low risk [Why is the change risky/not risky?]: Code is only executed if system distribution is on the device. [String changes made/needed]: None Aurora is the main ask. I'm asking for beta as it's a "nice to have"
Attachment #8837776 -
Flags: approval-mozilla-beta?
Attachment #8837776 -
Flags: approval-mozilla-aurora?
Updated•7 years ago
|
status-firefox52:
--- → affected
Comment 30•7 years ago
|
||
Comment on attachment 8837776 [details] [diff] [review] Review comments addressed Support partner to provide the ability for system distributions to turn off the startup wizard and was verified locally. Aurora53+.
Attachment #8837776 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 31•7 years ago
|
||
Comment on attachment 8837776 [details] [diff] [review] Review comments addressed If we can live without this change in 52 then I'd rather do that at this stage.
Attachment #8837776 -
Flags: approval-mozilla-beta? → approval-mozilla-beta-
Comment 32•7 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/8d0c434a7542
Updated•7 years ago
|
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•