Bug 1013024 (downloaddistro)

Handle install referrer intent to determine distribution

RESOLVED FIXED in Firefox 33

Status

()

RESOLVED FIXED
5 years ago
3 years ago

People

(Reporter: rnewman, Assigned: rnewman)

Tracking

(Depends on: 1 bug, Blocks: 1 bug)

Trunk
Firefox 33
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 12 obsolete attachments)

33.76 KB, patch
rnewman
: review+
Details | Diff | Splinter Review
16.85 KB, patch
mfinkle
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
We'd like for Fennec to be able to respond intelligently to tags included in Play Store referrer intents.

N.B., using Google Analytics is explicitly a non-goal.


Documentation links:

https://developers.google.com/analytics/devguides/collection/android/v1/devguide#google-play-builder

http://stevemiller.net/ReferrerTest/

https://github.com/rogerbinns/referraltester

http://gyurigrell.com/2012/02/tracking-install-sources-for-android-apps/
(Assignee)

Comment 1

5 years ago
This is a pretty trivial part 0. We'll be accruing more logic around distribution handling, and this is a nice step towards a svelte m/a/b/.
Attachment #8425203 - Flags: review?(margaret.leibovic)
(Assignee)

Comment 2

5 years ago
WIP for the first part of the meat. This approach catches and parses the incoming intent, aiming to deliver it to the Distribution handler before it does its initial work.

The idea is that if the referrer is there, we can do something more sophisticated, like picking a different distribution file.

A small extension is to say "oops" if we already loaded a distro, and do something else.

f? for the overall approach.
(Assignee)

Updated

5 years ago
Attachment #8425204 - Flags: feedback?(mark.finkle)
Comment on attachment 8425204 [details] [diff] [review]
Part 1: catch install intent and deliver it to the distribution handler. v1

Does this mean we can kill:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/ReferrerReceiver.java
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/AndroidManifest.xml.in#261

I'd probably lean more to naming InstallReceiver to ReferrerReceiver (assuming we kill the current file) since the focus is the "referrer" part and we already have a few other "Install" related receivers.
Attachment #8425204 - Flags: feedback?(mark.finkle) → feedback+
(Assignee)

Comment 4

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #3)

> I'd probably lean more to naming InstallReceiver to ReferrerReceiver
> (assuming we kill the current file) since the focus is the "referrer" part
> and we already have a few other "Install" related receivers.

Yeah, I'll be "eating" the old ReferrerReceiver as we go.

Updated

5 years ago
Attachment #8425203 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 5

5 years ago
A trivial utility I wanted to use in Part 1. This simply generalizes our existing thread asserts to support checking "anything but", and adds the utility for checking "not the UI thread".
Attachment #8425915 - Flags: review?(margaret.leibovic)

Updated

5 years ago
Attachment #8425915 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Updated

5 years ago
Depends on: 1013684
(Assignee)

Comment 6

5 years ago
Testing this:

* Clear data, kill app.
* Send an intent:

am broadcast -a com.android.vending.INSTALL_REFERRER -n org.mozilla.fennec_rnewman/org.mozilla.gecko.distribution.ReferrerReceiver --es "referrer" "utm_source=mozilla&utm_medium=testmedium&utm_term=testterm&utm_content=testcontent&utm_campaign=testname"

* Launch Fennec. You'll see debug output like:

I/GeckoDistribution(10572): Getting file from distribution.
I/GeckoDistribution(10572): Downloading referred distribution: https://people.mozilla.com/~rnewman/distro/testcontent.zip
I/GeckoDistribution(10572): Distribution fetch: 301
D/GeckoDistribution(10572): Redirecting to https://people.mozilla.org/~rnewman/distro/testcontent.zip
I/GeckoDistribution(10572): Distribution fetch: 200
D/GeckoDistribution(10572): Distro fetch took 16318ms; result? true
I/GeckoDistribution(10572): Copying files from fetched zip.
D/GeckoDistribution(10572): Considering distribution/searchplugins/common/engine.xml
D/GeckoDistribution(10572): Considering distribution/bookmarks.json
D/GeckoDistribution(10572): Considering distribution/preferences.json


Yes, that's a 16-second fetch. Ouch.
(Assignee)

Comment 7

5 years ago
WIP. This has some resource closure errors that I haven't addressed, which ultimately cause a failure, but it's most of the way there. I imagine I'll need to fix Bug 1013684 first -- that sixteen-second fetch blocks top sites, because we're handling the distribution too soon.
(Assignee)

