crash in java.lang.NullPointerException: at org.mozilla.gecko.home.TopSitesPanel.access$N(TopSitesPanel.java)

RESOLVED FIXED in Firefox 36

Status

()

defect
--
critical
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: aaronmt, Assigned: mcomella)

Tracking

({crash})

36 Branch
Firefox 38
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox35 wontfix, firefox36 fixed, firefox37 fixed, firefox38 affected, fennec36+)

Details

(crash signature)

Attachments

(2 attachments, 1 obsolete attachment)

Reporter

Description

5 years ago
This bug was filed from the Socorro interface and is 
report bp-6670856a-9016-4377-8bfd-af2d52141110.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.home.TopSitesPanel.access$300(TopSitesPanel.java:68)
	at org.mozilla.gecko.home.TopSitesPanel$3.onItemClick(TopSitesPanel.java:238)
	at android.widget.AdapterView.performItemClick(AdapterView.java:299)
	at android.widget.AbsListView.performItemClick(AbsListView.java:1113)
	at android.widget.AbsListView$PerformClick.run(AbsListView.java:2911)
	at android.widget.AbsListView$3.run(AbsListView.java:3645)
	at android.widget.AbsListView.onDetachedFromWindow(AbsListView.java:2777)
	at android.view.View.dispatchDetachedFromWindow(View.java:12620)
	at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2587)
	at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2585)
	at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2585)
	at android.view.ViewGroup.removeViewInternal(ViewGroup.java:3845)
	at android.view.ViewGroup.removeViewInternal(ViewGroup.java:3818)
	at android.view.ViewGroup.removeView(ViewGroup.java:3750)
	at android.support.v4.view.ViewPager.removeView(ViewPager.java:1326)
	at android.support.v4.app.FragmentManagerImpl.moveToState(FragmentManager.java:1040)
	at android.support.v4.app.FragmentManagerImpl.removeFragment(FragmentManager.java:1218)
	at android.support.v4.app.BackStackRecord.run(BackStackRecord.java:652)
	at android.support.v4.app.FragmentManagerImpl.execPendingActions(FragmentManager.java:1484)
	at android.support.v4.app.FragmentManagerImpl.executePendingTransactions(FragmentManager.java:482)
	at android.support.v4.app.FragmentStatePagerAdapter.finishUpdate(FragmentStatePagerAdapter.java:163)
	at android.support.v4.view.ViewPager.setAdapter(ViewPager.java:420)
	at org.mozilla.gecko.home.HomePager.unload(HomePager.java:263)
	at org.mozilla.gecko.BrowserApp.hideHomePager(BrowserApp.java:2308)
	at org.mozilla.gecko.BrowserApp.hideHomePager(BrowserApp.java:2288)
	at org.mozilla.gecko.BrowserApp.onTabChanged(BrowserApp.java:291)
	at org.mozilla.gecko.Tabs$5.run(Tabs.java:623)
	at android.os.Handler.handleCallback(Handler.java:733)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:136)
	at android.app.ActivityThread.main(ActivityThread.java:5034)
	at java.lang.reflect.Method.invokeNative(Native Method)
	at java.lang.reflect.Method.invoke(Method.java:515)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:1270)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1086)
	at dalvik.system.NativeStart.main(Native Method)

Updated

5 years ago
Blocks: 1096953

Comment 1

5 years ago
Dyslexio, sorry
No longer blocks: 1096953
Reporter

Comment 2

5 years ago
Seems to be raising on 35
tracking-fennec: --- → ?
Assignee: nobody → michael.l.comella
tracking-fennec: ? → 35+
Ping?
Flags: needinfo?(michael.l.comella)
Will start working on this today - was heads down in new tablet stuff.
Flags: needinfo?(michael.l.comella)
at org.mozilla.gecko.home.TopSitesPanel.access$300(TopSitesPanel.java:70)
at org.mozilla.gecko.home.TopSitesPanel$3.onItemClick(TopSitesPanel.java:242)

  mTilesRecorder.recordAction(tab, TilesRecorder.ACTION_CLICK, position, getTilesSnapshot(), localeTag); [1]

