988800 - crash in java.lang.NullPointerException: at org.mozilla.gecko.gfx.ScrollbarLayer.draw(ScrollbarLayer.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[u421692]

This bug was filed from the Socorro interface and is 
report bp-363ddc09-90a5-474a-98a8-21b5c2140326.
=============================================================

Environment:
Device: Alcatel One Touch 8008D (Android 4.1.2)
Build: Nightly 30.0a1 (2014-03-26)

Steps to reproduce:
Have some apps in the background, have also Nightly 30.0a1 (2014-03-26) in the background
From task manager, change through apps; bring FF Nightly from background to front.

Actual result:
Nightly crashes.

Note: It seems that is a new crash, from what I can see in the crash-stats report: https://crash-stats.mozilla.com/report/list?product=FennecAndroid&query_type=contains&range_unit=days&process_type=any&hang_type=any&signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.gfx.ScrollbarLayer.draw%28ScrollbarLayer.java%29&date=2014-03-27+09%3A00%3A00&range_value=28#tab-reports . There are only two other crashes reported, besides the crashes from the device I've tested on: Scribe5HD.

Good build: Nightly 30.0a1 (2014-03-25)
Bad build: Nightly 30.0a1 (2014-03-26)

pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=5c0673441fc8&tochange=140ac04d7454

Comment 1

[Aaron Train [:aaronmt]]

java.lang.NullPointerException
	at org.mozilla.gecko.gfx.ScrollbarLayer.draw(ScrollbarLayer.java:203)
	at org.mozilla.gecko.gfx.LayerRenderer$Frame.drawForeground(LayerRenderer.java:631)
	at dalvik.system.NativeStart.run(Native Method)
	at dalvik.system.NativeStart.run(Native Method)

Comment 2

[u421692]

1395802177 - good
1395804220 - bad

http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=db3cd3239562&tochange=9a1c527eeafc still can't narrow what bug caused this regression

Comment 3

[u421692]

1395763242 - good
1395763360 - bad
http://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=7455653685a1&tochange=23f6bebf6b2e

Bug 987281 caused this regression

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Kats, can someone from gfx take a quick look? This is a crash that we were ignoring before, until bug 987281 made exceptions crash if not handled. We can always restore the previous behavior (ignore exceptions), but I want to see if this crash indicates a real bug or not before we do that.

Comment 5

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

Hm.. so this NPE would happen if the coordBuffer is null in the RenderContext, which should never really be happening. The most likely condition under which this would happen given the STR is that we are invoking the drawForeground on a LayerRenderer that has already been destroyed. That is, we have a stale handle in the JNI code and are using that when compositing.

Comment 6

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

So yeah I guess this is a bug in the fennec graphics code. We might want to just catch and ignore the exception but I'll take a quick look to see if there's an easy fix.

Comment 7

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

Yeah I think it's probably best to just ignore the exception. I think this is a race condition where the view is destroyed and the java-side objects are killed while we're in the middle of doing a composition on the compositor thread. So the LayerRenderer.destroy() function is called, and then the compositor calls the drawForeground stuff to draw the scrollbars, and the coordBuffer has already been nulled out. In this case just ignoring the exception and failing safely should be fine; the compositor will get paused anyway once the composite finishes and it doesn't matter if it can't draw the scrollbars on that frame.

Jim, is there a flag to make the AutoLocalJNIFrame ignore the exception, or do I have to manually stick in a try/catch block somewhere?

Comment 8

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

I think we wanted to expose the crashes and then address on a case-by-case basis.

Comment 9

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

Attachment: Patch

Here's a patch to catch the condition and ignore it properly. I was able to compile this but since I couldn't package the build (bug 991017) I wasn't able to test it.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=bd046ded6981

Comment 10

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

Comment on attachment 8400545 [details] [diff] [review]
Patch

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

Try is green

Comment 11

[Chris Lord [:cwiiis]]

Comment on attachment 8400545 [details] [diff] [review]
Patch

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

LGTM.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5cd9ae69c171

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5cd9ae69c171

Comment 14

[u421692]

Verified as fixed on Nightly 31.0a1 (2014-04-07)