Bug 711426 - Fix some causes of crashes when rotating. r=kats
authorChris Lord <chrislord.net@gmail.com>
Sat, 24 Dec 2011 00:49:00 +0000
changeset 84525 2f6873cba874adaa9a5fd6afc80b480bd45f769e
parent 84524 c4033a3d158efea487688db70e69a894cde1d50d
child 84526 8d1b087b9713400ef46de7e42725b0fb4603cc84
push id805
push userakeybl@mozilla.com
push dateWed, 01 Feb 2012 18:17:35 +0000
treeherdermozilla-aurora@6fb3bf232436 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskats
bugs711426
milestone12.0a1
Bug 711426 - Fix some causes of crashes when rotating. r=kats The lock on the software buffer was not being respected when destroying the surface on screen rotation, meaning we could destroy it while Gecko was still drawing to it. This would certainly cause a crash on rotation under the right conditions. The same situation also occurred in GeckoSoftwareLayerClient.getBitmap(). We also waited until the next redraw when freeing the old texture associated with the surface. This had the effect of temporarily increasing the memory usage (generally by either 4.5 or 9 megabytes). If memory pressure is high, this could also cause a crash, though it is far less likely than the above case.
mobile/android/base/gfx/GeckoSoftwareLayerClient.java
mobile/android/base/gfx/TileLayer.java
widget/src/android/AndroidJavaWrappers.cpp
widget/src/android/AndroidJavaWrappers.h
widget/src/android/nsWindow.cpp
--- a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
+++ b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
@@ -139,18 +139,36 @@ public class GeckoSoftwareLayerClient ex
         if (mGeckoViewport != null) {
             layerController.setViewportMetrics(mGeckoViewport);
         }
 
         GeckoAppShell.registerGeckoEventListener("Viewport:Update", this);
         GeckoAppShell.registerGeckoEventListener("Viewport:UpdateLater", this);
     }
 