mTilesRecorder is a private member of TopSitesPanel, which explains the access$300. NullPointerException, presumably because mTilesRecorder is null.

[1]: https://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/home/TopSitesPanel.java?rev=790d52784837#242
Status: NEW → ASSIGNED
I'm not sure how this ever worked. According to [1]:

onAttach()
    Called when the fragment has been associated with the activity (the Activity is passed in here).
...
onActivityCreated()
    Called when the activity's onCreate() method has returned.

---

At the initialization [1], TopSitesPanel.onAttach:

  mTilesRecorder = ((BrowserTilesRecorderProvider) activity).getTilesRecorder();

BrowsesrApp.getTilesRecorder just returns BrowserApp.mTilesRecorder, which is initialized in BrowserApp.onCreate [2].

So we should either:
  1) Initialize TopSitesPanel.mTilesRecorder in onActivityCreated
  2) BrowserApp.mTilesRecorder is unused in BrowserApp and the constructor does nothing (i.e. is not time-dependent) so we can lazily instantiate it (a little less future-proof though).
  3) Move the TilesRecorder construction to TopSitesPanel because it's the only consumer anyway.

I like 3 (then 1), unless anyone has an idea of why this is done in BrowserApp.onCreate anyway.

[1]: https://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/home/TopSitesPanel.java?rev=790d52784837#143
[2]: https://mxr.mozilla.org/mozilla-beta/source/mobile/android/base/BrowserApp.java?rev=55bd32c43abd#667
(In reply to Michael Comella (:mcomella) from comment #6)
> I like 3 (then 1), unless anyone has an idea of why this is done in
> BrowserApp.onCreate anyway.

<bnicholson> mcomella: TilesRecorder used to be a lot heavier than it is now, and TopSitesPanel didn't have everything it needed to construct one
<bnicholson> i'd be OK with just creating/destroying it directly in TilesRecorder now since we probably aren't going to change it

Then 3 it is.
Note that I am not able to repro so my testing did not cover the crashing use
case.
Attachment #8553409 - Flags: review?(bnicholson)
Attachment #8553409 - Flags: review?(bnicholson) → review+
Comment on attachment 8553409 [details] [diff] [review]
Move TilesRecorder instance into TopSitesPanel

Finkle, this is tracking 35 - I assume that means we want this in release too, right?

Approval Request Comment
[Feature/regressing bug #]: Unknown

[User impact if declined]:
  Users crash.

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]: 
  Low - we're changing where a class is initialized such that this initialization is guaranteed to take place before it is used. Since this class holds and relies on no state, it doesn't matter when other things get initialized in relation to when it gets initialized (as long as it's initialized before it is used - since we're doing that, low risk).

[String/UUID change made/needed]: None
Flags: needinfo?(mark.finkle)
Attachment #8553409 - Flags: approval-mozilla-release?
Attachment #8553409 - Flags: approval-mozilla-beta?
Attachment #8553409 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/61d10b543fc8
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
Attachment #8553409 - Flags: approval-mozilla-beta?
Attachment #8553409 - Flags: approval-mozilla-beta+
Attachment #8553409 - Flags: approval-mozilla-aurora?
Attachment #8553409 - Flags: approval-mozilla-aurora+
Reporter

Comment 13

4 years ago
This is still reproducible on trunk, double tap any top-site and it instant-crashes for me (02/02 Trunk Nightly).

E/GeckoCrashHandler(12490): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoCrashHandler(12490): java.lang.NullPointerException
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.home.TopSitesPanel.access$300(TopSitesPanel.java:69)
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.home.TopSitesPanel$3.onItemClick(TopSitesPanel.java:232)
E/GeckoCrashHandler(12490): 	at android.widget.AdapterView.performItemClick(AdapterView.java:298)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Reporter

Comment 14

4 years ago
Stack on trunk (02/02)

E/GeckoCrashHandler(12490): java.lang.NullPointerException
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.home.TopSitesPanel.access$300(TopSitesPanel.java:69)
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.home.TopSitesPanel$3.onItemClick(TopSitesPanel.java:232)
E/GeckoCrashHandler(12490): 	at android.widget.AdapterView.performItemClick(AdapterView.java:298)
E/GeckoCrashHandler(12490): 	at android.widget.AbsListView.performItemClick(AbsListView.java:1104)
E/GeckoCrashHandler(12490): 	at android.widget.AbsListView$PerformClick.run(AbsListView.java:2792)
E/GeckoCrashHandler(12490): 	at android.widget.AbsListView$1.run(AbsListView.java:3468)
E/GeckoCrashHandler(12490): 	at android.widget.AbsListView.onDetachedFromWindow(AbsListView.java:2674)
E/GeckoCrashHandler(12490): 	at android.view.View.dispatchDetachedFromWindow(View.java:12342)
E/GeckoCrashHandler(12490): 	at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2570)
E/GeckoCrashHandler(12490): 	at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2568)
E/GeckoCrashHandler(12490): 	at android.view.ViewGroup.dispatchDetachedFromWindow(ViewGroup.java:2568)
E/GeckoCrashHandler(12490): 	at android.view.ViewGroup.removeViewInternal(ViewGroup.java:3788)
E/GeckoCrashHandler(12490): 	at android.view.ViewGroup.removeViewInternal(ViewGroup.java:3761)
E/GeckoCrashHandler(12490): 	at android.view.ViewGroup.removeView(ViewGroup.java:3693)
E/GeckoCrashHandler(12490): 	at android.support.v4.view.ViewPager.removeView(Unknown Source)
E/GeckoCrashHandler(12490): 	at android.support.v4.app.FragmentManagerImpl.moveToState(Unknown Source)
E/GeckoCrashHandler(12490): 	at android.support.v4.app.FragmentManagerImpl.removeFragment(Unknown Source)
E/GeckoCrashHandler(12490): 	at android.support.v4.app.BackStackRecord.run(Unknown Source)
E/GeckoCrashHandler(12490): 	at android.support.v4.app.FragmentManagerImpl.execPendingActions(Unknown Source)
E/GeckoCrashHandler(12490): 	at android.support.v4.app.FragmentManagerImpl.executePendingTransactions(Unknown Source)
E/GeckoCrashHandler(12490): 	at android.support.v4.app.FragmentStatePagerAdapter.finishUpdate$52bc874c(Unknown Source)
E/GeckoCrashHandler(12490): 	at android.support.v4.view.ViewPager.setAdapter(Unknown Source)
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.home.HomePager.unload(HomePager.java:258)
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.BrowserApp.hideHomePager(BrowserApp.java:2536)
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.BrowserApp.hideHomePager(BrowserApp.java:2516)
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.BrowserApp.onTabChanged(BrowserApp.java:452)
E/GeckoCrashHandler(12490): 	at org.mozilla.gecko.Tabs$4.run(Tabs.java:618)
E/GeckoCrashHandler(12490): 	at android.os.Handler.handleCallback(Handler.java:730)
E/GeckoCrashHandler(12490): 	at android.os.Handler.dispatchMessage(Handler.java:92)
E/GeckoCrashHandler(12490): 	at android.os.Looper.loop(Looper.java:213)
E/GeckoCrashHandler(12490): 	at android.app.ActivityThread.main(ActivityThread.java:5225)
E/GeckoCrashHandler(12490): 	at java.lang.reflect.Method.invokeNative(Native Method)
E/GeckoCrashHandler(12490): 	at java.lang.reflect.Method.invoke(Method.java:525)
E/GeckoCrashHandler(12490): 	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:741)
E/GeckoCrashHandler(12490): 	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:557)
E/GeckoCrashHandler(12490): 	at dalvik.system.NativeStart.main(Native Method)
STR discussion via IRC:

