1233614 - Use Adjust.setEnabled in favor of checking the GeckoPreference each time

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It's hard to remember to check the GeckoPreference each time an Adjust method is called – it looks like Adjust has built-in functionality to solve this problem for us.

Additionally, if we don't use this pref, there can potentially be some inconsistency issues (e.g. we don't call Adjust.onCreate because we check the pref, the pref changes, so we call Adjust.onResume without calling Adjust.onCreate – what does the Adjust SDK do?).

Finkle, is there any reason we check the prefs rather than using this setting? Do we need to vet it turns everything off before we use it?

Comment 1

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

This seems like a reasonable change. Yes, we should do a quick audit the SDK code and logcat to make sure Adjust is really not sending anything, but this approach seems good to me.

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Looking into the master branch (34397b6bcd0c68ba176c5904a2937eab78cb68e4)...

Adjust.setEnabled calls into:
 AdjustInstance.setEnabled (note: needs activityHandler to be initialized, which means onCreate must be called and we can't add our own flag there)
  ActivityHandler.setEnabled [1] (which sets `activityState.enabled = <new value>` and calls `writeActivityState`)
   ActivityHandler.updateStatus [2]

The latter of which will either start or stop the "subsession" based on the boolean flag passed in. Since we're following the disabling code path, that is Activity.trackSubsessionEnd, which sends a SessionHandler.END message. This is read in:
 ActivityHandler.handleMessage
  ActivityHandler.endInternal [3] (this is called via SessionHandler's `sessionHandlerReference` WeakReference which is stored in the current ActivityHandler as `sessionHandler`)

This performs several notable actions. First, it calls PackageHandler.pauseSending which sets the `PackageHandler.paused = true`. This short-circuits PackageHandler.sendFirstInternal which, through RequestHandler.sendMessage (w/ `IntenalHandler.SEND message type), calls RequestHandler.sendInternal, which makes a network connection, which I have not audited. Upon skimming, I don't see any other network calls from this class.

`paused = true` only occurs in `PackageHandler.resumeSending`, which is only called from ActivityHandler.updatePackageHandlerStatus when ActivityHandler.paused is false (this calls upon ActivityHandler.isEnabled, which calls out to ActivityState.enabled. In the edge case, ActivityHandler.enabled is returned which I have not audited but, if we distrust the software, probably should).

ActivityHandler.endInternal then calls AttributionHandler.pauseSending which also sets `paused = true`. Similarly, `resumeSending` is the only user to call `paused = false`, and is similar called in ActivityHandler.updateAttributionHandlerStatus, which also is only called when `ActivityHandler.paused()` returns false (which I should audit). `paused` is read & short-circuits in AttributionHandler.getAttributionInternal before creating a network connection. I see no other network connections in this file.

As a bonus, I checked the call hierarchy of some of the methods in their common connection pattern, Util.readHttpResponse [4], and it is only called from AttributionHandler.getAttributionInternal and RequestHandler.sendInternal, which I audited.

Finally, ActivityHandler.endInternal calls:
 ActivityHandler.stopTimer
  TimerCycle.suspend

which cancels the waiting task created when the Timer was started. The one use of this TimerCycle is in ActivityHandler.initInternal [5], where the TimerCycle will call ActivityHandler.timerFired when it ends. This sends a message (SessionHandler.TIMER_FIRED) read in SessionHandler.handleMessage, which calls SessionsHandler.timerFiredInternal which initiates uploads via PackageHandler.sendFirstPackage [6]. This also short-circuits on `paused` for good measure.

So far, it seems that isEnabled does what we want. Remaining questions:
  * Does isEnabled prevent measurements from occurring? e.g. if we set isEnabled(false) and call onPause, will the session measurements be stored and uploaded when we call isEnabled(true)? What is the desired outcome here?
  * When do we need to call isEnabled?
    - Does onCreate do any uploading? Will we upload between an onCreate call and an isEnabled call?
    - Is stored state read and restored? If so, when? Will we upload before this state is restored?
  * Audit ActivityHandler.enabled for its use in `paused` (see above)
  * Audit for uses of reflection to access APIs (these would not appear in IDE call hierarchies and thus could be called without us knowing)
  * Audit for additional network calls

[1]: https://github.com/adjust/android_sdk/blob/34397b6bcd0c68ba176c5904a2937eab78cb68e4/Adjust/adjust/src/main/java/com/adjust/sdk/ActivityHandler.java#L151
[2]: https://github.com/adjust/android_sdk/blob/34397b6bcd0c68ba176c5904a2937eab78cb68e4/Adjust/adjust/src/main/java/com/adjust/sdk/ActivityHandler.java#L172
[3]: https://github.com/adjust/android_sdk/blob/34397b6bcd0c68ba176c5904a2937eab78cb68e4/Adjust/adjust/src/main/java/com/adjust/sdk/ActivityHandler.java#L525
[4]: https://github.com/adjust/android_sdk/blob/34397b6bcd0c68ba176c5904a2937eab78cb68e4/Adjust/adjust/src/main/java/com/adjust/sdk/AttributionHandler.java#L137
[5]: https://github.com/adjust/android_sdk/blob/34397b6bcd0c68ba176c5904a2937eab78cb68e4/Adjust/adjust/src/main/java/com/adjust/sdk/ActivityHandler.java#L441
[6]: https://github.com/adjust/android_sdk/blob/34397b6bcd0c68ba176c5904a2937eab78cb68e4/Adjust/adjust/src/main/java/com/adjust/sdk/ActivityHandler.java#L776

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

To further analyze the uploads, PackageHandler maintains a member, `packageQueue` to which items get added before they are uploaded via `sendFirstInternal`. Items can be added to the queue whether Adjust is enabled or not though upload will not occur if Adjust is disabled. The queue is saved to disk and can be uploaded later. (If we need to make a work-around to prevent data storage, the queue is inited from a file on disk which we can manually delete with `PackageHandler.deletePackageQueue`).

PackageHandler.addPackage is called from a few places:
  ActivityHandler.trackEventInternal (short-circuit w/ isEnabled)
  ActivityHandler.transferSessionPackage (short-circuit w/ activityState.enabled)
  ActivityHandler.readOpenUrlInternal (no short-circuit)
  ActivityHandler.sendReferrerInternal (no short-circuit)

Finkle, the calls that do not short-circuit will upload their data, afaict, if Adjust is ever re-enabled – do you think I should audit these calls to see if they are storing data we don't want to upload in the future?

The other entry point to network I saw, AttributionHandler.getAttributionInternal, short-circuits within the method on paused. However, this is a GET request, so we shouldn't have to worry about it.

(In reply to Michael Comella (:mcomella) from comment #2)
>   * Audit for additional network calls

To check for additional network calls, I opened the other class files, looked at the imports, and if anything looked network related, I checked it out – I did not find any other network calls.

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #2)
> So far, it seems that isEnabled does what we want. Remaining questions:
>   * Does isEnabled prevent measurements from occurring? e.g. if we set
> isEnabled(false) and call onPause, will the session measurements be stored
> and uploaded when we call isEnabled(true)? What is the desired outcome here?

Some measurements yes, some no – see comment 3 and the details on addPackage. Finkle should answer what the desired outcome is.

>     - Does onCreate do any uploading?

onCreate calls sendReferrerInternal which adds the referrer to the outgoing packages but does not actually upload.

>     - Is stored state read and restored? If so, when? Will we upload before
> this state is restored?

Yes, state is restored from `ActivityHandler.readActivityState` [1]. Since this occurs before the timer that initiates upload is constructed, we read enabled state before uploading, preventing upload.

>  Will we upload between an onCreate call and an isEnabled call?

The timer is started only after a call to ActivityHandler.trackSubsessionStart, which occurs first in onResume unless setEnabled is called first. Therefore, we can call onCreate, setEnabled(false), and onResume without uploading.

>   * When do we need to call isEnabled?

I can't remember if it's opt-in or opt-out so...
 Opt-in: Call setEnabled(false) immediately after onCreate. We can grab the preference value asynchronously and update this ASAP – note that some tracked events could be missed in the meantime, which may not be desirable – I think we're only tracking session data so we should verify that's there.
 Opt-out: Just call setEnabled from the preference change listener. (There is a possibility that this stored data is deleted by Adjust but I'm going to assume not)

>   * Audit for uses of reflection to access APIs (these would not appear in
> IDE call hierarchies and thus could be called without us knowing)

The only reflection I see is the Reflection class, which does not appear to access the network.

[1]: https://github.com/adjust/android_sdk/blob/34397b6bcd0c68ba176c5904a2937eab78cb68e4/Adjust/adjust/src/main/java/com/adjust/sdk/ActivityHandler.java#L428

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Pending Finkle feedback, this lgtm.

Finkle, is this data opt-in or opt-out? I remember FHR being opt-in but I'd think the Adjust install data is opt-out and maybe the session data is opt-in, but I don't think we have that granularity.

Comment 6

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

(In reply to Michael Comella (:mcomella) from comment #5)
> Pending Finkle feedback, this lgtm.
> 
> Finkle, is this data opt-in or opt-out? I remember FHR being opt-in but I'd
> think the Adjust install data is opt-out and maybe the session data is
> opt-in, but I don't think we have that granularity.

FHR data is opt-out. The Adjust data should be opt-out too.

Comment 7

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

(In reply to Michael Comella (:mcomella) from comment #3)
> To further analyze the uploads, PackageHandler maintains a member,
> `packageQueue` to which items get added before they are uploaded via
> `sendFirstInternal`. Items can be added to the queue whether Adjust is
> enabled or not though upload will not occur if Adjust is disabled. The queue
> is saved to disk and can be uploaded later. (If we need to make a
> work-around to prevent data storage, the queue is inited from a file on disk
> which we can manually delete with `PackageHandler.deletePackageQueue`).
> 
> PackageHandler.addPackage is called from a few places:
>   ActivityHandler.trackEventInternal (short-circuit w/ isEnabled)
>   ActivityHandler.transferSessionPackage (short-circuit w/
> activityState.enabled)
>   ActivityHandler.readOpenUrlInternal (no short-circuit)
>   ActivityHandler.sendReferrerInternal (no short-circuit)
> 
> Finkle, the calls that do not short-circuit will upload their data, afaict,
> if Adjust is ever re-enabled – do you think I should audit these calls to
> see if they are storing data we don't want to upload in the future?

I think this is OK. Storing data while not enabled might even be what FHR does currently.

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1233614 - Remove health report flags in favor of Adjust.setEnabled. r=mfinkle

Review commit: https://reviewboard.mozilla.org/r/28909/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28909/

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1233614 - Adjust.setEnabled before upload can occur. r=mfinkle

Review commit: https://reviewboard.mozilla.org/r/28911/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28911/

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1233614 - Remove MOZ_INSTALL_TRACKING branch around Adjust usage. r=mfinkle

Review commit: https://reviewboard.mozilla.org/r/28913/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/28913/

Comment 11

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

Comment on attachment 8701148 [details]
MozReview Request: Bug 1233614 - Remove health report flags in favor of Adjust.setEnabled. r=mfinkle

https://reviewboard.mozilla.org/r/28909/#review25673

Comment 12

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

Comment on attachment 8701149 [details]
MozReview Request: Bug 1233614 - Adjust.setEnabled before upload can occur. r=mfinkle

https://reviewboard.mozilla.org/r/28911/#review25675

Comment 13

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

Comment on attachment 8701150 [details]
MozReview Request: Bug 1233614 - Remove MOZ_INSTALL_TRACKING branch around Adjust usage. r=mfinkle

https://reviewboard.mozilla.org/r/28913/#review25677

Comment 14

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/integration/fx-team/rev/98a0770068d98a77fac609a1c4a5e8a309cb50a5
Bug 1233614 - Remove health report flags in favor of Adjust.setEnabled. r=mfinkle

https://hg.mozilla.org/integration/fx-team/rev/ffe2cb1db99201e0f4fcc67d0141202842543750
Bug 1233614 - Adjust.setEnabled before upload can occur. r=mfinkle

https://hg.mozilla.org/integration/fx-team/rev/b80edbdca7e1a2ea09f7db53b11385f1331fd7d2
Bug 1233614 - Remove MOZ_INSTALL_TRACKING branch around Adjust usage. r=mfinkle

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Dependent bug 1232773 is tracking 44.

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8701148 [details]
MozReview Request: Bug 1233614 - Remove health report flags in favor of Adjust.setEnabled. r=mfinkle

This request applies to the three patches that landed in comment 10.

Approval Request Comment
[Feature/regressing bug #]: dependent bug 1232773 is tracking 44 (beta).
[User impact if declined]: When using more adjust features than the install ping (onCreate), we need to use Adjust.setEnabled to ensure the Adjust SDK's assumptions are met. This patch does that and bug 1232773 will add more Adjust features.

[Describe test coverage new/current, TreeHerder]: Tested locally.
[Risks and why]: We're relying on 3rd party software to ensure we're not uploading user data. I audited the code but it's possible I missed something. We could in theory mess up the calls to prevent this (Adjust.setEnabled), but they're fairly simple and I doubt it. Additionally, I removed some redundant compile-time flags for accessing the Adjust APIs. The code path for the remaining flag is not tested (except locally though it should run on Nightly/Aurora builds when it lands) so it's possible it's broken, but again, it's simple so I think we're fine.

[String/UUID change made/needed]: None

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/98a0770068d9
https://hg.mozilla.org/mozilla-central/rev/ffe2cb1db992
https://hg.mozilla.org/mozilla-central/rev/b80edbdca7e1

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8701148 [details]
MozReview Request: Bug 1233614 - Remove health report flags in favor of Adjust.setEnabled. r=mfinkle

Let's take that in aurora to test before beta.

Comment 19

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/c49a97660618
https://hg.mozilla.org/releases/mozilla-aurora/rev/31d2c8fc149c
https://hg.mozilla.org/releases/mozilla-aurora/rev/6004294f6843

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8701148 [details]
MozReview Request: Bug 1233614 - Remove health report flags in favor of Adjust.setEnabled. r=mfinkle

Patches have been on m-c for a few days, seems safe to uplift to Beta44.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8701149 [details]
MozReview Request: Bug 1233614 - Adjust.setEnabled before upload can occur. r=mfinkle

Beta44+

Comment 22

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8701150 [details]
MozReview Request: Bug 1233614 - Remove MOZ_INSTALL_TRACKING branch around Adjust usage. r=mfinkle

Beta44+

Comment 23

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

We need a patch for 44 that works around the lack of Bug 1107811.

Comment 24

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (44 branch) Part 1 - Remove health report flags in favor of Adjust.setEnabled

I ran `gsed -i".bak" "s.java/org/mozilla/gecko/..g" <patch-file>` to do these
rebases, with a little manually intervention for .rej files.

Comment 25

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (44 branch) Part 2 - Adjust.setEnabled before upload can occur

See the added comment for motivations.

Comment 26

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: (44 branch) Part 3 - Remove MOZ_INSTALL_TRACKING branch around Adjust usage

Comment 27

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI for uplift.

Comment 28

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/releases/mozilla-beta/rev/80a4b18ecf02
https://hg.mozilla.org/releases/mozilla-beta/rev/920acabe41ac
https://hg.mozilla.org/releases/mozilla-beta/rev/93c9caf91815

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/80a4b18ecf02
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/920acabe41ac
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/93c9caf91815