WebView: Refactor PlatformServiceBridge.getInstance

android_webview/glue/java/src/com/android/webview/chromium/WebViewChromiumFactoryProvider.java

 // Copyright 2014 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 com.android.webview.chromium;
 
 import android.Manifest;
 import android.app.ActivityManager;
 import android.content.ComponentCallbacks2;
 import android.content.Context;
@@ skipping 401 matching lines @@
         final boolean isExternalService = true;
         AwBrowserProcess.configureChildProcessLauncher(webViewPackageName, isExternalService);
         AwBrowserProcess.start();
 
         final boolean enableMinidumpUploadingForTesting = CommandLine.getInstance().hasSwitch(
                 CommandLineUtil.CRASH_UPLOADS_ENABLED_FOR_TESTING_SWITCH);
         if (enableMinidumpUploadingForTesting) {
             AwBrowserProcess.handleMinidumps(webViewPackageName, true /* enabled */);
         }
 
         // Actions conditioned on whether the Android Checkbox is toggled on

[timvolodine, 2017/04/03 12:18:52]

perhaps also provide a comment that the "createInstance" call creates a
singleton instance used elsewhere and that it's crucial to do this here at
startup i.e. the location of this code is important.

[paulmiller, 2017/04/03 23:34:16]

Done.

-        PlatformServiceBridge.getInstance(context)
-                .queryMetricsSetting(new ValueCallback<Boolean>() {
+        PlatformServiceBridge.createInstance(context).queryMetricsSetting(

[Tobias Sargeant, 2017/04/03 10:40:06]

Keep this getInstance() and add a createInstance() earlier in the method? I had
to check that handleMinidumps didn't rely on the instance having been created,
and I'm not certain that there aren't other methods that start operations on
other threads above.

[paulmiller, 2017/04/03 23:34:16]

IMHO, if we preemptively create our singletons as early as possible, that
removes the create-on-first-use benefit of singletons. The only case we need to
worry about is calling "getInstance(null)" (because that's the version that
can't create if it's the first use) and there's only 1 of those:

https://cs.chromium.org/search/?q=PlatformServiceBridge%5C.getInstance%5C(nul...

[Tobias Sargeant, 2017/04/04 12:30:47]

Is create-on-first-use really that much of a benefit, though? Especially if
there are cases where we can't create? It becomes kind of hard to reason about
whether those cases execute before or after creation of the singleton instance.

In the example here it would be easy for the AsyncTask that handleMinidumps()
creates to be changed to call getInstance (because it doesn't have a context to
call getOrCreateInstance with) and have that be unintentionally racy. If we can
ensure that that can't happen now by creating the instance earlier, shouldn't
we?

+                new ValueCallback<Boolean>() {
                     public void onReceiveValue(Boolean enabled) {
                         ThreadUtils.assertOnUiThread();
                         AwMetricsServiceClient.setConsentSetting(context, enabled);
 
                         if (!enableMinidumpUploadingForTesting) {
                             AwBrowserProcess.handleMinidumps(webViewPackageName, enabled);
                         }
                     }
                 });
 
@@ skipping 256 matching lines @@
     WebViewDelegate getWebViewDelegate() {
         return mWebViewDelegate;
     }
 
     // The method to support unreleased Android.
     WebViewContentsClientAdapter createWebViewContentsClientAdapter(WebView webView,
             Context context) {
         return new WebViewContentsClientAdapter(webView, context, mWebViewDelegate);
     }
 }