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

RESOLVED FIXED in Firefox 44

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: mcomella)

Tracking

unspecified
Firefox 46
All
Android
Points:
---

Firefox Tracking Flags

(firefox44 fixed, firefox45 fixed, firefox46 fixed, b2g-v2.5 fixed, fennec44+)

Details

Attachments

(6 attachments)

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?
Flags: needinfo?(mark.finkle)
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.
Flags: needinfo?(mark.finkle)
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
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.
Flags: needinfo?(mark.finkle)
(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
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.
(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.
Flags: needinfo?(mark.finkle)
(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.
Created attachment 8701148 [details]
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/
Attachment #8701148 - Flags: review?(mark.finkle)
Created attachment 8701149 [details]
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/
Attachment #8701149 - Flags: review?(mark.finkle)
Created attachment 8701150 [details]
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/
Attachment #8701150 - Flags: review?(mark.finkle)
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
Attachment #8701148 - Flags: review?(mark.finkle) → review+
Attachment #8701149 - Flags: review?(mark.finkle) → review+
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 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
Attachment #8701150 - Flags: review?(mark.finkle) → review+
Dependent bug 1232773 is tracking 44.
tracking-fennec: --- → 44+
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
Attachment #8701148 - Flags: approval-mozilla-beta?
Attachment #8701148 - Flags: approval-mozilla-aurora?

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/98a0770068d9
https://hg.mozilla.org/mozilla-central/rev/ffe2cb1db992
https://hg.mozilla.org/mozilla-central/rev/b80edbdca7e1
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
status-firefox44: --- → affected
status-firefox45: --- → affected
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.
Attachment #8701148 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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.
Attachment #8701148 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8701149 [details]
MozReview Request: Bug 1233614 - Adjust.setEnabled before upload can occur. r=mfinkle

Beta44+
Attachment #8701149 - Flags: approval-mozilla-beta+
Comment on attachment 8701150 [details]
MozReview Request: Bug 1233614 - Remove MOZ_INSTALL_TRACKING branch around Adjust usage. r=mfinkle

Beta44+
Attachment #8701150 - Flags: approval-mozilla-beta+
We need a patch for 44 that works around the lack of Bug 1107811.
Flags: needinfo?(michael.l.comella)
Flags: needinfo?(michael.l.comella)
Summary: Investigate using Adjust.setEnabled in favor of checking the GeckoPreference each time → Use Adjust.setEnabled in favor of checking the GeckoPreference each time
Created attachment 8702339 [details] [diff] [review]
(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.
Created attachment 8702340 [details] [diff] [review]
(44 branch) Part 2 - Adjust.setEnabled before upload can occur

See the added comment for motivations.
Created attachment 8702341 [details] [diff] [review]
(44 branch) Part 3 - Remove MOZ_INSTALL_TRACKING branch around Adjust usage
NI for uplift.
Flags: needinfo?(wkocher)
Flags: needinfo?(wkocher)
You need to log in before you can comment on or make changes to this bug.