-    public void beginDrawing() {
+    public void beginDrawing(int width, int height) {
         beginTransaction(mTileLayer);
+
+        if (mBufferSize.width != width || mBufferSize.height != height) {
+            mBufferSize = new IntSize(width, height);
+
+            // Reallocate the buffer if necessary
+
+            // * 2 because it's a 16-bit buffer (so 2 bytes per pixel).
+            int size = mBufferSize.getArea() * 2;
+            if (mBuffer == null || mBuffer.capacity() != size) {
+                // Free the old buffer
+                if (mBuffer != null) {
+                    GeckoAppShell.freeDirectBuffer(mBuffer);
+                    mBuffer = null;
+                }
+
+                mBuffer = GeckoAppShell.allocateDirectBuffer(size);
+            }
+        }
     }
 
     private void updateViewport(String viewportDescription, final boolean onlyUpdatePageSize) {
         try {
             JSONObject viewportObject = new JSONObject(viewportDescription);
 
             // save and restore the viewport size stored in java; never let the
             // JS-side viewport dimensions override the java-side ones because
@@ -204,26 +222,33 @@ public class GeckoSoftwareLayerClient ex
     public ViewportMetrics getGeckoViewportMetrics() {
         // Return a copy, as we modify this inside the Gecko thread
         if (mGeckoViewport != null)
             return new ViewportMetrics(mGeckoViewport);
         return null;
     }
 
     public Bitmap getBitmap() {
-        if (mBufferSize.width <= 0 || mBufferSize.height <= 0)
-            return null;
+        // Begin a tile transaction, otherwise the buffer can be destroyed while
+        // we're reading from it.
+        beginTransaction(mTileLayer);
         try {
-            Bitmap b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
-                                           CairoUtils.cairoFormatTobitmapConfig(mFormat));
-            b.copyPixelsFromBuffer(mBuffer.asIntBuffer());
-            return b;
-        } catch (OutOfMemoryError oom) {
-            Log.w(LOGTAG, "Unable to create bitmap", oom);
-            return null;
+            if (mBuffer == null || mBufferSize.width <= 0 || mBufferSize.height <= 0)
+                return null;
+            try {
+                Bitmap b = Bitmap.createBitmap(mBufferSize.width, mBufferSize.height,
+                                               CairoUtils.cairoFormatTobitmapConfig(mFormat));
+                b.copyPixelsFromBuffer(mBuffer.asIntBuffer());
+                return b;
+            } catch (OutOfMemoryError oom) {
+                Log.w(LOGTAG, "Unable to create bitmap", oom);
+                return null;
+            }
+        } finally {
+            endTransaction(mTileLayer);
         }
     }
 
     /** Returns the back buffer. This function is for Gecko to use. */
     public ByteBuffer lockBuffer() {
         return mBuffer;
     }
 
@@ -246,31 +271,22 @@ public class GeckoSoftwareLayerClient ex
             mScreenSize = new IntSize(metrics.widthPixels, metrics.heightPixels);
             int maxSize = getLayerController().getView().getMaxTextureSize();
 
             // XXX Introduce tiling to solve this?
             if (mScreenSize.width > maxSize || mScreenSize.height > maxSize)
                 throw new RuntimeException("Screen size of " + mScreenSize + " larger than maximum texture size of " + maxSize);
 
             // Round to next power of two until we use NPOT texture support
-            mBufferSize = new IntSize(Math.min(maxSize, IntSize.nextPowerOfTwo(mScreenSize.width + LayerController.MIN_BUFFER.width)),
-                                      Math.min(maxSize, IntSize.nextPowerOfTwo(mScreenSize.height + LayerController.MIN_BUFFER.height)));
-
-            // Free the old buffer first, if it exists
-            if (mBuffer != null) {
-                GeckoAppShell.freeDirectBuffer(mBuffer);
-                mBuffer = null;
-            }
-
-            // * 2 because it's a 16-bit buffer (so 2 bytes per pixel).
-            mBuffer = GeckoAppShell.allocateDirectBuffer(mBufferSize.getArea() * 2);
+            IntSize bufferSize = new IntSize(Math.min(maxSize, IntSize.nextPowerOfTwo(mScreenSize.width + LayerController.MIN_BUFFER.width)),
+                                             Math.min(maxSize, IntSize.nextPowerOfTwo(mScreenSize.height + LayerController.MIN_BUFFER.height)));
 
             Log.i(LOGTAG, "Screen-size changed to " + mScreenSize);
             GeckoEvent event = new GeckoEvent(GeckoEvent.SIZE_CHANGED,
-                                              mBufferSize.width, mBufferSize.height,
+                                              bufferSize.width, bufferSize.height,
                                               metrics.widthPixels, metrics.heightPixels);
             GeckoAppShell.sendEventToGecko(event);
         }
 
         render();
     }
 
     @Override
--- a/mobile/android/base/gfx/TileLayer.java
+++ b/mobile/android/base/gfx/TileLayer.java
@@ -93,17 +93,17 @@ public abstract class TileLayer extends 
         mDirtyRect.union(rect);
     }
 
     public void invalidate() {
         IntSize bufferSize = mImage.getSize();
         invalidate(new Rect(0, 0, bufferSize.width, bufferSize.height));
     }
 
