930162 - crash in java.lang.NullPointerException: at org.mozilla.gecko.home.TabMenuStrip.onPageScrolled(TabMenuStrip.java)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

This bug was filed from the Socorro interface and is 
report bp-d71598d5-6e58-4355-b5a9-2404d2131022.
=============================================================

java.lang.NullPointerException
	at org.mozilla.gecko.home.TabMenuStrip.onPageScrolled(TabMenuStrip.java:107)
	at org.mozilla.gecko.home.HomePager$1.onPageScrolled(HomePager.java:114)
	at android.support.v4.view.ViewPager.onPageScrolled(ViewPager.java:1647)

Comment 1

[Sriram Ramasubramanian [:sriram]]

Attachment: Patch

There are no steps to reproduce. Hence don't know why any of these views could be null.
Just a preventive fix here.

Comment 2

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

Comment on attachment 821902 [details] [diff] [review]
Patch

Should this check be before the call to setScrollingData ? Just asking.

Comment 3

[Sriram Ramasubramanian [:sriram]]

(In reply to Mark Finkle (:mfinkle) from comment #2)
> Comment on attachment 821902 [details] [diff] [review]
> Patch
> 
> Should this check be before the call to setScrollingData ? Just asking.

setScrollingData() sets the values for "fromTab" and "toTab", which is null for some reason.
We are cross checking what setScrollingData() did.

Comment 4

[Lucas Rocha (:lucasr)]

Comment on attachment 821902 [details] [diff] [review]
Patch

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

Again, not keen on doing these things without understand the underlying issue. But this check looks more harmless than the ones on the other bugs.

::: mobile/android/base/home/TabMenuStrip.java
@@ +103,5 @@
>          }
>  
>          setScrollingData(position, positionOffset);
>  
> +        if (fromTab == null || toTab == null) {

Please file a follow-up bug to rename:
fromTab -> mFromTab
toTab -> mToTab
progress -> mProgress

Comment 5

[Sriram Ramasubramanian [:sriram]]

https://hg.mozilla.org/integration/fx-team/rev/fefaca8e866e

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 7

[Aaron Train [:aaronmt]]

155 reports I see, should get this uplifted maybe. Don't have steps to reproduce though to verify this.

Comment 8

[Kevin Brosnan [Ex-Mozilla]]

Still crashed on the 2013110600 build I think the patch should have made it into that build as it was merged to m-c at 5 PM Pacific and the Nightly build was created in the early AM on the 6th. The crash is https://crash-stats.mozilla.com/report/index/8f54b9f2-ba57-445a-9409-aec6e2131106

Signature shifted slightly from Aaron's crash ID.

java.lang.NullPointerException
	at org.mozilla.gecko.home.TabMenuStrip.onPageScrolled(TabMenuStrip.java:107)
	at org.mozilla.gecko.home.HomePager$1.onPageScrolled(HomePager.java:114)
	at android.support.v4.view.ViewPager.onPageScrolled(ViewPager.java:1647)
	at android.support.v4.view.ViewPager.pageScrolled(ViewPager.java:1585)
	at android.support.v4.view.ViewPager.computeScroll(ViewPager.java:1550)
	at android.view.ViewGroup.drawChild(ViewGroup.java:2729)
	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2489)
	at android.view.View.getDisplayList(View.java:10444)
	at android.view.ViewGroup.drawChild(ViewGroup.java:2850)
	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2489)
	at android.view.View.getDisplayList(View.java:10444)
	at android.view.ViewGroup.drawChild(ViewGroup.java:2850)
	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2489)
	at android.view.View.draw(View.java:11010)
	at android.view.View.getDisplayList(View.java:10446)
	at android.view.ViewGroup.drawChild(ViewGroup.java:2850)
	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2489)
	at android.view.View.getDisplayList(View.java:10444)
	at android.view.ViewGroup.drawChild(ViewGroup.java:2850)
	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2489)
	at android.view.View.getDisplayList(View.java:10444)
	at android.view.ViewGroup.drawChild(ViewGroup.java:2850)
	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2489)
	at android.view.View.getDisplayList(View.java:10444)
	at android.view.ViewGroup.drawChild(ViewGroup.java:2850)
	at android.view.ViewGroup.dispatchDraw(ViewGroup.java:2489)
	at android.view.View.getDisplayList(View.java:10444)
	at android.view.HardwareRenderer$GlRenderer.draw(HardwareRenderer.java:842)
	at android.view.ViewRootImpl.draw(ViewRootImpl.java:1924)
	at android.view.ViewRootImpl.performTraversals(ViewRootImpl.java:1647)
	at android.view.ViewRootImpl.handleMessage(ViewRootImpl.java:2462)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:137)
	at android.app.ActivityThread.main(ActivityThread.java:4533)
	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:784)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:551)
	at dalvik.system.NativeStart.main(Native Method)

