aw: Fix detach and destroy interaction

aw: Fix detach and destroy interaction

Previously we cannot destroy native side before detach because we would
have leaked hardware resources. This is no longer the case ever since
requestDrawGL takes a waitForCompletion parameter. Now we can
synchronously call nativeOnDetachedFromWindow from inside destroy to
clean up hardware resources, and then actually destroy native side
immediately.

Then also found that AwSettings will crash after destroying native since
it does not always check that the native pointer is not 0. Fixed these
and added a test.

BUG=

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=268034

[boliu, 2014-05-02 23:10:05]

sgurun for review

hush/mnaganov for fyi

[hush (inactive), 2014-05-02 23:42:27]

https://codereview.chromium.org/263903008/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/263903008/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:638: *
Deletes the native counterpart of this object. Normally happens immediately,
documentation here needs to be updated as well.

[boliu, 2014-05-02 23:45:31]

https://codereview.chromium.org/263903008/diff/1/android_webview/java/src/org...
File android_webview/java/src/org/chromium/android_webview/AwContents.java
(right):

https://codereview.chromium.org/263903008/diff/1/android_webview/java/src/org...
android_webview/java/src/org/chromium/android_webview/AwContents.java:638: *
Deletes the native counterpart of this object. Normally happens immediately,
On 2014/05/02 23:42:28, hush wrote:
> documentation here needs to be updated as well.

Done. Moar deletions!

[sgurun-gerrit only, 2014-05-02 23:54:34]

https://codereview.chromium.org/263903008/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
(right):

https://codereview.chromium.org/263903008/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:155:
assertEquals(newBlockNetworkLoads, awSettings.getBlockNetworkLoads());
I think you also want to test statics too?

[sgurun-gerrit only, 2014-05-03 00:20:25]

lgtm

https://codereview.chromium.org/263903008/diff/1/android_webview/javatests/sr...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java
(right):

https://codereview.chromium.org/263903008/diff/1/android_webview/javatests/sr...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsTest.java:155:
assertEquals(newBlockNetworkLoads, awSettings.getBlockNetworkLoads());
himm scratch this. they are statics already :)
On 2014/05/02 23:54:34, sgurun wrote:
> I think you also want to test statics too?

[boliu, 2014-05-03 01:01:00]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-03 01:01:25]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/263903008/20001

[commit-bot: I haz the power, 2014-05-03 01:01:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-05-03 01:01:30]

Failed to apply patch for
android_webview/java/src/org/chromium/android_webview/AwContents.java:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file
android_webview/java/src/org/chromium/android_webview/AwContents.java
  Hunk #1 FAILED at 61.
  Hunk #2 succeeded at 240 (offset 1 line).
  Hunk #3 succeeded at 638 (offset 1 line).
  Hunk #4 succeeded at 656 (offset 1 line).
  Hunk #5 succeeded at 1616 (offset 1 line).
  1 out of 5 hunks FAILED -- saving rejects to file
android_webview/java/src/org/chromium/android_webview/AwContents.java.rej

Patch:      
android_webview/java/src/org/chromium/android_webview/AwContents.java
Index: android_webview/java/src/org/chromium/android_webview/AwContents.java
diff --git
a/android_webview/java/src/org/chromium/android_webview/AwContents.java
b/android_webview/java/src/org/chromium/android_webview/AwContents.java
index
57e4314632820e3d0589cbcc53a9510af2c572a9..9a507482b588c13a949dc9ce0657058df01f886d
100644
--- a/android_webview/java/src/org/chromium/android_webview/AwContents.java
+++ b/android_webview/java/src/org/chromium/android_webview/AwContents.java
@@ -61,9 +61,7 @@ import java.io.File;
 import java.lang.annotation.Annotation;
 import java.net.MalformedURLException;
 import java.net.URL;
-import java.util.ArrayList;
 import java.util.HashMap;
-import java.util.List;
 import java.util.Map;
 import java.util.concurrent.Callable;
 
@@ -239,11 +237,6 @@ public class AwContents {
     // (ie before it is destroyed).
     private CleanupReference mCleanupReference;
 
-    // A list of references to native pointers where the Java counterpart has
been
-    // destroyed, but are held here because they are waiting for
onDetachFromWindow
-    // to release GL resources. This is cleared inside onDetachFromWindow.
-    private List<CleanupReference> mPendingDetachCleanupReferences;
-
    
//--------------------------------------------------------------------------------------------
     private class IoThreadClientImpl implements AwContentsIoThreadClient {
         // All methods are called on the IO thread.
@@ -642,13 +635,17 @@ public class AwContents {
     }
 
     /**
-     * Deletes the native counterpart of this object. Normally happens
immediately,
-     * but maybe deferred until the appropriate time for GL resource cleanup.
Either way
-     * this is transparent to the caller: after this function returns the
object is
-     * effectively dead and methods are no-ops.
+     * Deletes the native counterpart of this object.
      */
     public void destroy() {
         if (mCleanupReference != null) {
+            assert mNativeAwContents != 0;
+            // If we are attached, we have to call native detach to clean up
+            // hardware resources.
+            if (mIsAttachedToWindow) {
+                nativeOnDetachedFromWindow(mNativeAwContents);
+            }
+
             // We explicitly do not null out the mContentViewCore reference
here
             // because ContentViewCore already has code to deal with the case
             // methods are called on it after it's been destroyed, and other
@@ -656,17 +653,7 @@ public class AwContents {
             mContentViewCore.destroy();
             mNativeAwContents = 0;
 
-            // We cannot destroy immediately if we are still attached to the
window.
-            // Instead if we make sure to null out the native pointer so there
is no more native
-            // calls, and delay the actual destroy until onDetachedFromWindow.
-            if (mIsAttachedToWindow) {
-                if (mPendingDetachCleanupReferences == null) {
-                    mPendingDetachCleanupReferences = new
ArrayList<CleanupReference>();
-                }
-                mPendingDetachCleanupReferences.add(mCleanupReference);
-            } else {
-                mCleanupReference.cleanupNow();
-            }
+            mCleanupReference.cleanupNow();
             mCleanupReference = null;
         }
 
@@ -1626,13 +1613,6 @@ public class AwContents {
         }
 
         mScrollAccessibilityHelper.removePostedCallbacks();
-
-        if (mPendingDetachCleanupReferences != null) {
-            for (int i = 0; i < mPendingDetachCleanupReferences.size(); ++i) {
-                mPendingDetachCleanupReferences.get(i).cleanupNow();
-            }
-            mPendingDetachCleanupReferences = null;
-        }
     }
 
     /**

[boliu, 2014-05-03 01:29:24]

The CQ bit was checked by boliu@chromium.org

[commit-bot: I haz the power, 2014-05-03 01:30:56]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/boliu@chromium.org/263903008/40001

[commit-bot: I haz the power, 2014-05-03 03:42:32]

Change committed as 268034