Add AwVariationsSeedFetchService and refactory VariationsSeedFetcher

android_webview/java/src/org/chromium/android_webview/AwVariationsSeedFetchService.java

+// Copyright 2017 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+package org.chromium.android_webview;
+
+import android.annotation.SuppressLint;
+import android.app.job.JobParameters;
+import android.app.job.JobService;
+import android.os.AsyncTask;
+
+import org.chromium.base.ContextUtils;
+import org.chromium.base.Log;
+import org.chromium.base.ThreadUtils;
+import org.chromium.components.variations.firstrun.VariationsSeedFetcher;

[gsennton, 2017/07/13 13:31:59]

What's the initialization process for VariationsSeedFetcher, does it require any
native code to be loaded?
Since this Service will be run in its own separate process we have to be very
careful to ensure we run all the initialization code we need to make sure
everything works in the service (and whether that code is run before our service
is started might depend on whether we are using the Standalone WebView package,
or Monochrome, as WebView implementation).

[Alexei Svitkine (slow), 2017/07/13 20:36:25]

VariationsSeedFetcher does not depend on any native code and this is by design.
(The way it's used in Chrome is specifically to fetch variations before native
has been initialized.)

[yiyuny, 2017/07/14 01:33:57]

Thanks for the explanation from Alexie. Paul and I search for the place which
probably contains the code to call the native code and find it safe to use it.

+import org.chromium.components.variations.firstrun.VariationsSeedFetcher.SeedInfo;
+
+import java.io.Closeable;
+import java.io.File;
+import java.io.FileNotFoundException;
+import java.io.FileOutputStream;
+import java.io.IOException;
+import java.io.ObjectOutputStream;
+import java.io.Serializable;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+/**
+ * AwVariationsSeedFetchService is a Job Service to fetch test seed data which is used by Finch
+ * to enable AB testing experiments in the native code. The fetched data is stored in the local
+ * directory which belongs to the Service process.
+ */
+@SuppressLint("NewApi") // JobService requires API level 21.

[gsennton, 2017/07/13 13:31:58]

WebView doesn't support pre-L anyway:
please replace the suppresslint statement above with
@TargetApi(Build.VERSION_CODES.LOLLIPOP)

[yiyuny, 2017/07/14 01:33:57]

Done.

