Last Comment Bug 713774 - Crash on rotation after adjusting viewport - part ii
: Crash on rotation after adjusting viewport - part ii
Status: VERIFIED FIXED
: crash, regression, reproducible, topcrash
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: Trunk
: ARM Android
: P2 critical (vote)
: Firefox 12
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
: Sebastian Kaspari (:sebastian)
Mentors:
about:home
Depends on: 711426
Blocks: 708307 711852
  Show dependency treegraph
 
Reported: 2011-12-27 16:13 PST by Aaron Train [:aaronmt]
Modified: 2012-01-19 09:15 PST (History)
20 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
+
verified
11+


Attachments
Nightly (12/27) raw log (25.16 KB, text/plain)
2011-12-27 16:13 PST, Aaron Train [:aaronmt]
no flags Details
Fix typos causing a crash after orientation changes (1.87 KB, patch)
2012-01-04 09:05 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
no flags Details | Diff | Splinter Review
Same patch with more context (2.81 KB, patch)
2012-01-04 10:08 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
blassey.bugs: review+
Details | Diff | Splinter Review
Modified patch that was committed to m-c (3.29 KB, text/plain)
2012-01-05 08:53 PST, James Willcox (:snorp) (jwillcox@mozilla.com)
snorp: review+
akeybl: approval‑mozilla‑aurora+
Details

Description Aaron Train [:aaronmt] 2011-12-27 16:13:54 PST
Created attachment 584502 [details]
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 2 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-01-03 17:54:42 PST
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] 2012-01-04 06:42:56 PST
(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) (jwillcox@mozilla.com) 2012-01-04 09:05:14 PST
Created attachment 585778 [details] [diff] [review]
Fix typos causing a crash after orientation changes
Comment 5 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-04 09:06:23 PST
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) (jwillcox@mozilla.com) 2012-01-04 10:08:01 PST
Created attachment 585800 [details] [diff] [review]
Same patch with more context
Comment 7 Aaron Train [:aaronmt] 2012-01-04 10:44:16 PST
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?) 2012-01-04 10:55:11 PST
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) (jwillcox@mozilla.com) 2012-01-04 11:29:35 PST
(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?) 2012-01-04 15:08:38 PST
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) (jwillcox@mozilla.com) 2012-01-04 18:55:54 PST
(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) (jwillcox@mozilla.com) 2012-01-05 07:54:12 PST
Landed on m-c:

https://hg.mozilla.org/mozilla-central/rev/df09a93f0887
Comment 13 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-05 08:53:09 PST
Created attachment 586104 [details]
Modified patch that was committed to m-c
Comment 14 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-05 08:54:31 PST
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] 2012-01-05 13:56:22 PST
I'll leave this in the queue to let it bake on m-c for a day and come back to approve tomorrow.
Comment 16 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-01-06 07:11:31 PST
*** Bug 711852 has been marked as a duplicate of this bug. ***
Comment 17 Alex Keybl [:akeybl] 2012-01-06 11:02:06 PST
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 2012-01-10 07:17:26 PST
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) (jwillcox@mozilla.com) 2012-01-10 10:34:53 PST
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?) 2012-01-19 09:08:20 PST
(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

Note You need to log in before you can comment on or make changes to this bug.