713774 - Crash on rotation after adjusting viewport - part ii

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

[Aaron Train [:aaronmt]]

Attachment: Nightly (12/27) raw log

+++ This bug was initially created as a clone of Bug #711426 +++

See attached log

STR:

1. about:home or any website
2. Rotate to landscape (if under portrait), rotate to portrait and back to landscape (if under landscape)

On both the SII and Nexus S, rotating from landscape to portrait (on about:home and any other page) will yield the crash.

On both the SII and Nexus S, restoring from session restore in landscape, and rotating to portrait will not yield the crash. But, rotating back to landscape will yield the crash.

On both the SII/Nexus S, I was getting plenty of the following entries (not sure if that's related).

/AndroidGraphicBuffer(10410): GL error [glEGLImageTargetTexture2DOES]:                                      501

--
Mozilla/5.0 (Android; Linux armv7l; rv:12.0a1) Gecko/20111227 Firefox/12.0a1 Fennec/12.0a1

Comment 1

[Aaron Train [:aaronmt]]

Range from central

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5efcb9c3b375&tochange=80ac06ad280d

I see bug 708307.

Comment 2

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Do you get an crash report in about:crashes?  If so please place in the crash signature.  I am guessing that this is due to the neon_arm_fill.

Comment 3

[Aaron Train [:aaronmt]]

(In reply to Naoki Hirata :nhirata from comment #2)
> Do you get an crash report in about:crashes?  If so please place in the
> crash signature.  I am guessing that this is due to the neon_arm_fill.

bp-09a72c3e-fd07-4c2f-be20-2909a2120104 -- and I'm positive it's bug 708307 from that range in comment #1.

Comment 4

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Fix typos causing a crash after orientation changes

Comment 5

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 585778 [details] [diff] [review]
Fix typos causing a crash after orientation changes

This should fix things up. Embarrassing.

The last hunk is just a little cleanup, unrelated to the fix.

Comment 6

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Same patch with more context

Comment 7

[Aaron Train [:aaronmt]]

Patch posted in a custom build in #mobile works for me so far on the Nexus S and Galaxy SII.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 585800 [details] [diff] [review]
Same patch with more context

Review of attachment 585800 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/android/AndroidGraphicBuffer.cpp
@@ -306,5 @@
>  AndroidGraphicBuffer::EnsureBufferCreated(PRUint32 aWidth, PRUint32 aHeight, PRUint32 aUsage, gfxImageFormat aFormat)
>  {
>    if (!mHandle) {
>      mHandle = malloc(GRAPHIC_BUFFER_SIZE);
> -    sGLFunctions.fGraphicBufferCtor(mHandle, mWidth, mHeight, GetAndroidFormat(aFormat), GetAndroidUsage(aUsage));

shouldn't mWidth == aWidth? When would they be different?

::: widget/src/android/nsWindow.cpp
@@ -1176,5 @@
>      unsigned char *bits = NULL;
>      if (sHasDirectTexture) {
> -      if ((sDirectTexture->Width() != gAndroidBounds.width ||
> -           sDirectTexture->Height() != gAndroidBounds.height) &&
> -          gAndroidBounds.width != 0 && gAndroidBounds.height != 0) {

was this causing an issue, or are you just removing it because its unneeded? If its unneeded, why?

Comment 9

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Brad Lassey [:blassey] from comment #8)
> Comment on attachment 585800 [details] [diff] [review]
> Same patch with more context
> 
> Review of attachment 585800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/src/android/AndroidGraphicBuffer.cpp
> @@ -306,5 @@
> >  AndroidGraphicBuffer::EnsureBufferCreated(PRUint32 aWidth, PRUint32 aHeight, PRUint32 aUsage, gfxImageFormat aFormat)
> >  {
> >    if (!mHandle) {
> >      mHandle = malloc(GRAPHIC_BUFFER_SIZE);
> > -    sGLFunctions.fGraphicBufferCtor(mHandle, mWidth, mHeight, GetAndroidFormat(aFormat), GetAndroidUsage(aUsage));
> 
> shouldn't mWidth == aWidth? When would they be different?

When we are resizing (reallocating) they are different until we reassign.

> 
> ::: widget/src/android/nsWindow.cpp
> @@ -1176,5 @@
> >      unsigned char *bits = NULL;
> >      if (sHasDirectTexture) {
> > -      if ((sDirectTexture->Width() != gAndroidBounds.width ||
> > -           sDirectTexture->Height() != gAndroidBounds.height) &&
> > -          gAndroidBounds.width != 0 && gAndroidBounds.height != 0) {
> 
> was this causing an issue, or are you just removing it because its unneeded?
> If its unneeded, why?

It wasn't causing any problems, just removed it because I noticed it was redundant. Someone added the bounds == 0 check above it.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 585800 [details] [diff] [review]
Same patch with more context

Review of attachment 585800 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/src/android/AndroidGraphicBuffer.cpp
@@ -306,5 @@
>  AndroidGraphicBuffer::EnsureBufferCreated(PRUint32 aWidth, PRUint32 aHeight, PRUint32 aUsage, gfxImageFormat aFormat)
>  {
>    if (!mHandle) {
>      mHandle = malloc(GRAPHIC_BUFFER_SIZE);
> -    sGLFunctions.fGraphicBufferCtor(mHandle, mWidth, mHeight, GetAndroidFormat(aFormat), GetAndroidUsage(aUsage));

It would seem better to me to set mWidth and mHeight when you allocate mHandle, and then use them in the constructor.

r=me if you do that. If for some reason you don't think that's correct, please re-request review.

Comment 11

[James Willcox (:snorp) ([e-mail]) (he/him)]

(In reply to Brad Lassey [:blassey] from comment #10)
> Comment on attachment 585800 [details] [diff] [review]
> Same patch with more context
> 
> Review of attachment 585800 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/src/android/AndroidGraphicBuffer.cpp
> @@ -306,5 @@
> >  AndroidGraphicBuffer::EnsureBufferCreated(PRUint32 aWidth, PRUint32 aHeight, PRUint32 aUsage, gfxImageFormat aFormat)
> >  {
> >    if (!mHandle) {
> >      mHandle = malloc(GRAPHIC_BUFFER_SIZE);
> > -    sGLFunctions.fGraphicBufferCtor(mHandle, mWidth, mHeight, GetAndroidFormat(aFormat), GetAndroidUsage(aUsage));
> 
> It would seem better to me to set mWidth and mHeight when you allocate
> mHandle, and then use them in the constructor.
> 
> r=me if you do that. If for some reason you don't think that's correct,
> please re-request review.

You're right, that's much better. Pushed to inbound.

http://hg.mozilla.org/integration/mozilla-inbound/rev/df09a93f0887

Comment 12

[James Willcox (:snorp) ([e-mail]) (he/him)]

Landed on m-c:

https://hg.mozilla.org/mozilla-central/rev/df09a93f0887

Comment 13

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Modified patch that was committed to m-c

Comment 14

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 586104 [details]
Modified patch that was committed to m-c

This is one of the top crashers in Aurora and has little risk. Without this patch, quite a few devices will crash when orientation is changed.

Comment 15

[Alex Keybl [:akeybl]]

I'll leave this in the queue to let it bake on m-c for a day and come back to approve tomorrow.

Comment 17

[Alex Keybl [:akeybl]]

Comment on attachment 586104 [details]
Modified patch that was committed to m-c

[Triage Comment]
Mobile only and top crash - approved for Aurora.

Comment 18

[Camelia Urian]

Verified with:
Aurora 11.0a2 (2012-01-10) Samsung Nexus S (Android 2.3.6)
Nightly 12.0a1 (2012-01-10) Samsung Nexus S (Android 2.3.6)

No crashes seen on rotation.

Comment 19

[James Willcox (:snorp) ([e-mail]) (he/him)]

Actually the gralloc stuff was (unknown to me) backed out in Aurora on 12/17, so we don't need this fix. I guess if we want to reland the gralloc bits on aurora we'd need this, but I don't know if we want that just yet.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to Camelia Urian from comment #18)
> Verified with:
> Aurora 11.0a2 (2012-01-10) Samsung Nexus S (Android 2.3.6)
> Nightly 12.0a1 (2012-01-10) Samsung Nexus S (Android 2.3.6)
> 
> No crashes seen on rotation.

not sure how this was verified on aurora when it hadn't landed there yet

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7f3c25137b1