1124492 - Allow for distribution intent processing to occur after first use

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Richard Newman [:rnewman]]

Sometimes, even after Bug 1105590, we get the referrer intent way, way after launch -- on the order of five seconds.

There is some evidence that this is network related. It only repros for me on a cellular connection.

To make this reliable, we essentially have to be ready for a distribution intent to arrive at any time.

Comment 1

[Richard Newman [:rnewman]]

Attachment: Allow for distribution intent processing to occur after first use. WIP

Work in progress.

This keeps the ready queue around, turning it into a "delayed ready" queue.

Haven't hooked up the delayed invocation yet.

Most of the time, this approach should work fine.

Comment 2

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=75d61f85ed63

Comment 3

[:Margaret Leibovic]

Comment on attachment 8552875 [details] [diff] [review]
Allow for distribution intent processing to occur after first use. WIP

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

Looks reasonable to me. I just don't like how our distribution logic continues to grow hairier with every bug we fix :(

::: mobile/android/base/GeckoProfile.java
@@ +876,5 @@
> +                    // We assume we've been called very soon after startup, and so our offset
> +                    // into "Mobile Bookmarks" is the number of bookmarks in the DB.
> +                    final ContentResolver cr = context.getContentResolver();
> +                    final int offset = db.getCount(cr, "bookmarks");
> +                    db.addDistributionBookmarks(cr, distribution, offset);

Do distributions even want bookmarks anymore? Now that we support suggested sites, I think we should see if we can get rid of this bookmark functionality (or at least deprecate it). It seems like it's more pain that its worth.

::: mobile/android/base/health/BrowserHealthRecorder.java
@@ +569,5 @@
> +                    if (state == State.INITIALIZING) {
> +                        initializeStorage();
> +                    } else {
> +                        onEnvironmentChanged();
> +                    }

I'm not familiar with the details of the BHR profile cache, but this logic seems to make sense.

::: mobile/android/search/java/org/mozilla/search/providers/SearchEngineManager.java
@@ +155,5 @@
> +            @Override
> +            public void distributionArrivedLate(Distribution distribution) {
> +                // Let's see if there's a name in the distro.
> +                // If so, just this once we'll override the saved value.
> +                final String name = getDefaultEngineNameFromDistribution();

Good thing we have these handy helper methods! :)

getDefaultEngineNameFromDistribution will return null if it doesn't find a search engine preference, we should bail if that's the case.

Comment 4

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d24ce349b679

Comment 5

[Richard Newman [:rnewman]]

Status update:

* Top sites works!
* Search engine works, but the added engine appears at position #4 until you restart.
* I haven't tested ActivityChooserModel, but see Bug 1021176.
* I'm not seeing any added bookmarks at all, which is weird because I stepped through that function. Will continue to investigate.

Comment 6

[Richard Newman [:rnewman]]

> * I'm not seeing any added bookmarks at all, which is weird because I
> stepped through that function. Will continue to investigate.

Ah, bookmarks did work. The bookmarks pane doesn't show pinned bookmarks.

Comment 7

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