Updated

5 years ago
Attachment #8425204 - Attachment is obsolete: true
(Assignee)

Comment 8

5 years ago
 I/GeckoDistribution(15146): Downloading referred distribution: https://people.mozilla.com/~rnewman/distro/testcontent.zip
 I/GeckoDistribution(15146): Getting file from distribution.
 I/GeckoDistribution(15146): Distribution fetch: 200
 D/GeckoDistribution(15146): Distro fetch took 1277ms; result? true
 I/GeckoDistribution(15146): Copying files from fetched zip.
 D/GeckoDistribution(15146): Considering distribution/searchplugins/common/engine.xml
 D/GeckoDistribution(15146): Considering distribution/bookmarks.json
 D/GeckoDistribution(15146): Considering distribution/preferences.json
 I/GeckoDistribution(15146): Getting file from distribution.
 I/GeckoDistribution(15146): Getting file from distribution.
 D/GeckoHealthRec(15146): Distribution is test-partner


Verified that the right distribution shows up in the only FHR environment, which is woo awesome.
(Assignee)

Updated

5 years ago
Depends on: 1014242
(Assignee)

Updated

5 years ago
Depends on: 1014283
(Assignee)

Updated

5 years ago
Attachment #8425934 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 1014338
(Assignee)

Comment 10

5 years ago
Comment on attachment 8425203 [details] [diff] [review]
Part 0: move Distribution class into its own package. v1

Moved to Bug 1014338.
Attachment #8425203 - Attachment is obsolete: true
(Assignee)

Comment 11

5 years ago
Comment on attachment 8425915 [details] [diff] [review]
Pre: add ThreadUtils.assertNotOnUiThread. v1

Moved to Bug 1014338.
Attachment #8425915 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8426618 - Attachment is obsolete: true
(Assignee)

Comment 13

5 years ago
Apparently all of my imports are disappearing today!
(Assignee)

Updated

5 years ago
Attachment #8426719 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Duplicate of this bug: 986119
(Assignee)

Comment 15

5 years ago
I duped Bug 986119 to this bug, 'cos this is the direction we're taking for now. There are patches in that bug for SIM-based approaches, should we need those.
Summary: Handle install referrer intent → Handle install referrer intent to determine distribution
(Assignee)

Updated

5 years ago
Depends on: 1016611
Attachment 8426762 [details] [diff] triggers a build failure due to a trivial class path issue in TestDistribution.java.  Here's a patch for the patch that fixes the problem.
(Assignee)

Comment 17

5 years ago
Comment on attachment 8429600 [details] [diff] [review]
patch to fix build failure in patch

This is fixed in my local copy; this bug is kinda dormant until the sub-bugs get through the queue.

Thanks for the fix, though!
Attachment #8429600 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Alias: downloaddistro
(Assignee)

Updated

5 years ago
Depends on: 923581
(Assignee)

Updated

5 years ago
Blocks: 1020032
(Assignee)

Updated

5 years ago
Depends on: 1021176
(Assignee)

Comment 18

5 years ago
Things to verify are loaded correctly from the distribution, either on first run or second:

1. Add-ons (second) -- Bug 923581 applies here
2. Prefs (first or second?)
3. Bookmarks (first)
4. Search engines (first?)
5. Themes, perhaps
6. Quickshare defaults (second until Bug 1021176 is implemented)


And not loaded from distributions yet:
1. Suggested sites.
(Assignee)

Comment 19

5 years ago
Now with telemetry. Tested by hand, with and without network; we record a certain set of errors, and also how long the fetch took.

Obviously still waiting on final URL!
(Assignee)

Updated

5 years ago
Attachment #8426762 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Attachment #8442436 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Depends on: 1029032
(Assignee)

Comment 21

5 years ago
I tested themes today. They weren't working before because the test distro had outdated theme data; the images didn't exist. Once I got over a PEBKAC (two in one morning!), they work.

It takes a while, though -- we download the distro, Gecko loads, downloads the images, hands them to Java, we display them. Gecko startup time is about 4-5 seconds, maybe a little more on first run, so we don't show a theme for the first 5 seconds you're looking at the browser. On a low-end device that can be as much as 10+ seconds. Not much we can do about that unless we decouple theming from Gecko.
(Assignee)

Comment 22

5 years ago
N.B., Bug 846184 tracks improving theme loading. We'd need to extend that to also be aware of distros.
(Assignee)

Comment 23