<mcomella> AaronMT: Is there an STR?
<AaronMT> mcomella: yeah I'm double tapping on a top-site quickly
<mcomella> AaronMT: Any locale settings?
<mcomella> AaronMT: Or is this a new profile?
<AaronMT> mcomella: doesn't matter
<mcomella> AaronMT: All devices?
<AaronMT> dunno
<mcomella> AaronMT: Only on startup?
<mcomella> (the tapping)
<AaronMT> nope
<mcomella> AaronMT: What device are you using?
<AaronMT> xperia tablet right now
<mcomella> AaronMT: so any time you open top sites (e.g. new tab, or click toolbar), you can repro?
<AaronMT> mcomella: no only double tapping a thumbnail on top-sites
<mcomella> AaronMT: Right, so from either of those? about:home as a new tab or clicking the toolbar
<mcomella> And no matter what you were doing in the browser before
<mcomella> I can't repro on my N7 :\
<AaronMT> mcomella: same on my nexus 6, maybe a race, need slower device
Stack trace from an unoptimized build:

E/GeckoCrashHandler(  595): java.lang.NullPointerException
E/GeckoCrashHandler(  595):     at org.mozilla.gecko.home.TopSitesPanel.getTilesSnapshot(TopSitesPanel.java:283)
E/GeckoCrashHandler(  595):     at org.mozilla.gecko.home.TopSitesPanel.access$300(TopSitesPanel.java:69)
E/GeckoCrashHandler(  595):     at org.mozilla.gecko.home.TopSitesPanel$3.onItemClick(TopSitesPanel.java:232)

