752172 - java.lang.NullPointerException: at org.mozilla.gecko.SiteIdentityPopup.show(SiteIdentityPopup.java)

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Scoobidiver (away)]

There are two crashes from two different users, the first one in 15.0a1/20120504040218 and the second one in 15.0a1/20120504122939: bp-79fa4f9f-b145-4380-88bd-1f1ee2120505.

java.lang.NullPointerException
	at org.mozilla.gecko.SiteIdentityPopup.show(SiteIdentityPopup.java:72)
	at org.mozilla.gecko.BrowserToolbar$4.onClick(BrowserToolbar.java:184)
	at android.view.View.performClick(View.java:3511)
	at android.view.View$PerformClick.run(View.java:14105)
	at android.os.Handler.handleCallback(Handler.java:605)
	at android.os.Handler.dispatchMessage(Handler.java:92)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4424)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:511)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:787)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:554)
	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.SiteIdentityPopup.show%28SiteIdentityPopup.java%29

Comment 1

[Aaron Train [:aaronmt]]

Introduced with bug 695204.

Comment 2

[:Margaret Leibovic]

Attachment: Turn SiteIdentityPopup into a singleton class

Instead of needing to worry about whether or not mSiteIdentityPopup has been initialized, I decided it would be nicer to just get rid of it, and turn SiteIdentityPopup into a singleton class.

I really wanted to totally decouple it from GeckoApp, but unfortunately we still need to refer to GeckoApp.mAppContext, and we also already refer to GeckoApp.mBrowserToolbar in there.

If you like this, I think we could do similar things with DoorHangerPopup and FormAssistPopup. I also noticed FormAssistPopup was declared in gecko_app.xml, and I think that should just go in its own xml like the other popups. I can do this cleanup/consistency work in another bug, though.

Comment 3

[:Margaret Leibovic]

Comment on attachment 621758 [details] [diff] [review]
Turn SiteIdentityPopup into a singleton class

Switching the review request to spread around review load a little more :)

Comment 4

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4940a331b147

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4940a331b147

Comment 6

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

(In reply to Margaret Leibovic [:margaret] from comment #2)

> If you like this, I think we could do similar things with DoorHangerPopup
> and FormAssistPopup. I also noticed FormAssistPopup was declared in
> gecko_app.xml, and I think that should just go in its own xml like the other
> popups. I can do this cleanup/consistency work in another bug, though.

We intend to start using different activities for the Browser and WebApps, so we need to make sure these classes can be used by multiple activities. Creating singletons might not allow for that very easily.

Comment 7

[Scoobidiver (away)]

There is one crash in 15.0a1/20120512: bp-3d300374-a0d8-47e9-8e39-b5ca22120513.

Comment 8

[:Margaret Leibovic]

Attachment: (Part 2) Protect against null selected tab

Looks like I read the crash stack incorrectly last time. My last patch fixed a potential crash, but not this one.

Looking at the Tabs code, it seems like Tabs.getInstance.getSelectedTab() should really only return null if selectTab() has never been called yet. However, we only make the lock icon visible if there is a valid selected tab, since we use that tab's data to decide to show the lock. Looking at the lock code, we don't actually hide it, but rather set it to be transparent, so I'd guess this crash is just happening if a user accidentally taps on that transparent lock before a tab is selected. If that's the case, bailing if the selected tab is null shouldn't be noticeable to the user. However, I think it would also be worth looking into hiding the lock instead of making it transparent, so that we can never trigger this click listener in that case.

Comment 9

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

Comment on attachment 623689 [details] [diff] [review]
(Part 2) Protect against null selected tab

Nice explanation

Comment 10

[:Margaret Leibovic]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d0f6caf941d1

Comment 11

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d0f6caf941d1