-    private void validateTexture() {
+    private void validateTexture(GL10 gl) {
         /* Calculate the ideal texture size. This must be a power of two if
          * the texture is repeated or OpenGL ES 2.0 isn't supported, as
          * OpenGL ES 2.0 is required for NPOT texture support (without
          * extensions), but doesn't support repeating NPOT textures.
          *
          * XXX Currently, we don't pick a GLES 2.0 context, so always round.
          */
         IntSize bufferSize = mImage.getSize();
@@ -114,29 +114,29 @@ public abstract class TileLayer extends 
         if (!textureSize.equals(mSize)) {
             mSize = textureSize;
 
             // Delete the old texture
             if (mTextureIDs != null) {
                 TextureReaper.get().add(mTextureIDs);
                 mTextureIDs = null;
 
-                // XXX This won't be freed until the next frame is drawn, so we
-                //     temporarily have a larger-than-necessary memory requirement.
-                //     Is this what we want?
+                // Free the texture immediately, so we don't incur a
+                // temporarily increased memory usage.
+                TextureReaper.get().reap(gl);
             }
         }
     }
 
     @Override
     protected void performUpdates(GL10 gl) {
         super.performUpdates(gl);
 
         // Reallocate the texture if the size has changed
-        validateTexture();
+        validateTexture(gl);
 
         // Don't do any work if the image has an invalid size.
         if (!mImage.getSize().isPositive())
             return;
 
         // If we haven't allocated a texture, assume the whole region is dirty
         if (mTextureIDs == null)
             uploadFullTexture(gl);
@@ -147,19 +147,25 @@ public abstract class TileLayer extends 
     }
 
     private void uploadFullTexture(GL10 gl) {
         IntSize bufferSize = mImage.getSize();
         uploadDirtyRect(gl, new Rect(0, 0, bufferSize.width, bufferSize.height));
     }
 
     private void uploadDirtyRect(GL10 gl, Rect dirtyRect) {
+        // If we have nothing to upload, just return for now
         if (dirtyRect.isEmpty())
             return;
 
+        // It's possible that the buffer will be null, check for that and return
+        ByteBuffer imageBuffer = mImage.getBuffer();
+        if (imageBuffer == null)
+            return;
+
         boolean newlyCreated = false;
 
         if (mTextureIDs == null) {
             mTextureIDs = new int[1];
             gl.glGenTextures(mTextureIDs.length, mTextureIDs, 0);
             newlyCreated = true;
         }
 
@@ -169,35 +175,40 @@ public abstract class TileLayer extends 
         int cairoFormat = mImage.getFormat();
         CairoGLInfo glInfo = new CairoGLInfo(cairoFormat);
 
         bindAndSetGLParameters(gl);
 
         if (newlyCreated || dirtyRect.contains(bufferRect)) {
             if (mSize.equals(bufferSize)) {
                 gl.glTexImage2D(gl.GL_TEXTURE_2D, 0, glInfo.internalFormat, mSize.width, mSize.height,
-                                0, glInfo.format, glInfo.type, mImage.getBuffer());
+                                0, glInfo.format, glInfo.type, imageBuffer);
                 return;
             } else {
                 gl.glTexImage2D(gl.GL_TEXTURE_2D, 0, glInfo.internalFormat, mSize.width, mSize.height,
                                 0, glInfo.format, glInfo.type, null);
                 gl.glTexSubImage2D(gl.GL_TEXTURE_2D, 0, 0, 0, bufferSize.width, bufferSize.height,
-                                   glInfo.format, glInfo.type, mImage.getBuffer());
+                                   glInfo.format, glInfo.type, imageBuffer);
                 return;
             }
         }
 
+        // Make sure that the dirty region intersects with the buffer rect,
+        // otherwise we'll end up with an invalid buffer pointer.
+        if (!Rect.intersects(dirtyRect, bufferRect))
+            return;
+
         /*
          * Upload the changed rect. We have to widen to the full width of the texture
          * because we can't count on the device having support for GL_EXT_unpack_subimage,
          * and going line-by-line is too slow.
          *
          * XXX We should still use GL_EXT_unpack_subimage when available.
          */
-        Buffer viewBuffer = mImage.getBuffer().slice();
+        Buffer viewBuffer = imageBuffer.slice();
         int bpp = CairoUtils.bitsPerPixelForCairoFormat(cairoFormat) / 8;
         int position = dirtyRect.top * bufferSize.width * bpp;
         if (position > viewBuffer.limit()) {
             Log.e(LOGTAG, "### Position outside tile! " + dirtyRect.top);
             return;
         }
 
         viewBuffer.position(position);
--- a/widget/src/android/AndroidJavaWrappers.cpp
+++ b/widget/src/android/AndroidJavaWrappers.cpp
@@ -314,17 +314,17 @@ AndroidGeckoSoftwareLayerClient::InitGec
 #ifdef MOZ_JAVA_COMPOSITOR
     initInit();
 
     jGeckoSoftwareLayerClientClass =
         getClassGlobalRef("org/mozilla/gecko/gfx/GeckoSoftwareLayerClient");
 
     jLockBufferMethod = getMethod("lockBuffer", "()Ljava/nio/ByteBuffer;");
     jUnlockBufferMethod = getMethod("unlockBuffer", "()V");
