Don't alter accessibility JS APIs unless a page is about to load.

Previously we were adding and removing the APIs on window attach and
detach, but a page's life cycle is not related to attach/detach. This
patch also ensures that ChromeVox is not null before calling. Includes
formatting fixes and extra comments in the waitForResultTimedLocked
method to improve readability. Fixes parsing of integer resultId, which
was being parsed using Long.

Bug: 7328348
Change-Id: I6b81a8e4d8209f8e99419da9b8250abe57e25048

2 files changed, 96 insertions, 45 deletions

diff --git a/core/java/android/webkit/AccessibilityInjector.java b/core/java/android/webkit/AccessibilityInjector.java
index 357a16e..fe5cad4 100644
--- a/core/java/android/webkit/AccessibilityInjector.java
+++ b/core/java/android/webkit/AccessibilityInjector.java
@@ -97,9 +97,12 @@ class AccessibilityInjector {
     // Template for JavaScript that performs AndroidVox actions.
     private static final String ACCESSIBILITY_ANDROIDVOX_TEMPLATE =
             "(function() {" +
-                    "  if ((typeof(cvox) != 'undefined')"+
+                    "  if ((typeof(cvox) != 'undefined')" +
+                    "      && (cvox != null)" +
                     "      && (typeof(cvox.ChromeVox) != 'undefined')" +
+                    "      && (cvox.ChromeVox != null)" +
                     "      && (typeof(cvox.AndroidVox) != 'undefined')" +
+                    "      && (cvox.AndroidVox != null)" +
                     "      && cvox.ChromeVox.isActive) {" +
                     "    return cvox.AndroidVox.performAction('%1s');" +
                     "  } else {" +
@@ -110,9 +113,12 @@ class AccessibilityInjector {
     // JS code used to shut down an active AndroidVox instance.
     private static final String TOGGLE_CVOX_TEMPLATE =
             "javascript:(function() {" +
-                    "  if ((typeof(cvox) != 'undefined')"+
+                    "  if ((typeof(cvox) != 'undefined')" +
+                    "      && (cvox != null)" +
                     "      && (typeof(cvox.ChromeVox) != 'undefined')" +
-                    "      && (typeof(cvox.ChromeVox.host) != 'undefined')) {" +
+                    "      && (cvox.ChromeVox != null)" +
+                    "      && (typeof(cvox.ChromeVox.host) != 'undefined')" +
+                    "      && (cvox.ChromeVox.host != null)) {" +
                     "    cvox.ChromeVox.host.activateOrDeactivateChromeVox(%b);" +
                     "  }" +
                     "})();";
@@ -132,33 +138,60 @@ class AccessibilityInjector {
     }
 
     /**
+     * If JavaScript is enabled, pauses or resumes AndroidVox.
+     *
+     * @param enabled Whether feedback should be enabled.
+     */
+    public void toggleAccessibilityFeedback(boolean enabled) {
+        if (!isAccessibilityEnabled() || !isJavaScriptEnabled()) {
+            return;
+        }
+
+        toggleAndroidVox(enabled);
+
+        if (!enabled && (mTextToSpeech != null)) {
+            mTextToSpeech.stop();
+        }
+    }
+
+    /**
      * Attempts to load scripting interfaces for accessibility.
      * <p>
-     * This should be called when the window is attached.
-     * </p>
+     * This should only be called before a page loads.
      */
-    public void addAccessibilityApisIfNecessary() {
+    private void addAccessibilityApisIfNecessary() {
         if (!isAccessibilityEnabled() || !isJavaScriptEnabled()) {
             return;
         }
 
         addTtsApis();
         addCallbackApis();
-        toggleAndroidVox(true);
     }
 
     /**
      * Attempts to unload scripting interfaces for accessibility.
      * <p>
-     * This should be called when the window is detached.
-     * </p>
+     * This should only be called before a page loads.
      */
-    public void removeAccessibilityApisIfNecessary() {
-        toggleAndroidVox(false);
+    private void removeAccessibilityApisIfNecessary() {
         removeTtsApis();
         removeCallbackApis();
     }
 
+    /**
+     * Destroys this accessibility injector.
+     */
+    public void destroy() {
+        if (mTextToSpeech != null) {
+            mTextToSpeech.shutdown();
+            mTextToSpeech = null;
+        }
+
+        if (mCallback != null) {
+            mCallback = null;
+        }
+    }
+
     private void toggleAndroidVox(boolean state) {
         if (!mAccessibilityScriptInjected) {
             return;
@@ -517,7 +550,12 @@ class AccessibilityInjector {
      *         settings.
      */
     private boolean isJavaScriptEnabled() {
-        return mWebView.getSettings().getJavaScriptEnabled();
+        final WebSettings settings = mWebView.getSettings();
+        if (settings == null) {
+            return false;
+        }
+
+        return settings.getJavaScriptEnabled();
     }
 
     /**
@@ -732,7 +770,7 @@ class AccessibilityInjector {
         private final String mInterfaceName;
 
         private boolean mResult = false;
-        private long mResultId = -1;
+        private int mResultId = -1;
 
         private CallbackHandler(String interfaceName) {
             mInterfaceName = interfaceName;
@@ -784,34 +822,46 @@ class AccessibilityInjector {
          * @return Whether the result was received.
          */
         private boolean waitForResultTimedLocked(int resultId) {
-            if (DEBUG)
-                Log.d(TAG, "Waiting for CVOX result...");
-            long waitTimeMillis = RESULT_TIMEOUT;
             final long startTimeMillis = SystemClock.uptimeMillis();
+
+            if (DEBUG)
+                Log.d(TAG, "Waiting for CVOX result with ID " + resultId + "...");
+
             while (true) {
+                // Fail if we received a callback from the future.
+                if (mResultId > resultId) {
+                    if (DEBUG)
+                        Log.w(TAG, "Aborted CVOX result");
+                    return false;
+                }
+
+                final long elapsedTimeMillis = (SystemClock.uptimeMillis() - startTimeMillis);
+
+                // Succeed if we received the callback we were expecting.
+                if (DEBUG)
+                    Log.w(TAG, "Check " + mResultId + " versus expected " + resultId);
+                if (mResultId == resultId) {
+                    if (DEBUG)
+                        Log.w(TAG, "Received CVOX result after " + elapsedTimeMillis + " ms");
+                    return true;
+                }
+
+                final long waitTimeMillis = (RESULT_TIMEOUT - elapsedTimeMillis);
+
+                // Fail if we've already exceeded the timeout.
+                if (waitTimeMillis <= 0) {
+                    if (DEBUG)
+                        Log.w(TAG, "Timed out while waiting for CVOX result");
+                    return false;
+                }
+
                 try {
-                    if (mResultId == resultId) {
-                        if (DEBUG)
-                            Log.w(TAG, "Received CVOX result");
-                        return true;
-                    }
-                    if (mResultId > resultId) {
-                        if (DEBUG)
-                            Log.w(TAG, "Obsolete CVOX result");
-                        return false;
-                    }
-                    final long elapsedTimeMillis = SystemClock.uptimeMillis() - startTimeMillis;
-                    waitTimeMillis = RESULT_TIMEOUT - elapsedTimeMillis;
-                    if (waitTimeMillis <= 0) {
-                        if (DEBUG)
-                            Log.w(TAG, "Timed out while waiting for CVOX result");
-                        return false;
-                    }
+                    if (DEBUG)
+                        Log.w(TAG, "Start waiting...");
                     mResultLock.wait(waitTimeMillis);
                 } catch (InterruptedException ie) {
                     if (DEBUG)
                         Log.w(TAG, "Interrupted while waiting for CVOX result");
-                    /* ignore */
                 }
             }
         }
@@ -827,11 +877,11 @@ class AccessibilityInjector {
         @SuppressWarnings("unused")
         public void onResult(String id, String result) {
             if (DEBUG)
-                Log.w(TAG, "Saw CVOX result of '" + result + "'");
-            final long resultId;
+                Log.w(TAG, "Saw CVOX result of '" + result + "' for ID " + id);
+            final int resultId;
 
             try {
-                resultId = Long.parseLong(id);
+                resultId = Integer.parseInt(id);
             } catch (NumberFormatException e) {
                 return;
             }
@@ -840,6 +890,9 @@ class AccessibilityInjector {
                 if (resultId > mResultId) {
                     mResult = Boolean.parseBoolean(result);
                     mResultId = resultId;
+                } else {
+                    if (DEBUG)
+                        Log.w(TAG, "Result with ID " + resultId + " was stale vesus " + mResultId);
                 }
                 mResultLock.notifyAll();
             }
diff --git a/core/java/android/webkit/WebViewClassic.java b/core/java/android/webkit/WebViewClassic.java
index d68511c..6a505e1 100644
--- a/core/java/android/webkit/WebViewClassic.java
+++ b/core/java/android/webkit/WebViewClassic.java
@@ -2132,6 +2132,10 @@ public final class WebViewClassic implements WebViewProvider, WebViewProvider.Sc
 
     private void destroyJava() {
         mCallbackProxy.blockMessages();
+        if (mAccessibilityInjector != null) {
+            mAccessibilityInjector.destroy();
+            mAccessibilityInjector = null;
+        }
         if (mWebViewCore != null) {
             // Tell WebViewCore to destroy itself
             synchronized (this) {
@@ -3967,8 +3971,6 @@ public final class WebViewClassic implements WebViewProvider, WebViewProvider.Sc
         // null, and that will be the case
         mWebView.setCertificate(null);
 
-        // reset the flag since we set to true in if need after
-        // loading is see onPageFinished(Url)
         if (isAccessibilityInjectionEnabled()) {
             getAccessibilityInjector().onPageStarted(url);
         }
@@ -5384,7 +5386,7 @@ public final class WebViewClassic implements WebViewProvider, WebViewProvider.Sc
         if (mWebView.hasWindowFocus()) setActive(true);
 
         if (isAccessibilityInjectionEnabled()) {
-            getAccessibilityInjector().addAccessibilityApisIfNecessary();
+            getAccessibilityInjector().toggleAccessibilityFeedback(true);
         }
 
         updateHwAccelerated();
@@ -5397,11 +5399,7 @@ public final class WebViewClassic implements WebViewProvider, WebViewProvider.Sc
         if (mWebView.hasWindowFocus()) setActive(false);
 
         if (isAccessibilityInjectionEnabled()) {
-            getAccessibilityInjector().removeAccessibilityApisIfNecessary();
-        } else {
-            // Ensure the injector is cleared if we're detaching from the window
-            // and accessibility is disabled.
-            mAccessibilityInjector = null;
+            getAccessibilityInjector().toggleAccessibilityFeedback(false);
         }
 
         updateHwAccelerated();