Closed Bug 1124492 Opened 7 years ago Closed 7 years ago

Allow for distribution intent processing to occur after first use

Categories

(Firefox for Android Graveyard :: General, defect)

36 Branch
All
Android
defect
Not set
normal

Tracking

(firefox36 verified, firefox37 fixed, firefox38 fixed, fennec36+)

VERIFIED FIXED
Firefox 38
Tracking Status
firefox36 --- verified
firefox37 --- fixed
firefox38 --- fixed
fennec 36+ ---

People

(Reporter: rnewman, Assigned: rnewman)

References

Details

Attachments

(1 file, 2 obsolete files)

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.
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.
Assignee: nobody → rnewman
Status: NEW → ASSIGNED
Attachment #8552875 - Flags: feedback?(margaret.leibovic)
tracking-fennec: ? → 36+
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.
Attachment #8552875 - Flags: feedback?(margaret.leibovic) → feedback+
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.
> * 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.
(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?
(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
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.
Attachment #8554058 - Flags: review?(margaret.leibovic)
Attachment #8552875 - Attachment is obsolete: true
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.
Attachment #8554058 - Flags: review?(margaret.leibovic)
This works.
Attachment #8554229 - Flags: review?(margaret.leibovic)
Attachment #8554058 - Attachment is obsolete: true
Try is green on all versions.
(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.
> > 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 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?
Attachment #8554229 - Flags: review?(margaret.leibovic) → review+
(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 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.
Attachment #8554229 - Flags: approval-mozilla-beta?
Attachment #8554229 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/9bfda2f52e82
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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.
Attachment #8554229 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Flags: needinfo?(nalexander)
(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 :/
Flags: needinfo?(nalexander) → needinfo?(rnewman)
Thanks, Nick.
Depends on: 1107134
Flags: needinfo?(rnewman)
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)
Attachment #8554229 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Manually verified in Beta.
Status: RESOLVED → VERIFIED
(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);
Flags: needinfo?(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.
Flags: needinfo?(rnewman)
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.