-    jBeginDrawingMethod = getMethod("beginDrawing", "()V");
+    jBeginDrawingMethod = getMethod("beginDrawing", "(II)V");
     jEndDrawingMethod = getMethod("endDrawing", "(IIIILjava/lang/String;)V");
 #endif
 }
 
 #undef initInit
 #undef initClassGlobalRef
 #undef getField
 #undef getMethod
@@ -599,21 +599,21 @@ void
 AndroidGeckoSoftwareLayerClient::UnlockBuffer()
 {
     NS_ASSERTION(!isNull(), "UnlockBuffer() called on null software layer client!");
     AndroidBridge::AutoLocalJNIFrame(1);
     JNI()->CallVoidMethod(wrapped_obj, jUnlockBufferMethod);
 }
 
 void
-AndroidGeckoSoftwareLayerClient::BeginDrawing()
+AndroidGeckoSoftwareLayerClient::BeginDrawing(int aWidth, int aHeight)
 {
     NS_ASSERTION(!isNull(), "BeginDrawing() called on null software layer client!");
     AndroidBridge::AutoLocalJNIFrame(1);
-    return JNI()->CallVoidMethod(wrapped_obj, jBeginDrawingMethod);
+    return JNI()->CallVoidMethod(wrapped_obj, jBeginDrawingMethod, aWidth, aHeight);
 }
 
 void
 AndroidGeckoSoftwareLayerClient::EndDrawing(const nsIntRect &aRect, const nsAString &aMetadata)
 {
     NS_ASSERTION(!isNull(), "EndDrawing() called on null software layer client!");
     AndroidBridge::AutoLocalJNIFrame(1);
     jstring jMetadata = JNI()->NewString(nsPromiseFlatString(aMetadata).get(), aMetadata.Length());
--- a/widget/src/android/AndroidJavaWrappers.h
+++ b/widget/src/android/AndroidJavaWrappers.h
@@ -156,17 +156,17 @@ public:
      void Init(jobject jobj);
  
     AndroidGeckoSoftwareLayerClient() {}
     AndroidGeckoSoftwareLayerClient(jobject jobj) { Init(jobj); }
 
     jobject LockBuffer();
     unsigned char *LockBufferBits();
     void UnlockBuffer();
-    void BeginDrawing();
+    void BeginDrawing(int aWidth, int aHeight);
     void EndDrawing(const nsIntRect &aRect, const nsAString &aMetadata);
 
 private:
     static jclass jGeckoSoftwareLayerClientClass;
     static jmethodID jLockBufferMethod;
     static jmethodID jUnlockBufferMethod;
 
 protected:
--- a/widget/src/android/nsWindow.cpp
+++ b/widget/src/android/nsWindow.cpp
@@ -1165,17 +1165,17 @@ nsWindow::OnDraw(AndroidGeckoEvent *ae)
     AndroidBridge::AutoLocalJNIFrame jniFrame;
 #ifdef MOZ_JAVA_COMPOSITOR
     // We haven't been given a window-size yet, so do nothing
     if (gAndroidBounds.width <= 0 || gAndroidBounds.height <= 0)
         return;
 
     AndroidGeckoSoftwareLayerClient &client =
         AndroidBridge::Bridge()->GetSoftwareLayerClient();
-    client.BeginDrawing();
+    client.BeginDrawing(gAndroidBounds.width, gAndroidBounds.height);
 
     nsAutoString metadata;
     unsigned char *bits = NULL;
     if (sHasDirectTexture) {
       if ((sDirectTexture->Width() != gAndroidBounds.width ||
            sDirectTexture->Height() != gAndroidBounds.height) &&
           gAndroidBounds.width != 0 && gAndroidBounds.height != 0) {
         sDirectTexture->Reallocate(gAndroidBounds.width, gAndroidBounds.height);