+public class AwVariationsSeedFetchService extends JobService {
+    private static final String TAG = "AwVartnsSeedFetchSvc";
+
+    public static final String WEBVIEW_VARIATIONS_DIR = "WebView_Variations/";
+    public static final String SEED_DATA_FILENAME = "variations_seed_data";
+    public static final String SEED_PREF_FILENAME = "variations_seed_pref";
+
+    // Synchronization lock to prevent simultaneous local seed file writing.
+    private static final Lock sLock = new ReentrantLock();
+
+    @Override
+    public boolean onStartJob(JobParameters params) {
+        new FetchFinchSeedDataTask(params).execute();
+        return true;
+    }
+
+    @Override
+    public boolean onStopJob(JobParameters params) {
+        // When job parameter is not satisfied any more, the job should be rescheduled.

[gsennton, 2017/07/13 13:31:59]

I have no idea what this comment means :)

[yiyuny, 2017/07/14 01:33:57]

Oh, what I want to say is that the function will only be called when the job's
parameter is no longer satisfied before the jobFinished function is called. So
at this time, the function return true so that the job will be rescheduled.

[gsennton, 2017/07/14 12:45:39]

Right, I think the comment could be made more clear - "job parameters" is not
really the term we want here IMO (job parameters include lots of stuff, not just
the scheduling-restrictions of the job). E.g.
"This method is called by the JobScheduler to stop a job before it has finished.
Return true here to reschedule the job."
?

[yiyuny, 2017/07/15 00:18:59]

Done.

+        return true;
+    }
+
+    private class FetchFinchSeedDataTask extends AsyncTask<Void, Void, Void> {
+        private JobParameters mJobParams;
+
+        FetchFinchSeedDataTask(JobParameters params) {
+            mJobParams = params;
+        }
+
+        @Override
+        protected Void doInBackground(Void... params) {
+            AwVariationsSeedFetchService.fetchSeed();
+            return null;
+        }
+
+        @Override
+        protected void onPostExecute(Void success) {
+            jobFinished(mJobParams, false);

[gsennton, 2017/07/13 13:31:58]

Please add an inline comment describing what false means here (so that the next
person looking at this code doesn't need to double-check what that boolean
means)
e.g.
jobFinished(mJobParams, false /* needsReschedule */);
, or
jobFinished(mJobParams, false /* false -> don't reschedule */);

[yiyuny, 2017/07/14 01:33:57]

Done.

+        }
+    }
+
+    private static void fetchSeed() {
+        assert !ThreadUtils.runningOnUiThread();
+        // TryLock will drop calls from other threads when one threads are executing the function.
+        // TODO(yiyuny): Add explicitly control to to ensure there's only one threading fetching at

[gsennton, 2017/07/13 13:31:59]

Do you have a plan on how to solve this TODO?
Are you planning to add asserts, or tests as well?

[yiyuny, 2017/07/14 01:33:56]

Yes, I will the do the explicit control in the FinchConfigurationService. For
now, I have manually tested it.

+        // a time and that the seed doesn't get fetched too frequently
+        if (sLock.tryLock()) {
+            try {
+                SeedInfo seedInfo = VariationsSeedFetcher.get().downloadContent(

[gsennton, 2017/07/13 13:31:58]

What's the size of the data we are downloading here?
Do we care about the kind of network we are using for the download? (WiFi vs.
data plan).

[Alexei Svitkine (slow), 2017/07/13 20:36:25]

The chrome one is around 50k. It will be smaller for webview because only
specific experiments will target it.

[yiyuny, 2017/07/14 01:33:57]

I'm using NETWORK_TYPE_ANY(data plan/wifi both work) in the next CL. So if is
under 50k, I think that will be fine. Thanks to Alexie for providing the data.

[gsennton, 2017/07/14 12:45:39]

What I mean is, we should discuss whether it is OK to use users' data plans for
this - WebView is used by 800+ million people, so if all of those users use 50k
of data over a data plan that means 50kB * 8e8 = 4e10kB = 40TB. So we would be
using 40TB of data plan data (please correct me if my calculations are off).
That's quite a lot of data to pay for :P

[paulmiller, 2017/07/14 17:41:05]

System health will be an important consideration for when we enable this for
real, but for this summer project we just want a prototype, which will not be
enabled in prod. (I also don't think it makes sense to multiply by number of
users, because each user pays individually. The addition of more WebView users
doesn't increase data usage for other users.)

[gsennton, 2017/07/17 14:32:10]

Ah alright, I did not realize that this is a prototype. Do we intent to commit
this code, or is this review solely here for us to look at / download?

If we're committing it, could we make it clear somewhere that this is a
prototype? (e.g. through some comment at the top of this file).

+                        VariationsSeedFetcher.VariationsPlatform.ANDROID_WEBVIEW, "");
+                storeVariationsSeed(seedInfo);
+            } catch (IOException e) {
+                // Exceptions are handled in the downloadContent function and rethrowing the

[gsennton, 2017/07/13 13:31:59]

Maybe note that exceptions are also logged within downloadContent(). If they
weren't we would want logging here.

so

"Exceptions are handled in the downloadContent"
->
"Exceptions are handled and logged in the downloadContent"

[yiyuny, 2017/07/14 01:33:57]

Done.

+                // exception is to stop the normal logic flow after it, so no error-handling here.
+                // Not explicitly handing SocketTimeoutException and UnknownHostException for they
+                // are both subclasses of IOException.
+            } finally {
+                sLock.unlock();
+            }
+        }
+    }
+
+    /**
+     * SeedPreference is used to serialize/deserialize related fields of seed data when reading or
+     * writing them to the internal storage.
+     */
+    public static class SeedPreference implements Serializable {
+        /**
+         * Let the program deserialize the data when the fields are changed.
+         */
+        private static final long serialVersionUID = 0L;
+
+        public final String signature;
+        public final String country;
+        public final String date;
+        public final boolean isGzipCompressed;
+
+        public SeedPreference(SeedInfo seedInfo) {
+            signature = seedInfo.signature;
+            country = seedInfo.country;
+            date = seedInfo.date;
+            isGzipCompressed = seedInfo.isGzipCompressed;
+        }
+    }
+
+    private static File getOrCreateWebViewVariationsDir() {
+        File webViewFileDir = ContextUtils.getApplicationContext().getFilesDir();

[gsennton, 2017/07/13 13:31:59]

Are we guaranteed that ContextUtils is initialized at this point? (AFAICT this
code is running in a process separate to the normal WebView-loading code).

[yiyuny, 2017/07/14 01:33:57]

I added a initApplicationContext in onStartJob. The code will crash in System
WebView apk without the initialization. Thanks to Gustav and Paul for helping me
find this bug.

+        File dir = new File(webViewFileDir, WEBVIEW_VARIATIONS_DIR);
+        if (dir.mkdir() || dir.isDirectory()) {
+            return dir;
+        }
+        return null;
+    }
+
+    private static void storeVariationsSeed(SeedInfo seedInfo) {
+        File webViewVariationsDir = getOrCreateWebViewVariationsDir();
+        if (webViewVariationsDir == null) {

[gsennton, 2017/07/13 13:31:59]

Please add a log-statement before the return-statement here - I assume we should
almost never fail to create webviewVariationsDir? (so we should log an
error/warning here).

[yiyuny, 2017/07/14 01:33:57]

Done.

+            return;
+        }
+
+        FileOutputStream fosSeed = null;
+        ObjectOutputStream oosSeedPref = null;
+        try {
+            fosSeed = new FileOutputStream(new File(webViewVariationsDir, SEED_DATA_FILENAME));

[gsennton, 2017/07/13 13:31:58]

One very common pattern when performing file writes (especially when the data
written is large) is to first write the data to a temporary file, and then
rename that file to the final file-name. If you don't follow that pattern you
might end up crashing in the middle of the data-write (and thus end up with a
file with broken content).

How will these two files be read?
If we are overwriting these files every time we download a new seed we should
definitely make sure we aren't overwriting them partially / messing the files up
:) 

[yiyuny, 2017/07/14 01:33:57]

There should be read write lock on the seed pref file. I will do it in the next
CL.

+            fosSeed.write(seedInfo.seedData, 0, seedInfo.seedData.length);
+            // Store separately so that reading large seed data (which is expensive) can be

[gsennton, 2017/07/13 13:31:59]

Just to clarify (to make sure I understand): we are storing the fetch time in a
separate time to avoid having to open the large seed-data file just to read the
fetch-time?

[yiyuny, 2017/07/14 01:33:56]

Yes

+            // prevented when checking the last seed fetch time.
+            oosSeedPref = new ObjectOutputStream(
+                    new FileOutputStream(new File(webViewVariationsDir, SEED_PREF_FILENAME)));
+            oosSeedPref.writeObject(new SeedPreference(seedInfo));
+        } catch (FileNotFoundException e) {
+            Log.e(TAG,
+                    "FileNotFoundException failed to open file to write seed data or preference.");

[gsennton, 2017/07/13 13:31:59]

Include the actual exception in the logs please (this helps a lot when something
goes wrong :))
Log.e(TAG, "FileNotFoundException failed to open file to write seed data or
preference.", e);

And for the IOException as well.
Actually, given that the FileNotFoundException is a type of IOException, I would
just merge these two catch-statements since we are just printing a generic
message anyway.

[yiyuny, 2017/07/14 01:33:56]

I agree with combining two exceptions together.

Paul recommends not passing e, because a stack trace would be unnecessarily
verbose. It wouldn't be very informative, because there's only 1 code path to
this function, and logcat is already pretty noisy with stack traces.

[gsennton, 2017/07/14 12:45:39]

Well, if you're not gonna print the exception then you should probably split
this catch-statement in two - so we at least know what kind of exception is
thrown.

I still think we should print the actual exception to aid debugging - if you
don't print the exception you still won't know exactly what went wrong. But
whether or not it's worth it depends on how often we expect to receive an
exception here - if it is rare enough it should be fine to print the full
exception stack trace.

[paulmiller, 2017/07/14 23:06:24]

I think these error messages strings are getting nit-picked back and forth a bit
too much; too many cooks, I suppose :) Gustav, if you have a particular message
style you'd like to prescribe, we can just go with that. Otherwise let's leave
them as-is.

[yiyuny, 2017/07/15 00:18:59]

I will add 'e' to the end of the string to log what kind of exception is thrown.

+        } catch (IOException e) {
+            Log.e(TAG,
+                    "IOException failed to write variations seed data or preference to the file.");
+        } finally {
+            closeStream(fosSeed, SEED_DATA_FILENAME);
+            closeStream(oosSeedPref, SEED_PREF_FILENAME);
+        }
+    }
+
+    private static void closeStream(Closeable stream, String filename) {
+        if (stream != null) {
+            try {
+                stream.close();
+            } catch (IOException e) {
+                Log.e(TAG, "IOException failed to close " + filename + " file.");
+            }
+        }
+    }
+}