711426 - Crash on rotation after adjusting viewport

Status: VERIFIED FIXED

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

Description

[Aaron Train [:aaronmt]]

GeckoFavicons( 1123): Loaded favicon from DB successfully for URL = http://www.neowin.net/
D/GeckoFavicons( 1123): LoadFaviconTask finished for URL = http://www.neowin.net/ (2)
I/GeckoApp( 1123): Favicon successfully loaded for URL = http://www.neowin.net/
I/GeckoApp( 1123): Favicon is for current URL = http://www.neowin.net/
I/GeckoTab( 1123): Updated favicon for tab with id: 1
I/GeckoSoftwareLayerClient( 1123): Adjusting viewport
I/Gecko   ( 1123): AndroidGeckoEvent: 0x405f56a8 : 19
I/Gecko   ( 1123): WARNING: NS_ENSURE_TRUE(mState == STATE_TRANSFERRING) failed: file /builds/slave/m-cen-andrd-dbg/build/netwerk/base/src/nsSocketTransport2.cpp, line 1911
I/Gecko   ( 1123): WARNING: NS_ENSURE_TRUE(mState == STATE_TRANSFERRING) failed: file /builds/slave/m-cen-andrd-dbg/build/netwerk/base/src/nsSocketTransport2.cpp, line 1911
I/Gecko   ( 1123): AndroidBridge::GetDPI
I/Gecko   ( 1123): void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/GeckoApp( 1123): Got message: Content:StateChange
I/GeckoApp( 1123): State - 196612
I/Gecko   ( 1123): leaving void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/WindowManager(  110): Setting rotation to 1, animFlags=0
I/ActivityManager(  110): Config changed: { scale=1.0 imsi=302/220 loc=en_US touch=3 keys=1/1/2 nav=1/1 orien=2 layout=34 uiMode=17 seq=24}
I/GeckoApp( 1123): configuration changed
I/Gecko   ( 1123): void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/GeckoApp( 1123): Got message: Content:StateChange
I/GeckoApp( 1123): State - 131088
I/GeckoApp( 1123): Got a document stop
I/Gecko   ( 1123): leaving void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/Gecko   ( 1123): void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/GeckoSoftwareLayerClient( 1123): Screen-size changed to (800,480)
I/GeckoApp( 1123): Got message: Content:StateChange
I/Gecko   ( 1123): AndroidGeckoEvent: 0x40644cb8 : 8
I/GeckoSoftwareLayerClient( 1123): Adjusting viewport
I/GeckoApp( 1123): State - 131088
I/GeckoApp( 1123): Got a document stop
I/Gecko   ( 1123): leaving void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/Gecko   ( 1123): AndroidGeckoEvent: 0x40568280 : 19
I/Gecko   ( 1123): void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/Gecko   ( 1123): leaving void mozilla::AndroidBridge::HandleGeckoMessage(const nsAString_internal&, nsAString_internal&)
I/Gecko   ( 1123): nsWindow[0x46d36500]::Resize [0 0 2048 2048] (repaint 1)
I/Gecko   ( 1123): nsWindow: 0x46d36500 OnSizeChanged [2048 2048]
I/Gecko   ( 1123): nsWindow[0x46d36c80]::Resize [0 0 2048 2048] (repaint 0)
I/Gecko   ( 1123): nsWindow: 0x46d36c80 OnSizeChanged [2048 2048]
I/GeckoSoftwareLayerClient( 1123): Adjusting viewport
I/Gecko   ( 1123): AndroidGeckoEvent: 0x40643b48 : 19
I/WindowManager(  110): Setting rotation to 0, animFlags=0
I/ActivityManager(  110): Config changed: { scale=1.0 imsi=302/220 loc=en_US touch=3 keys=1/1/2 nav=1/1 orien=1 layout=34 uiMode=17 seq=25}
I/GeckoApp( 1123): configuration changed
I/GeckoSoftwareLayerClient( 1123): Screen-size changed to (480,800)
I/Gecko   ( 1123): AndroidGeckoEvent: 0x40607178 : 8
I/GeckoSoftwareLayerClient( 1123): Adjusting viewport
I/Gecko   ( 1123): AndroidGeckoEvent: 0x405b2b68 : 19
I/GeckoSoftwareLayerClient( 1123): Adjusting viewport
I/Gecko   ( 1123): AndroidGeckoEvent: 0x405ac868 : 19
I/WindowManager(  110): Setting rotation to 1, animFlags=0
I/ActivityManager(  110): Config changed: { scale=1.0 imsi=302/220 loc=en_US touch=3 keys=1/1/2 nav=1/1 orien=2 layout=34 uiMode=17 seq=26}
I/GeckoApp( 1123): configuration changed
I/GeckoSoftwareLayerClient( 1123): Screen-size changed to (800,480)
I/Gecko   ( 1123): AndroidGeckoEvent: 0x4054e650 : 8
I/GeckoSoftwareLayerClient( 1123): Adjusting viewport
I/ActivityManager(  110): Process org.mozilla.fennec (pid 1123) has died.
I/WindowManager(  110): WIN DEATH: Window{406f5648 SurfaceView paused=false}
I/WindowManager(  110): WIN DEATH: Window{406d09e0 org.mozilla.fennec/org.mozilla.fennec.App paused=false}