Comment 9

[Lukas Blakk [:lsblakk] use ?needinfo]

Can we get an update on whether this has actually shifted in volume?  Also any URLs that might help with STR?

Comment 10

[Kevin Brosnan [Ex-Mozilla]]

URLs are unlikely to be helpful for a UI crash. As expected about:home and about:blank are top URLs. 

I'm unwilling to make volume assessments in Nightly or Aurora. If you want to try this patch speculatively on the next beta there have been 163 crashes on Beta 2 over the last 7 days.

The about:blank being the top URL suggests this may happen when getting a URL from and external app or opening in a new tab? Are there any other places we create a tab with about:blank set?

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

There's not enough to show that this is worth considering on Beta if it's not even confirmed to fix the issue on Nightly - too speculative at this point and it's not a topcrasher.  Go ahead with a nom for Aurora if you want to try this in the early days of FF27 Betas and see if the data shows this helped.

Comment 12

[Sriram Ramasubramanian [:sriram]]

I tried double-tapping on History button. I couldn't reproduce it. I am fine with uplifting this as this is a safety check.

Comment 13

[Aaron Train [:aaronmt]]

let's do it

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #13)
> let's do it

Do what exactly? Didn't comment #8 state that the patch didn't actually fix the crash?

Comment 15

[Aaron Train [:aaronmt]]

Err, saw the r+ and scrolled all the way down :)

Comment 16

[Lucas Rocha (:lucasr)]

(In reply to Aaron Train [:aaronmt] from comment #15)
> Err, saw the r+ and scrolled all the way down :)

I don't see this bug among the FF28 top crashers. I wonder if the patch has actually worked around this crash after all?

Comment 17

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

(In reply to Lucas Rocha (:lucasr) from comment #16)
> (In reply to Aaron Train [:aaronmt] from comment #15)
> > Err, saw the r+ and scrolled all the way down :)
> 
> I don't see this bug among the FF28 top crashers. I wonder if the patch has
> actually worked around this crash after all?

I agree. This crash no longer appears anywhere in the Fx28 crash list, but it's still #14 and #9 in Fx27 and Fx26, respectively. Let's uplift this patch.

Comment 18

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

Comment on attachment 821902 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: Crashes
Testing completed (on m-c, etc.): No longer crashing on m-c
Risk to taking this patch (and alternatives if risky): Low risk patch
String or IDL/UUID changes made by this patch: none

Comment 19

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 821902 [details] [diff] [review]
Patch

There's too much speculation and as Lucas pointed out in comment 4, we don't know the root cause. We've already built the final Beta and RC for FF26 and comment 8 shows we don't have a guarantee that landing this fixes the issue.  Not willing to take the risk of landing this without time to sit with users before ship.  Uplift to FF27 and it will be in the first beta next week.

Comment 20

[Lucas Rocha (:lucasr)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/cdd11c4a3224

Comment 21

[Lucas Rocha (:lucasr)]

I'm going to close this one given that this crasher disappeared from our top crashers in 28.

Comment 22

[Chris Peterson [:cpeterson]]

Removing steps-wanted keyword because this bug has been resolved.