Crash on rotation after adjusting viewport - part ii

VERIFIED FIXED in Firefox 11

Status

()

Firefox for Android
General
P2
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: aaronmt, Assigned: snorp)

Tracking

(4 keywords)

Trunk
Firefox 12
ARM
Android
crash, regression, reproducible, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox11+ fixed, firefox12+ verified, fennec11+)

Details

(URL)

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

Updated

5 years ago
tracking-firefox11: ? → +
tracking-firefox12: ? → +
(Reporter)

Comment 1

5 years ago
Range from central

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

I see bug 708307.
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.
(Reporter)

Comment 3

5 years ago
(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.
Created attachment 585778 [details] [diff] [review]
Fix typos causing a crash after orientation changes
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.
Attachment #585778 - Flags: review?(blassey.bugs)
Created attachment 585800 [details] [diff] [review]
Same patch with more context
Attachment #585778 - Attachment is obsolete: true
Attachment #585778 - Flags: review?(blassey.bugs)
Attachment #585800 - Flags: review?(blassey.bugs)
(Reporter)

Comment 7

5 years ago
Patch posted in a custom build in #mobile works for me so far on the Nexus S and Galaxy SII.
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?
(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 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.
Attachment #585800 - Flags: review?(blassey.bugs) → review+
(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
Landed on m-c:

https://hg.mozilla.org/mozilla-central/rev/df09a93f0887
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Target Milestone: --- → Firefox 12
Created attachment 586104 [details]
Modified patch that was committed to m-c
Attachment #586104 - Flags: review+
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.
Attachment #586104 - Flags: approval-mozilla-aurora?
I'll leave this in the queue to let it bake on m-c for a day and come back to approve tomorrow.
Duplicate of this bug: 711852
Comment on attachment 586104 [details]
Modified patch that was committed to m-c

[Triage Comment]
Mobile only and top crash - approved for Aurora.
Attachment #586104 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → 11+

Updated

5 years ago
Blocks: 711852
Assignee: nobody → snorp
status-firefox12: affected → fixed

Comment 18

5 years ago
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.
Status: RESOLVED → VERIFIED
status-firefox11: affected → verified
status-firefox12: fixed → verified
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.
(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

Updated

5 years ago
status-firefox11: verified → fixed
You need to log in before you can comment on or make changes to this bug.