Prevent thread assertion for HTC mail apk

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

Index: android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
diff --git a/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java b/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
index 0f8ca85b46b4b6dc8b195baf1c92a20f3a56962e..dbecc67a38e309e89b9c4429fef1c888f0eb46f8 100644
--- a/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
+++ b/android_webview/glue/java/src/com/android/webview/chromium/WebViewChromium.java
@@ -8,6 +8,7 @@ import android.annotation.SuppressLint;
 import android.annotation.TargetApi;
 import android.content.Context;
 import android.content.Intent;
+import android.content.pm.PackageInfo;
 import android.content.res.Configuration;
 import android.graphics.Bitmap;
 import android.graphics.Canvas;
@@ -62,9 +63,11 @@ import org.chromium.content_public.browser.NavigationHistory;
 
 import java.io.BufferedWriter;
 import java.io.File;
+import java.lang.reflect.Field;
 import java.lang.reflect.Method;
 import java.util.Map;
 import java.util.concurrent.Callable;
+import java.util.concurrent.atomic.AtomicBoolean;
 
 /**
  * This class is the delegate to which WebViewProxy forwards all API calls.
@@ -102,6 +105,8 @@ class WebViewChromium implements WebViewProvider, WebViewProvider.ScrollDelegate
 
     private WebViewChromiumFactoryProvider mFactory;
 
+    private static volatile AtomicBoolean sIsHtcMail = new AtomicBoolean(false);

[Torne, 2016/08/12 09:24:23]

For future reference, don't use volatile for this - make it final, but per my
comments later there's no need for atomics (or a boolean) at all. :)

[Changwan Ryu, 2016/08/12 11:14:06]

Acknowledged.

[Torne, 2016/08/12 13:25:41]

Your new CL still has this as volatile. If we actually need this and aren't
going to remove it entirely, it should be final.

[Changwan Ryu, 2016/08/16 10:30:24]

My bad. Fixed as final.

+
     private static boolean sRecordWholeDocumentEnabledByApi = false;
     static void enableSlowWholeDocumentDraw() {
         sRecordWholeDocumentEnabledByApi = true;
@@ -135,6 +140,8 @@ class WebViewChromium implements WebViewProvider, WebViewProvider.ScrollDelegate
     @TargetApi(Build.VERSION_CODES.LOLLIPOP)
     public void init(final Map<String, Object> javaScriptInterfaces,
             final boolean privateBrowsing) {
+        sIsHtcMail.set(isHtcMail(mWebView));

[Torne, 2016/08/12 09:24:23]

There's no need to do this in WebViewChromium and have to keep testing the
package and so on - it's much easier to just do this in
WebViewChromiumFactoryProvider.initialize() which will execute only once.

[Changwan Ryu, 2016/08/12 11:14:06]

Unfortunately, disableThreadChecking should be called every time
WebViewChromium.init() gets called because sEnforceThreadChecking will be set
every time WebView gets instantiated.

Having this logic spread out between WebViewChromium and WebViewChromiumProvider
is probably not a good idea. Isn't it?

[Torne, 2016/08/12 13:25:41]

Ah, I didn't realise it was reset in the constructor. That's terrible, and
actually broken in some vanishingly unlikely cases (multiple apps in the same
process with different target sdk levels) :(
It is if it avoids having to do atomics, or recheck package names over and over
redundantly. However, you don't have to spread it out in this case: you could
check whether it's HTC Mail once in global initialisation, and then set
sEnforceThreadChecking back to the right value in
WebViewChromiumFactoryProvider.createWebView.

[Changwan Ryu, 2016/08/16 10:30:24]

Moved the checking logic to factory and replaced atomicboolean by final.
BTW, I assume that sEnforceThreadChecking in HTC mail would be set to false
while other apps should have the right value. Why do we need to set
sEnforceThreadChecking back to the right value? 

+        if (sIsHtcMail.get()) disableThreadChecking(mWebView);
         if (privateBrowsing) {
             mFactory.startYourEngines(true);
             final String msg = "Private browsing is not supported in WebView.";
@@ -198,6 +205,55 @@ class WebViewChromium implements WebViewProvider, WebViewProvider.ScrollDelegate
         });
     }
 
+    private boolean isHtcMail(View view) {
+        if (mAppTargetSdkVersion > Build.VERSION_CODES.M) return false;
+        final String htcMailPackageId = "com.htc.android.mail";
+        if (view.getClass().getName() == null

[Torne, 2016/08/12 09:24:23]

Instead of checking the class name of a view, just call
Context.getPackageName().

[Changwan Ryu, 2016/08/12 11:14:06]

Done.

+                || !view.getClass().getName().startsWith(htcMailPackageId)) {
+            return false;
+        }
+
+        try {
+            PackageInfo packageInfo =
+                    view.getContext().getPackageManager().getPackageInfo(htcMailPackageId, 0);
+            // These values are provided by HTC.
+            if ((mAppTargetSdkVersion < Build.VERSION_CODES.M
+                    && packageInfo.versionCode < 864021756)
+                    || packageInfo.versionCode < 866001861) {
+                Log.w(TAG, "Disabling thread check in WebView (http://crbug.com/622151). "
+                        + "APK name: " + htcMailPackageId + ", versionCode: "
+                        + packageInfo.versionCode);
+                return true;
+            }
+        } catch (Exception e) {
+            e.printStackTrace();

[Torne, 2016/08/12 09:24:23]

Printing a stack trace and throwing the exception away is never really useful.
Catch more specific exception types here, and just swallow them silently; if the
package manager can't tell us the version of the mail app then we're not in a
state where enabling this workaround is going to help us :)

[Changwan Ryu, 2016/08/12 11:14:06]

Changed not to print stack trace.

+        }
+        return false;
+    }
+
+    // This is a workaround for https://crbug.com/622151.
+    // In HTC's email app, InputConnection.setComposingText() will call WebView.evaluateJavaScript,
+    // and thread assertion will occur. We turn off WebView thread assertion for this app.
+    private void disableThreadChecking(View view) {
+        for (Class<?> webViewClass = view.getClass();

[Torne, 2016/08/12 09:24:23]

No need to search the class hierarchy here (and this is fragile, since it's only
doing a substring match): just use Class.forName("android.webkit.WebView"). It's
also a static field, so it's sufficient to do this once. So, it can just be done
immediately after the check in WebViewChromiumFactoryProvider.initialize()

[Changwan Ryu, 2016/08/12 11:14:06]

Done. BTW, this should be done for each WebView.init() call.

+                webViewClass != null;
+                webViewClass = webViewClass.getSuperclass()) {
+            String name = webViewClass.getName();
+            if (name != null && name.contains("WebView")) {
+                try {
+                    Field field = webViewClass.getDeclaredField("sEnforceThreadChecking");
+                    field.setAccessible(true);
+                    field.setBoolean(null, false);
+                    field.setAccessible(false);
+                } catch (NoSuchFieldException | IllegalAccessException
+                        | IllegalArgumentException e) {
+                    e.printStackTrace();

[Torne, 2016/08/12 09:24:23]

If we failed here, probably just print a single-line error saying we couldn't
turn it off. The exception is unlikely to be interesting.

[Changwan Ryu, 2016/08/12 11:14:06]

Done.

+                }
+                return;
+            }
+        }
+    }
+
     private void initForReal() {
         AwContentsStatics.setRecordFullDocument(sRecordWholeDocumentEnabledByApi
                 || mAppTargetSdkVersion < Build.VERSION_CODES.LOLLIPOP);
@@ -540,7 +596,22 @@ class WebViewChromium implements WebViewProvider, WebViewProvider.ScrollDelegate
         mAwContents.loadDataWithBaseURL(baseUrl, data, mimeType, encoding, historyUrl);
     }
 
-    public void evaluateJavaScript(String script, ValueCallback<String> resultCallback) {
+    public void evaluateJavaScript(
+            final String script, final ValueCallback<String> resultCallback) {
+        if (sIsHtcMail.get()) {
+            // This is a workaround for https://crbug.com/622151.
+            ThreadUtils.postOnUiThread(new Runnable() {

[Torne, 2016/08/12 09:24:23]

Use checkNeedsPost() instead of doing it unconditionally, and just call
mAwContents.evaluateJavaScript directly instead of introducing the extra method.

[Changwan Ryu, 2016/08/12 11:14:06]

Changed to runOnUiThread and removed extra method.

+                @Override
+                public void run() {
+                    evaluateJavaScriptInternal(script, resultCallback);
+                }
+            });
+        } else {
+            evaluateJavaScriptInternal(script, resultCallback);
+        }
+    }
+
+    private void evaluateJavaScriptInternal(String script, ValueCallback<String> resultCallback) {
         checkThread();
         mAwContents.evaluateJavaScript(script, resultCallback);
     }