1334562 - DLC: Create a means to prevent downloads during testing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Bob Clary [:bc] (inactive)]

http://phonedash.mozilla.org/#/2017-01-27/2017-01-31/binning=repo-phonetype-phoneid&rejected=norejected&errorbars=errorbars&errorbartype=standarderror&valuetype=median&remote-blank=on&remote-nytimes=on&remote-twitter=on&throbberstart=on&throbberstop=on&throbbertime=on&first=on&second=on&autoland=on&mozilla-aurora=on&mozilla-beta=on&mozilla-central=on&mozilla-inbound=on&mozilla-release=on&nexus-6p-12=on&nexus-6p-3=on&nexus-6p-4=on

Shows several spikes for the Nexus 6P devices. Looking at the logcats for these runs, I see 

01-27 10:43:03.930 D/DLCDownloadAction( 6279): Downloading: [asset-archive,font] a173d1db-373b-ce42-1335-6b3285cfdebd (156124 bytes) 7bce66864e38eecd7c94b6657b9b38c35ebfacf8046bfb1247e08f07fe933198
01-27 10:43:03.934 D/DLCDownloadAction( 6279): Content already exists and is up-to-date.
...

While it is not completely certain that network accesses are the cause of the spikes, we should have a way of preventing this.

We normally set MOZ_DISABLE_NONLOCAL_CONNECTIONS and to force a crash if we attempt to access a Non-local network.

1. We should crash if MOZ_DISABLE_NONLOCAL_CONNECTIONS and a Non-local network access is made.

2. We should have a means of turning off downloading stuff like these fonts.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

I'll take this to fix at least (2): Disabling the download content service during test. Is MOZ_DISABLE_NONLOCAL_CONNECTIONS set as an environment variable?

Comment 2

[Bob Clary [:bc] (inactive)]

The env variable was set but it did not crash which it *should* have done.

In addition, there should be a pref to turn off these downloads once we do crash when the env variable is set.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Bob Clary [:bc:] from comment #2)
> The env variable was set but it did not crash which it *should* have done.

Yeah, Java code is mostly exempt from that.

For switchboard and the telemetry core ping we introduced MOZ_IN_AUTOMATION:
* https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/BrowserApp.java#618-619
* https://dxr.mozilla.org/mozilla-central/source/mobile/android/geckoview/src/main/java/org/mozilla/gecko/util/IntentUtils.java#94-108

I could use the same for DLC. Although I wonder why we do not use MOZ_DISABLE_NONLOCAL_CONNECTIONS.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

What's tricky here is that we already start the DLC service before we create the activity and read any environment variables from the Intent:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/GeckoApplication.java#194-196

Comment 5

[Bob Clary [:bc] (inactive)]

Looks like the unit tests already set MOZ_IN_AUTOMATION: http://searchfox.org/mozilla-central/search?q=MOZ_IN_AUTOMATION&path=

I can add that to Autophone's non unit tests such as the performance tests. Filed bug 1335017.

If this has the same purpose as MOZ_DISABLE_NONLOCAL_CONNECTIONS it would be nice to just use that instead of yet another env variable.

Comment 6

[Geoff Brown [:gbrown]]

As I recall, MOZ_DISABLE_NONLOCAL_CONNECTIONS causes a crash when a non-local connection is attempted in Gecko. MOZ_IN_AUTOMATION is a Firefox-for-Android-only environment variable used to turn off features or modify behavior (primarily hitting the network) not suitable for the automation environment. Two distinctions there: 1. Cause a crash on non-local connection vs disable possible broader features; 2. Gecko vs Fennec scope.

(On desktop, prefs are generally used to avoid non-local connections in tests; in Fennec, prefs are sometimes not practical, as when non-local connections would be initiated in Java before Gecko is available.)

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1334562 - DLC: Start study action from BrowserApp.onCreate() and never in automation.

Previously we started the DLC service from GeckoApplication. The reason for that was that we wanted to
download content like fonts as early as possible so that they are available the next time we display
a website.

However we do not know whether we are running in automation until the BrowserApp activity is launched.
This patch will now start the service from the "delayed startup" callback because:

* It's running a second or two after GeckoApplication.onCreate(). So not much time is lost.
* When downloading a font we refresh Gecko's font list - so new fonts are available instantly without
  restart anyways.

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

Comment 8

[Andrzej Hunt :ahunt]

Comment on attachment 8831727 [details]
Bug 1334562 - DLC: Start study action from BrowserApp.onCreate() and never in automation.

https://reviewboard.mozilla.org/r/108280/#review109290

Comment 9

[Richard Newman [:rnewman]]

Comment on attachment 8831727 [details]
Bug 1334562 - DLC: Start study action from BrowserApp.onCreate() and never in automation.

https://reviewboard.mozilla.org/r/108280/#review109292

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java:1764
(Diff revision 1)
>                  }
>  
> -                if (AppConstants.MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE) {
> -                    // TODO: Better scheduling of sync action (Bug 1257492)
> +                if (AppConstants.MOZ_ANDROID_DOWNLOAD_CONTENT_SERVICE &&
> +                        !IntentUtils.getIsInAutomationFromEnvironment(new SafeIntent(getIntent()))) {
> +                    // TODO: Better scheduling of DLC actions (Bug 1257492)
> +                    DownloadContentService.startStudy(this);

`DelayedStartup` is sent -- IIRC -- in response to `DOMContentLoaded`.

What you're doing here is saying that no download work will begin until Gecko has loaded (2-3 seconds), aboutHome.xhtml or the external URL has been loaded and parsed, and stumbler has been kicked off.

`DelayedStartup` was intended to speed up Fennec startup by pushing non-critical, non-time-sensitive stuff to "later" -- after the first page has loaded.

I'm not convinced that DLC fits that category. Eventually this might include all kinds of content, including Android locale strings, that will affect the use of the browser and the display of pages. At some point we might even require that the DLC process completes before leaving the first-run experience… that's very incompatible with `DelayedStartup`.

Comment 10

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Richard Newman [:rnewman] from comment #9)
> `DelayedStartup` is sent -- IIRC -- in response to `DOMContentLoaded`.

Correct!


(In reply to Richard Newman [:rnewman] from comment #9)
> What you're doing here is saying that no download work will begin until
> Gecko has loaded (2-3 seconds), aboutHome.xhtml or the external URL has been
> loaded and parsed, and stumbler has been kicked off.

I was considering kicking it off in BrowserApp.onCreate() [instead of GeckoApplication], but ...

(In reply to Richard Newman [:rnewman] from comment #9)
> I'm not convinced that DLC fits that category. Eventually this might include
> all kinds of content, including Android locale strings, that will affect the
> use of the browser and the display of pages.

... no matter what we do there's no guarantee that anything will be available at this time. So is it worth having the 2 seconds advantage now?


(In reply to Richard Newman [:rnewman] from comment #9)
> At some point we might even require that the DLC process completes before
> leaving the first-run experience… that's very incompatible with `DelayedStartup`.

Okay, this will require some additional changes. But it sounds like BrowserApp.onCreate() would be early enough for that too.

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8831727 [details]
Bug 1334562 - DLC: Start study action from BrowserApp.onCreate() and never in automation.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/108280/diff/1-2/

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Pushed a new patch that starts the service from BrowserApp.onCreate(). It will be called shortly after GeckoApplication.onCreate() and we do not need to wait for Gecko

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/01aad8181f5d
DLC: Start study action from BrowserApp.onCreate() and never in automation. r=ahunt

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/01aad8181f5d