5 years ago
CDN fetch, https, wifi, Nexus 10: 783msec.
                  cell (AT&T), HTC One: 1046msec.
           http, cell, HTC One: 874msec.

So we have about a 200msec penalty for using a secure connection.

I think that's acceptable, particularly if it avoids us having to do client-side verification of payloads. Anyone else have an opinion?
Flags: needinfo?(mgoodwin)
(Assignee)

Comment 24

5 years ago
(Note that these numbers are spectacularly non-scientific: single runs. But they look sane.)
(In reply to Richard Newman [:rnewman] from comment #23)
> I think that's acceptable, particularly if it avoids us having to do
> client-side verification of payloads. Anyone else have an opinion?

I think that if we had to rely on a single mechanism I'd personally prefer robust package signing over relying solely on a secure connection.

But 'robust' is the key there... did you also get numbers on the JAR thing?
Flags: needinfo?(mgoodwin)
(Assignee)

Comment 26

5 years ago
(In reply to Mark Goodwin [:mgoodwin] from comment #25)

> But 'robust' is the key there... did you also get numbers on the JAR thing?

Not yet. Code needs to be written.
(In reply to Richard Newman [:rnewman] from comment #26)
> (In reply to Mark Goodwin [:mgoodwin] from comment #25)
> 
> > But 'robust' is the key there... did you also get numbers on the JAR thing?
> 
> Not yet. Code needs to be written.

Given that code needs to be written, I'd like to consider an HTTPS approach to start, and follow up with signing as we get it working.
(Assignee)

Comment 28

5 years ago
Posted patch Use jars instead of zips. v1 (obsolete) — Splinter Review
This is a trivial extension to use jars instead of zips, and handle security exceptions.

This does *not* mandate signed jars; it'll just error if the downloaded jar is trivially corrupted or modified. A smart attacker would remove all of the signing metadata when modifying the jar, and thus pass the checks.

However, this does pave the way for easily adding stronger verification down the line.
(Assignee)

Updated

5 years ago
Attachment #8442450 - Attachment is obsolete: true
(Assignee)

Comment 30

5 years ago
Comment on attachment 8447289 [details] [diff] [review]
Catch install intent and deliver it to the distribution handler, processing the distribution file dynamically. v5

Folded in the basic jar change. This is ready for first-pass review.

End-to-end test to come before landing.

FETCH_PATH will switch to /distro/ when we're done testing. I'm keeping namespaced for now, hence the TODO.
Attachment #8447289 - Attachment description: Catch install intent and deliver it to the distribution handler, processing the distribution file dynamically.* * * → Catch install intent and deliver it to the distribution handler, processing the distribution file dynamically. v5
Attachment #8447289 - Flags: review?(mark.finkle)
(Assignee)

Comment 31

5 years ago
Comment on attachment 8446834 [details] [diff] [review]
Use jars instead of zips. v1

Marking as obsolete, 'cos folded into v5 of the main patch.
Attachment #8446834 - Attachment is obsolete: true
Comment on attachment 8447289 [details] [diff] [review]
Catch install intent and deliver it to the distribution handler, processing the distribution file dynamically. v5

>diff --git a/mobile/android/base/distribution/Distribution.java b/mobile/android/base/distribution/Distribution.java

>+    private static final String FETCH_HOSTNAME = "distro-download.cdn.mozilla.net";

Is this set in stone yet. My OCD doesn't like the "distro" part. Sounds too much like Linux. "download" is kinda redundant. If this is a lot of work for server-ops to change then leave it. Otherwise, something simpler, like "distributions.cdn.mozilla.net" ?

>+    private static final String FETCH_PATH = "/test/";          // TODO

For the final paths, I think we should bake a little platform and versioning. Something like "android/v1". I don't think using App Version is possible since we'd need to keep updating folders on the CDN. 
Thoughts?

>+    private boolean checkIntentDistribution() {

>+        Log.d(LOGTAG, "Downloading referred distribution: " + uri);

Should we be logging the URL?

>+        recordFetchTelemetry(status);

Do you want recordFetchTelemetry(status) as a separate method? Looks like it's not used anywhere else and the code right below this does some recording of telemetry based on status too.

>+    private static void recordPostFetchTelemetry(final Exception exception) {

Not worth just wrapping this into recordFetchTelemetry? All the recordFetchTelemetry variants makes this code a little harder to follow.

--
copyFiles()/copyFiles(JAR) and recordFetchTelemetry(status)/recordFetchTelemetry(exception) make me realize that I am still in the "use explicit method names" camp. I was not always in that camp, but it's my personal style. Just a realization. Not worth a request to change, except for the considerations of collapsing recordFetchTelemetry and recordPostFetchTelemetry.
--

>diff --git a/mobile/android/base/distribution/ReferrerDescriptor.java b/mobile/android/base/distribution/ReferrerDescriptor.java

>+}
>\ No newline at end of file

Throw in a newline

>diff --git a/mobile/android/base/distribution/ReferrerReceiver.java b/mobile/android/base/distribution/ReferrerReceiver.java

>+    private static final String MOZILLA_UTM_SOURCE = "mozilla";
>+
>+
>+    @Override
>+    public void onReceive(Context context, Intent intent) {

Two line breaks might be one too many

>+        ReferrerDescriptor referrer = new ReferrerDescriptor(intent.getStringExtra("referrer"));
>+
>+        // Track the referrer object for distribution handling.
>+        Distribution.onReceivedReferrer(referrer);

I worry that Mozilla campaigns will have a utm_content, because there is no rule against it. I might have even seen Marketing making some campaign URLs with more parts than needed already. Maybe we should add a utm_source = "distribution" check here so the two worlds don't collide. Might as well do it now.

>diff --git a/mobile/android/tests/browser/junit3/moz.build b/mobile/android/tests/browser/junit3/moz.build

These are not on TBPL yet right?

r+, with some "for your consideration" thrown in.
Attachment #8447289 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 33

5 years ago
(In reply to Mark Finkle (:mfinkle) from comment #32)
> Otherwise, something simpler, like
> "distributions.cdn.mozilla.net" ?

I would guess that it's moderately calcified. If you would like it changed, please comment in Bug 1020033?


> For the final paths, I think we should bake a little platform and
> versioning. Something like "android/v1". I don't think using App Version is
> possible since we'd need to keep updating folders on the CDN. 
> Thoughts?

Was planning to, yup. Versioning is less of an issue here -- it should be vanishingly uncommon for a user to reach the Play store for a build that doesn't support the matching distribution -- but it's habit for me.


> >+        Log.d(LOGTAG, "Downloading referred distribution: " + uri);
> 
> Should we be logging the URL?

I'm in two minds about this. It's not sensitive information, it's useful for debugging, and the info should already be in the log at some level (after all, you just installed an app and got a system intent).


> copyFiles()/copyFiles(JAR) and
> recordFetchTelemetry(status)/recordFetchTelemetry(exception) make me realize
> that I am still in the "use explicit method names" camp.

These used to be even more similar than they already were -- one took a ZipFile, one took a ZipInputStream. The latter still does, but only the JarInputStream flavor. From that perspective, this is the perfect example of polymorphism -- we just don't really use it.

At this point I think it makes sense to split them in name, too.


> >+        // Track the referrer object for distribution handling.
> >+        Distribution.onReceivedReferrer(referrer);
> 
> I worry that Mozilla campaigns will have a utm_content, because there is no
> rule against it. I might have even seen Marketing making some campaign URLs
> with more parts than needed already. Maybe we should add a utm_source =
> "distribution" check here so the two worlds don't collide. Might as well do
> it now.

Fair. I'll have docs explaining which fields are 'protected', too.


> >diff --git a/mobile/android/tests/browser/junit3/moz.build b/mobile/android/tests/browser/junit3/moz.build
> 
> These are not on TBPL yet right?

Correct. (Alas.)
(Assignee)

Comment 34

5 years ago
> Fair. I'll have docs explaining which fields are 'protected', too.

Enough docs to start for this particular thing:

https://wiki.mozilla.org/Mobile/Distribution_Files#Crafting_a_distribution_referrer
(Assignee)

Updated

5 years ago
Attachment #8447289 - Attachment is obsolete: true
(Assignee)

Comment 36

5 years ago
Comment on attachment 8449974 [details] [diff] [review]
Part 1: catch install intent and deliver it to the distribution handler, processing the distribution file dynamically. v6

Carrying forward review.
Attachment #8449974 - Flags: review+
(Assignee)

Comment 37

5 years ago
Attachment #8449975 - Flags: review?(mark.finkle)
Attachment #8449975 - Flags: review?(mark.finkle) → review+
https://hg.mozilla.org/mozilla-central/rev/cbb06d641e7c
https://hg.mozilla.org/mozilla-central/rev/318d32b0069d
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Are partners testing this?
(Assignee)

Comment 42

5 years ago
They'll be exercising it, but not QAing it.
You need to log in before you can comment on or make changes to this bug.