Closed Bug 859031 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox for Android :: General, defect)

22 Branch
ARM
Android
defect
Not set
critical

Tracking

()

RESOLVED FIXED
Firefox 23
Tracking Status
firefox21 --- unaffected
firefox22 --- affected
firefox23 --- fixed

People

(Reporter: scoobidiver, Assigned: cwiiis)

References

Details

(Keywords: crash, regression, Whiteboard: [native-crash])

Crash Data

Attachments

(1 file)

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
That line is the prefs return for the dynamic-toolbar pref - I guess mAboutHome can be null here?
(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).
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Attachment #735765 - Flags: review?(lucasr.at.mozilla)
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.
Attachment #735765 - Flags: review?(lucasr.at.mozilla) → review+
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
(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.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
https://hg.mozilla.org/mozilla-central/rev/0a4e209fddea
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
(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/
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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 :)
You need to log in before you can comment on or make changes to this bug.