Closed Bug 1096958 Opened 9 years ago Closed 8 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Version:
36 Branch
Platform:
All
Android
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

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

Status:
RESOLVED FIXED
Milestone:
Firefox 38
Tracking Flags:
Tracking Status
firefox35 --- wontfix
firefox36 --- fixed
firefox37 --- fixed
firefox38 --- affected
fennec 36+ ---

People

(Reporter: aaronmt, Assigned: mcomella)

References

Details

(Keywords: crash)

Crash Data

Attachments

(2 files, 1 obsolete file)

Move TilesRecorder instance into TopSitesPanel
5.17 KB, patch
bnicholson
: review+
Sylvestre
: approval-mozilla-aurora+
Sylvestre
: approval-mozilla-beta+
Details | Diff | Splinter Review
MozReview Request: Bug 1096958 - Part 2: Null mGrid's OnItemClickListener when destroying the view. r=liuche
39 bytes, text/x-review-board-request
liuche
: review+
Details 
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)
Dyslexio, sorry
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
Closed: 8 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+
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 → ---
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)
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
status-firefox36: affected → fixed
status-firefox37: affected → fixed
https://hg.mozilla.org/mozilla-central/rev/44ef3a85e840
Status: ASSIGNED → RESOLVED
Closed: 8 years ago → 8 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+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.