---

mGrid is null on the second click:

  final int count = mGrid.getCount(); [1]

I'm guessing we destroy the fragment after the first click and then mGrid is null on the second click. 

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TopSitesPanel.java?rev=61d10b543fc8#283
Oh, look - precedent [1]!

307         // Discard any additional item clicks on the list
308         // as the panel is getting destroyed (see bug 930160).
309         mList.setOnItemClickListener(null);

But no change to mGrid it seems.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/home/TopSitesPanel.java?rev=61d10b543fc8#307
/r/3259 - Bug 1096958 - Part 2: Null mGrid's OnItemClickListener when destroying the view. r=liuche

Pull down this commit:

hg pull review -r 9086ddc50a321f63503dd131dd2c78e73732d200
Attachment #8558064 - Flags: review?(liuche)
Attachment #8553409 - Flags: approval-mozilla-release?
Note: (via IRC) Aaron cannot repro with the build using the patch in comment 18.
https://reviewboard.mozilla.org/r/3259/#review2681

Ok, that looks fine to me for a quick fix.
Attachment #8558064 - Flags: review?(liuche) → review+
Comment on attachment 8558064 [details]
MozReview Request: bz://1096958/mcomella

There is precedent from bug 930160 so this is likely fix the issue and not cause regressions.

Approval Request Comment
[Feature/regressing bug #]: Unknown, but not fixed by part 1.

[User impact if declined]:
  Users crash.

[Describe test coverage new/current, TreeHerder]: None

[Risks and why]:
  Low - we null an OnItemClickListener when destroying the view - pretty harmless!

[String/UUID change made/needed]: None
Attachment #8558064 - Flags: approval-mozilla-beta?
Attachment #8558064 - Flags: approval-mozilla-aurora?
wfm, can we get a tracking 36?
tracking-fennec: 35+ → ?
Attachment #8558064 - Flags: approval-mozilla-beta?
Attachment #8558064 - Flags: approval-mozilla-beta+
Attachment #8558064 - Flags: approval-mozilla-aurora?
Attachment #8558064 - Flags: approval-mozilla-aurora+
tracking-fennec: ? → 36+
Flags: needinfo?(mark.finkle)
https://hg.mozilla.org/releases/mozilla-beta/rev/c860bf9bc4a8

(Sylvestre asked me to do the uplift 'cos RyanVM was afk.)
Status: REOPENED → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/44ef3a85e840
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
Comment on attachment 8558064 [details]
MozReview Request: bz://1096958/mcomella

https://reviewboard.mozilla.org/r/3257/#review3607

Ship It!
Attachment #8558064 - Flags: review+
Attachment #8558064 - Attachment is obsolete: true
Attachment #8618592 - Flags: review+
You need to log in before you can comment on or make changes to this bug.