859031 - java.lang.NullPointerException: at org.mozilla.gecko.BrowserApp$8$1.run(BrowserApp.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

There are two crashes, one in 22.0a2 and another one in 23.0a1, bp-930e1aa9-3f92-43f1-a091-1b6fd2130407.

java.lang.NullPointerException
	at org.mozilla.gecko.BrowserApp$8$1.run(BrowserApp.java:522)
	at android.os.Handler.handleCallback(Handler.java:587)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:143)
	at android.app.ActivityThread.main(ActivityThread.java:4717)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:521)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:860)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:618)
	at dalvik.system.NativeStart.main(Native Method)

More reports at:
https://crash-stats.mozilla.com/report/list?signature=java.lang.NullPointerException%3A+at+org.mozilla.gecko.BrowserApp%248%241.run%28BrowserApp.java%29

Comment 1

[Chris Lord [:cwiiis]]

That line is the prefs return for the dynamic-toolbar pref - I guess mAboutHome can be null here?

Comment 2

[Lucas Rocha (:lucasr)]

(In reply to Chris Lord [:cwiiis] from comment #1)
> That line is the prefs return for the dynamic-toolbar pref - I guess
> mAboutHome can be null here?

Yep, mAboutHomeContent should not be assumed non-null. It might be null if the user didn't open about:home before (e.g. when you open Firefox from a Twitter app link).

Comment 3

[Chris Lord [:cwiiis]]

Attachment: Fix possible NPE in BrowserApp dynamic toolbar prefs getter

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 735765 [details] [diff] [review]
Fix possible NPE in BrowserApp dynamic toolbar prefs getter

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

::: mobile/android/base/BrowserApp.java
@@ +518,5 @@
>                              setToolbarMargin(0);
>                          } else {
>                              // Immediately show the toolbar when disabling the dynamic
>                              // toolbar.
> +                            if (mAboutHomeContent != null) {

nit: we don't usually add {}'s for one-line if's.

Comment 5

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

(In reply to Lucas Rocha (:lucasr) from comment #4)
> nit: we don't usually add {}'s for one-line if's.

We do in some places. I'd like to see it happen everywhere.

Comment 6

[Lucas Rocha (:lucasr)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (In reply to Lucas Rocha (:lucasr) from comment #4)
> > nit: we don't usually add {}'s for one-line if's.
> 
> We do in some places. I'd like to see it happen everywhere.

I prefer {}'s everywhere too. I'm just echoing the comments I almost invariably get from mfinkle (and blassey?) when they review my patches :-)

Last time I checked (long-ish time ago), some people felt strongly against requiring {}'s on one-line if's. Maybe it's time to re-evaluate the decision? I'm all for it.

Comment 7

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

(In reply to Lucas Rocha (:lucasr) from comment #6)
> I prefer {}'s everywhere too. I'm just echoing the comments I almost
> invariably get from mfinkle (and blassey?) when they review my patches :-)
> 
> Last time I checked (long-ish time ago), some people felt strongly against
> requiring {}'s on one-line if's. Maybe it's time to re-evaluate the
> decision? I'm all for it.

I don't think we're going to get global consensus on this issue. But you're free to put it in code you write (as long as it's not reviewed by mfinkle and blassey?) and require it in code you review.

Comment 8

[Chris Lord [:cwiiis]]

Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4e209fddea

Comment 9

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

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> (In reply to Lucas Rocha (:lucasr) from comment #6)
> > I prefer {}'s everywhere too. I'm just echoing the comments I almost
> > invariably get from mfinkle (and blassey?) when they review my patches :-)
> > 
> > Last time I checked (long-ish time ago), some people felt strongly against
> > requiring {}'s on one-line if's. Maybe it's time to re-evaluate the
> > decision? I'm all for it.
> 
> I don't think we're going to get global consensus on this issue. But you're
> free to put it in code you write (as long as it's not reviewed by mfinkle
> and blassey?) and require it in code you review.

I have been letting single-line {} go in my reviews lately. I have other style-nits I care much more about. DEATH TO 80 CHAR LINES!

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/0a4e209fddea

Comment 11

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

(In reply to Mark Finkle (:mfinkle) from comment #9)
> I have been letting single-line {} go in my reviews lately. I have other
> style-nits I care much more about. DEATH TO 80 CHAR LINES!

\o/

Comment 12

[Chris Lord [:cwiiis]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #11)
> (In reply to Mark Finkle (:mfinkle) from comment #9)
> > I have been letting single-line {} go in my reviews lately. I have other
> > style-nits I care much more about. DEATH TO 80 CHAR LINES!
> 
> \o/

Disagree on this, but this is something that clearly isn't followed anywhere in the Mozilla codebase :)