STR:

1. www.neowin.net
2. Rotate to landscape, rotate to portrait, rotate to landscape

--
Samsung Nexus S (Android 2.3.6)
20111216052315
http://hg.mozilla.org/mozilla-central/rev/80ac06ad280d

Comment 1

[Aaron Train [:aaronmt]]

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

Looks like bug 708307 could be the culprit.

Comment 2

[Chris Lord [:cwiiis]]

I'm trying to reproduce this, and can't on my device (HTC Flyer), but this could depend on resolution. Are these absolutely concrete reproduction instructions?

My stab-in-the-dark would be that possibly it's an OOM and we should force freeing textures before allocating new ones (currently, it flushes on draw), but that doesn't seem hugely likely...

Can we get a stack-trace?

Comment 3

[Aaron Train [:aaronmt]]

(In reply to Chris Lord [:cwiiis] from comment #2)
> I'm trying to reproduce this, and can't on my device (HTC Flyer), but this
> could depend on resolution. Are these absolutely concrete reproduction
> instructions?
> 
> My stab-in-the-dark would be that possibly it's an OOM and we should force
> freeing textures before allocating new ones (currently, it flushes on draw),
> but that doesn't seem hugely likely...
> 
> Can we get a stack-trace?

Not sure what I'm doing with the prebuilt binaries at https://wiki.mozilla.org/Mobile/Fennec/Android/GDB what to do with them. Would be easier for QA to get builds that dump Gecko stack traces from threads.

Comment 4

[Aaron Train [:aaronmt]]

Also reproducible easier on timecube.com  --- http://www.youtube.com/watch?v=75eJMsVDTn4

Comment 5

[Aaron Train [:aaronmt]]

Attachment: full console log (~360kb)

Comment 7

[Catalin Suciu [:csuciu]]

I was able to reproduce this using the initial steps on the latest fennec native:
Mozilla/5.0(Android;Linux armv7l;rv:11.0a1)Gecko/20111218;Firefox/11.0a1 Fennec/11.0a1.
Device: HTC Desire (Android 2.2)

Crash report: https://crash-stats.mozilla.com/report/index/bp-54dfc905-9c8b-4b04-b4be-9298c2111219

Comment 8

[Aaron Train [:aaronmt]]

bp-9dec3a30-6151-4069-9ebb-c48dd2111219

Comment 9

[Kartikaya Gupta (email:[e-mail])]

The two crash reports linked here seem to have different signatures. The second one (bp-9dec3a30-6151-4069-9ebb-c48dd2111219) is the same as bug 712655, and will be tracked there. I'll leave this bug for the adreno signature.

Comment 10

[Aaron Train [:aaronmt]]

A lot of info tossed around in channel today.

I can reproduce on my Samsung Galaxy SII (Android 2.3.4), Samsung Nexus S (4.0.3), and Samsung Nexus One (2.3.6).

I can't reproduce on my Motorola Droid Pro (Android 2.3.4).

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

Comment 11

[CoJaBo]

I think this might be the issue I'm having too-
Its not consistently reproducible, unless an animated GIF is playing.
It might be easier to test with, e.g., http://fukung.net/v/31665/2dc5bb8a8e8aba3724eec34fe3e48083.gif
Immediate crash on rotate, 100% of the time on my device (Droid X, CyanogenMod firmware).

Comment 12

[Hong SeGi]

(In reply to CoJaBo from comment #11)
> I think this might be the issue I'm having too-
> Its not consistently reproducible, unless an animated GIF is playing.
> It might be easier to test with, e.g.,
> http://fukung.net/v/31665/2dc5bb8a8e8aba3724eec34fe3e48083.gif
> Immediate crash on rotate, 100% of the time on my device (Droid X,
> CyanogenMod firmware).

Oh, I have same crashings in that page. Desire HD, Gingerbread 2.3.3 (HTC Official).

Comment 13

[Chris Lord [:cwiiis]]

Attachment: Fix one very likely and one unlikely cause of rotation crash

This fixes destroying the buffer while it's being used and fixes the temporarily increased memory requirement when changing buffers. I've also added a check that the buffer exists in the drawing function, for robustness.

Comment 14

[Chris Lord [:cwiiis]]

Argh, damn queues, seem to have missed half of the patch - reuploading in a second.

Comment 15

[Chris Lord [:cwiiis]]

Attachment: Fix one very likely and one unlikely cause of rotation crash

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 584043 [details] [diff] [review]
Fix one very likely and one unlikely cause of rotation crash

>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.

Nice catch! :) This could definitely explain all the crashes we've been seeing.

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>             Log.i(LOGTAG, "Screen-size changed to " + mScreenSize);
>             GeckoEvent event = new GeckoEvent(GeckoEvent.SIZE_CHANGED,
>-                                              mBufferSize.width, mBufferSize.height,
>+                                              mNewBufferSize.width, mNewBufferSize.height,
>                                               metrics.widthPixels, metrics.heightPixels);
>             GeckoAppShell.sendEventToGecko(event);

As I mentioned on IRC, we might want to defer sending this event so that Java and Gecko don't get out of sync with respect to the buffer size. Your idea of passing in the expected buffer size in beginDrawing() probably works best.

Rest looks good.

Comment 17

[Chris Lord [:cwiiis]]

Attachment: Fix two very likely and one unlikely cause of rotation crash

As mentioned, in the previous patch there's still a possible race where we send the SIZE_CHANGED event, but we deal with a draw event before the SIZE_CHANGED is processed - in this situation, Gecko will have an incorrectly sized buffer, which may cause a crash.

This version of the patch fixes (hopefully) that case by handing over control of the buffer size to Gecko via width/height parameters in beginDrawing.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 584051 [details] [diff] [review]
Fix two very likely and one unlikely cause of rotation crash

>diff --git a/mobile/android/base/gfx/GeckoSoftwareLayerClient.java b/mobile/android/base/gfx/GeckoSoftwareLayerClient.java
>-            // * 2 because it's a 16-bit buffer (so 2 bytes per pixel).
>-            mBuffer = GeckoAppShell.allocateDirectBuffer(mBufferSize.getArea() * 2);
>+            // We store this in mNewBufferSize so that mBufferSize always
>+            // represents the size of the current buffer. The buffer is refreshed
>+            // on the next draw.

This comment needs updating, rest looks good.

Comment 19

[Chris Lord [:cwiiis]]

Attachment: Fix some likely and unlikely rotation crash causes

This rectifies that comment (by removing it, I don't think the situation warrants commenting on), and also adds one extra fix in GeckoSoftwareLayerClient.getBitmap(), which was also not respecting the data lock.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 584076 [details] [diff] [review]
Fix some likely and unlikely rotation crash causes

Good catch :)

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/2f6873cba874

Comment 22

[Aaron Train [:aaronmt]]

The above checked-in patch does not fix the crash I reported. Suggest keeping this open until the issue I filed is resolved.

Still reproducible on Samsung Galaxy SII (Android 2.3.4), and Samsung Nexus S (Android 4.0.3) and Samsung Nexus One (Android 2.3.4)

--
20111225031006
http://hg.mozilla.org/mozilla-central/rev/798b00a6fe29

Comment 23

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 584076 [details] [diff] [review]
Fix some likely and unlikely rotation crash causes

We want these fixes on aurora

Comment 24

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Seeing this crash with Nightly on Nexus S with Android 4.0.3 and crash signature libxul.so@0xae938c
https://crash-stats.mozilla.com/report/index/bbbdb6c6-03c0-4f92-a5af-8044b2111226

Comment 25

[Alex Keybl [:akeybl]]

Comment on attachment 584076 [details] [diff] [review]
Fix some likely and unlikely rotation crash causes

[Triage Comment]
Approved for Aurora.

Comment 26

[Mark Finkle (:mfinkle) (use needinfo?)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/25ca24acbf49

With the complexity of tracking in m-c and m-a, I think we should file a new bug instead of leaving this one open.

Comment 27

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Mark Finkle (:mfinkle) from comment #26)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/25ca24acbf49
> 
> With the complexity of tracking in m-c and m-a, I think we should file a new
> bug instead of leaving this one open.

To be clear, the patches on this bug have landed on m-c and m-a. If there is any additional work, a new bug should be opened because I am closing this one.

Comment 28

[Mark Finkle (:mfinkle) (use needinfo?)]

Could be that bug 713299 covers the rest of the needed fixes

Comment 29

[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)

Verified on site from bug, and others:
- www.neowin.net
- timecube.com
- http://fukung.net/v/31665/2dc5bb8a8e8aba3724eec34fe3e48083.gif

Comment 30

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

this chunk of the patch that landed on mozilla-central: 
https://hg.mozilla.org/mozilla-central/diff/2f6873cba874/mobile/android/base/gfx/GeckoSoftwareLayerClient.java#l1.41

didn't make it to mozilla-aurora:
https://hg.mozilla.org/releases/mozilla-aurora/diff/25ca24acbf49/mobile/android/base/gfx/GeckoSoftwareLayerClient.java

Comment 31

[Chris Lord [:cwiiis]]

(In reply to Brad Lassey [:blassey] from comment #30)
> this chunk of the patch that landed on mozilla-central: 
> https://hg.mozilla.org/mozilla-central/diff/2f6873cba874/mobile/android/base/
> gfx/GeckoSoftwareLayerClient.java#l1.41
> 
> didn't make it to mozilla-aurora:
> https://hg.mozilla.org/releases/mozilla-aurora/diff/25ca24acbf49/mobile/
> android/base/gfx/GeckoSoftwareLayerClient.java

I don't think we saw any crashes that that fixed, but it's theoretically possible (if getBitmap is called immediately after the keyboard pops up/down or the screen rotates) - we probably want to land it too...

What do I need to do to get this in? And is there a reason it wasn't landed in the first place?

Comment 32

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

(In reply to Chris Lord [:cwiiis] from comment #31)
> What do I need to do to get this in? And is there a reason it wasn't landed
> in the first place?

I landed the missing chunk:
https://hg.mozilla.org/releases/mozilla-aurora/rev/28f81ac0a1fa