(In reply to Richard Newman [:rnewman] from comment #6)
> > * I'm not seeing any added bookmarks at all, which is weird because I
> > stepped through that function. Will continue to investigate.
> 
> Ah, bookmarks did work. The bookmarks pane doesn't show pinned bookmarks.

Yeah, that stumped me a while ago too. I don't think pinned bookmarks are meaningful anymore?

Comment 8

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

(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Richard Newman [:rnewman] from comment #6)
> > > * I'm not seeing any added bookmarks at all, which is weird because I
> > > stepped through that function. Will continue to investigate.
> > 
> > Ah, bookmarks did work. The bookmarks pane doesn't show pinned bookmarks.
> 
> Yeah, that stumped me a while ago too. I don't think pinned bookmarks are
> meaningful anymore?

https://bugzilla.mozilla.org/show_bug.cgi?id=1086242#c11

Comment 9

[Richard Newman [:rnewman]]

Attachment: Allow for distribution intent processing to occur after first use. v1

This works in manual testing, both of search activity and launching browser.

Repeated intents are (correctly) dropped on the floor. Same for intents received later than first run.

Intents sent before/during initial startup are handled correctly, with one oddity: it seems to be possible for pinned bookmark insertion to race with inserting default bookmarks, so I'll work on a follow-up patch to fix that.

This also removes the 400msec delay. We shouldn't need it now.

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8554058 [details] [diff] [review]
Allow for distribution intent processing to occur after first use. v1

I have a much simpler (and more thread safe!) version coming.

Comment 11

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6129949ed796

Comment 12

[Richard Newman [:rnewman]]

Attachment: Allow for distribution intent processing to occur after first use. v2

This works.

Comment 13

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5abea60375aa

Comment 14

[Richard Newman [:rnewman]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6cecfdefa7f0

Comment 15

[Richard Newman [:rnewman]]

Try is green on all versions.

Comment 16

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> (In reply to Richard Newman [:rnewman] from comment #6)
> > > * I'm not seeing any added bookmarks at all, which is weird because I
> > > stepped through that function. Will continue to investigate.
> > 
> > Ah, bookmarks did work. The bookmarks pane doesn't show pinned bookmarks.
> 
> Yeah, that stumped me a while ago too. I don't think pinned bookmarks are
> meaningful anymore?

They should show up on the top sites page, right?

It's a bit redundant with suggested sites, but I think the difference is that pinned bookmarks will never go away without user action, whereas suggested sites would get bumped down with our frecency logic.

But maybe we should remove this ability, since I think we only added support for pinned bookmarks in the first place to let distributions specify where those tiles ended up in the top sites grid.

Comment 17

[Richard Newman [:rnewman]]

> > Yeah, that stumped me a while ago too. I don't think pinned bookmarks are
> > meaningful anymore?
> 
> They should show up on the top sites page, right?

Yeah, they do, as pinned.

Comment 18

[:Margaret Leibovic]

Comment on attachment 8554229 [details] [diff] [review]
Allow for distribution intent processing to occur after first use. v2

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

This referrer logic is tricky, but overall this looks fine to me. I just want to make sure we're clear about our assumptions about when we get these referrer intents and what they're used for (i.e. downloadable distributions vs. simple campaign tracking).

::: mobile/android/base/distribution/Distribution.java
@@ +292,5 @@
> +     */
> +    private void processDelayedReferrer(final ReferrerDescriptor ref) {
> +        ThreadUtils.assertOnBackgroundThread();
> +        if (state != STATE_NONE) {
> +            return;

When would we receive a referrer but already have a distribution set up? And why is it okay to bail in that case?

@@ +304,5 @@
> +            return;
> +        }
> +
> +        // Persist our new state.
> +        this.state = STATE_SET;

So we consider a distribution STATE_SET even if it's not a real distribution, but rather just a download that came from a referrer link? Or do we also consider that a proper distribution, even in the case where there's not a downloadable distribution?

I get confused between our use of distributions as a customization vehicle and as a campaign tracking tool. I think we should update our distribution wiki page to explain the nuances here.

@@ -389,5 @@
>          if (referrer == null) {
> -            // Wait a predetermined time and try again.
> -            // Just block the thread, because it's the simplest solution.
> -            try {
> -                Thread.sleep(DELAY_WAIT_FOR_REFERRER_MSEC);

Happy to see this go.

@@ +859,5 @@
> +            @Override
> +            public void run() {
> +                // Sanity.
> +                if (state != STATE_SET) {
> +                    return;

Should we report an error here? This should never happen.

::: mobile/android/chrome/content/browser.js
@@ +7547,5 @@
>      switch (aTopic) {
> +      case "Distribution:Changed":
> +        // Re-init the search service.
> +        try {
> +          Services.search._asyncReInit();

Hm... is it dangerous to be calling this "private" JS method? Should we instead be doing this with a notification?

Comment 19

[Richard Newman [:rnewman]]

(In reply to :Margaret Leibovic from comment #18)

> When would we receive a referrer but already have a distribution set up? And
> why is it okay to bail in that case?

If you click a referrer link on a device that has a /system distribution, for example. I don't think it makes sense to try to apply a distribution twice -- we'd have two sets of pinned bookmarks, two 'default' search engines, etc.

In theory it could also happen if we got a referrer intent twice.


> So we consider a distribution STATE_SET even if it's not a real
> distribution, but rather just a download that came from a referrer link? Or
> do we also consider that a proper distribution, even in the case where
> there's not a downloadable distribution?

A downloadable distribution is a real distribution; we determine the distribution from the referrer (for tracking purposes), and subsequently it might resolve to a downloadable distro. The check just above will short-circuit if there's no download; if it succeeds, it means we managed to download a distro for that ID, and we move to STATE_SET. We don't use STATE_SET unless we got a real distro (from /system, from the APK, or from a referrer), and thus can load files from it.


> I get confused between our use of distributions as a customization vehicle
> and as a campaign tracking tool. I think we should update our distribution
> wiki page to explain the nuances here.

Yeah, it confuses me a little, too. I'm comforted by the fact that I haven't changed the distribution logic here, only when it applies.


> Should we report an error here? This should never happen.

Sure.


> > +          Services.search._asyncReInit();
> 
> Hm... is it dangerous to be calling this "private" JS method? Should we
> instead be doing this with a notification?

Yeah, it's kinda dangerous. I opted to wrap with try..catch and hope for the best; all this does is make sure that the search service re-inits when you change the search engines. If it fails, the change will take effect on the next Gecko launch.

I personally added that _asyncReInit method in an earlier bug, so it's less dangerous than some :)

Comment 20

[Richard Newman [:rnewman]]

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

Comment 21

[Richard Newman [:rnewman]]

Comment on attachment 8554229 [details] [diff] [review]
Allow for distribution intent processing to occur after first use. v2

Approval Request Comment
[Feature/regressing bug #]:
  Oh Android.

[User impact if declined]:
  Unable to use OTA distributions, which is bad from a partnership standpoint.

[Describe test coverage new/current, TreeHerder]:
  Existing distribution tests pass. Manually tested functionality on Play.

[Risks and why]: 
  Changes distribution processing. In theory this could break shipping distros, cause errors during startup, etc.

  I'm generally satisfied with the amount of testing I've done. A night or two baking on Nightly should be enough thereafter. This is risk/reward.

[String/UUID change made/needed]:
  None.

Comment 22

[Ekanan Ketunuti]

https://hg.mozilla.org/mozilla-central/rev/9bfda2f52e82

Comment 23

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8554229 [details] [diff] [review]
Allow for distribution intent processing to occur after first use. v2

Taking it in aurora to improve the testing.
If no regression, I will take it next Monday for beta 6.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/33d9aa8ca571

Comment 25

[Richard Newman [:rnewman]]

This applies cleanly to Beta.

This *should* be safe to just uplift, but apologies for not being able to test a build.

I'm unable to do so, because a clean clobber build of Beta as-is fails on my machine:

20:23.19 check [lib] directlyIndependentOf [main]
20:23.19   Unexpected dependencies found:
20:23.19   org.mozilla.gecko.GeckoThread
20:23.19     -> org.mozilla.gecko.BrowserApp
20:23.19   org.mozilla.gecko.GeckoProfile
20:23.19     -> org.mozilla.gecko.BrowserApp
20:23.19   org.mozilla.gecko.GeckoEvent
20:23.19     -> org.mozilla.gecko.GeckoHalDefines

Nick, please ping me on IRC with thoughts on this.

Comment 26

[Nick Alexander :nalexander [he/him]]

(In reply to Richard Newman [:rnewman] from comment #25)
> This applies cleanly to Beta.
> 
> This *should* be safe to just uplift, but apologies for not being able to
> test a build.
> 
> I'm unable to do so, because a clean clobber build of Beta as-is fails on my
> machine:
> 
> 20:23.19 check [lib] directlyIndependentOf [main]
> 20:23.19   Unexpected dependencies found:
> 20:23.19   org.mozilla.gecko.GeckoThread
> 20:23.19     -> org.mozilla.gecko.BrowserApp
> 20:23.19   org.mozilla.gecko.GeckoProfile
> 20:23.19     -> org.mozilla.gecko.BrowserApp
> 20:23.19   org.mozilla.gecko.GeckoEvent
> 20:23.19     -> org.mozilla.gecko.GeckoHalDefines
> 
> Nick, please ping me on IRC with thoughts on this.

This is a compiler-dependent issue with Classycle.  We disabled Classycle in Bug 1107134.  Uplift the fix and you should be good.  Sorry :/

Comment 27

[Richard Newman [:rnewman]]

Thanks, Nick.

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8554229 [details] [diff] [review]
Allow for distribution intent processing to occur after first use. v2

Seems safe. Taking it in beta 6 (gtb tomorrow)

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/2dd8d79e19e4

Comment 30

[Richard Newman [:rnewman]]

Manually verified in Beta.

Comment 31

[Florian Quèze [:florian]]

(In reply to Richard Newman [:rnewman] from comment #19)

> > > +          Services.search._asyncReInit();
> > 
> > Hm... is it dangerous to be calling this "private" JS method? Should we
> > instead be doing this with a notification?
> 
> Yeah, it's kinda dangerous. I opted to wrap with try..catch and hope for the
> best; all this does is make sure that the search service re-inits when you
> change the search engines. If it fails, the change will take effect on the
> next Gecko launch.
> 
> I personally added that _asyncReInit method in an earlier bug, so it's less
> dangerous than some :)

Are you sure this _actually_ works? _asyncReInit isn't (and shouldn't) be exposed in the nsIBrowserSearchService interface, so it shouldn't be visible when using Services.search.

The hack used to trigger a search service restart in unit tests is:
   const kLocalePref = "general.useragent.locale";
   Services.search.QueryInterface(Ci.nsIObserver)
           .observe(null, "nsPref:changed", kLocalePref);

Comment 32

[Richard Newman [:rnewman]]

We tested it at the time -- Bug 1018240. That doesn't guarantee that it still works, or still works in all scenarios, of course!

Bug 1041665 is on file to do this better, so we'd be happy to revisit when someone tackles that.