1090287 - java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbar.updateProgressVisibility(BrowserToolbar.java:626)

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[James Willcox (:snorp) ([e-mail]) (he/him)]

Found from Google Play dashboard

java.lang.NullPointerException
at org.mozilla.gecko.toolbar.BrowserToolbar.updateProgressVisibility(BrowserToolbar.java:626)
at org.mozilla.gecko.toolbar.BrowserToolbar.startEditing(BrowserToolbar.java:1008)
at org.mozilla.gecko.BrowserApp.enterEditingMode(BrowserApp.java:1717)
at org.mozilla.gecko.BrowserApp.enterEditingMode(BrowserApp.java:1695)
at org.mozilla.gecko.BrowserApp.access$400(BrowserApp.java:124)
at org.mozilla.gecko.BrowserApp$9.onActivate(BrowserApp.java:659)
at org.mozilla.gecko.toolbar.BrowserToolbar$3.onClick(BrowserToolbar.java:325)
at android.view.View.performClick(View.java:4421)
at android.view.View$PerformClick.run(View.java:18190)
at android.os.Handler.handleCallback(Handler.java:725)
at android.os.Handler.dispatchMessage(Handler.java:92)
at android.os.Looper.loop(Looper.java:175)
at android.app.ActivityThread.main(ActivityThread.java:5279)
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:1102)
at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:869)
at dalvik.system.NativeStart.main(Native Method)

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Jim, this is on 33, but why didn't this crash make it into socorro? It looks like a pretty vanilla Java crash.

Comment 2

[Richard Newman [:rnewman]]

Play spot the bug:

    private void updateProgressVisibility() {
        final Tab selectedTab = Tabs.getInstance().getSelectedTab();
        updateProgressVisibility(selectedTab, selectedTab.getLoadProgress());
    }

if there's no selected tab (as is the case before any page has loaded), this will NPE.

This is unchanged in trunk.

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

We appear to check for the selected tab being null before updateProgressVisibility is called in most cases [1], except for in startEditing [2]. To fully analyze this bug, it'd be helpful to find where the initial tab selection occurs, though the blind fix is pretty trivial.

[1]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=2af61439e1e8#437
[2]: https://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/BrowserToolbar.java?rev=2af61439e1e8#772

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #1)
> Jim, this is on 33, but why didn't this crash make it into socorro? It looks
> like a pretty vanilla Java crash.

For 33, 34, and 35, we pass the exception to the Play Store when the exception was not handled by Breakpad (i.e. when Gecko is not loaded). 36 added the Java crash reporter that reports to Socorro without Breakpad.

Comment 5

[Richard Newman [:rnewman]]

Excellent stacks for this, which is still happening in new tablet code:

java.lang.NullPointerException
	at org.mozilla.gecko.toolbar.BrowserToolbar.updateProgressVisibility(BrowserToolbar.java:549)
	at org.mozilla.gecko.toolbar.BrowserToolbar.startEditing(BrowserToolbar.java:803)
	at org.mozilla.gecko.toolbar.BrowserToolbarNewTablet.startEditing(BrowserToolbarNewTablet.java:183)
	at org.mozilla.gecko.BrowserApp.enterEditingMode(BrowserApp.java:1971)
	at org.mozilla.gecko.BrowserApp.enterEditingMode(BrowserApp.java:1947)
	at org.mozilla.gecko.BrowserApp.access$600(BrowserApp.java:146)
	at org.mozilla.gecko.BrowserApp$10.onActivate(BrowserApp.java:833)
	at org.mozilla.gecko.toolbar.BrowserToolbar$1.onClick(BrowserToolbar.java:239)
	at android.view.View.performClick(View.java:4445) 

https://crash-stats.mozilla.com/report/index/eceb954b-1d01-419d-934f-ef7f42150125

We're getting 1500+ crashes a month with this signature.

Looks like it might be an easy fix.

Comment 6

[Kevin Brosnan [Ex-Mozilla]]

1500 crashes a week

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Richard Newman [:rnewman] from comment #5)
> Excellent stacks for this, which is still happening in new tablet code:

Not just new tablet [1] (and it's in 35) - seems rooted in all the toolbar code.

Uptimes are low (assuming millis) which means it's probably that the selected tab is not yet initialized (i.e. null) and the toolbar click is the initial toolbar hit.

As per comment 3, we should find how the tab is initialized, see if there's anything to be done there, and if not, block it with if (... != null).

[1]: https://crash-stats.mozilla.com/report/index/1ade1cac-bb01-4972-b5ad-d1dee2150123

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

GeckoApp.onWindowFocusChange
 GA.initialize
  GA.loadStartupTab
   Tabs.loadUrl
    Tabs.selectTab ***

Since onWindowFocusChange is not a lifecycle callback, it's not guaranteed to be called before BrowserApp.onCreate (where inflation and thus BrowserToolbar initialization takes place). Thus, the user can tap the toolbar before GeckoApp, and thus the selected tab, is initialized.

Talked to Finkle and it seems the (if (selectedTab != null) band-aid is appropriate (filed bug 1127631 to remove other null band-aids).

bug 1085591 to fix where GeckoApp is initialized.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: bz://1090287/mcomella

/r/3153 - Bug 1090287 - Check that the selected tab is not null before updating progress visibility. r=rnewman

Pull down this commit:

hg pull review -r 7857e8e1cce5acc2e7ab9eb28e30dd9fc0c7ed4e

Comment 10

[Richard Newman [:rnewman]]

Comment on attachment 8556800 [details]
MozReview Request: bz://1090287/mcomella

https://reviewboard.mozilla.org/r/3151/#review2531

Ship It!

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

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

Comment 12

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/068b172026ef

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8556800 [details]
MozReview Request: bz://1090287/mcomella

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

[User impact if declined]:
  Crashes occassionally when the toolbar is tapped before Gecko has finished loading (i.e. selected a tab).

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

[Risks and why]:
  Low - added a nul check, pretty harmless.
 
[String/UUID change made/needed]: None

Comment 14

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8556800 [details]
MozReview Request: bz://1090287/mcomella

We've had NPE checks cause issues before. However, looking at how we're handling the NPE in this case, I agree that this is low risk.

I think we should take this fix on Aurora and Beta. a+ to both.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e7182df6ae9
https://hg.mozilla.org/releases/mozilla-beta/rev/0a9d521bf670

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: MozReview Request: Bug 1090287 - Check that the selected tab is not null before updating progress visibility. r=rnewman