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

RESOLVED FIXED in Firefox 36

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: snorp, Assigned: mcomella)

Tracking

({crash})

Trunk
Firefox 38
All
Android
crash
Points:
---

Firefox Tracking Flags

(firefox33 wontfix, firefox35 wontfix, firefox36 fixed, firefox37 fixed, firefox38 fixed, fennec+)

Details

(crash signature)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 1 obsolete attachment)

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)
Jim, this is on 33, but why didn't this crash make it into socorro? It looks like a pretty vanilla Java crash.
status-firefox33: --- → affected
Flags: needinfo?(nchen)
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.
Version: unspecified → Trunk
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
(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) 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.
Flags: needinfo?(nchen)
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.
tracking-fennec: --- → ?
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.gecko.toolbar.BrowserToolbar.updateProgressVisibility(BrowserToolbar.java)]
status-firefox35: --- → affected
status-firefox36: --- → affected
status-firefox37: --- → affected
status-firefox38: --- → affected
Keywords: crash
Assignee: nobody → michael.l.comella
tracking-fennec: ? → +
1500 crashes a week
(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
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.
Created attachment 8556800 [details]
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
Attachment #8556800 - Flags: review?(rnewman)
Comment on attachment 8556800 [details]
MozReview Request: bz://1090287/mcomella

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

Ship It!
Attachment #8556800 - Flags: review?(rnewman) → review+
https://hg.mozilla.org/integration/fx-team/rev/068b172026ef
https://hg.mozilla.org/mozilla-central/rev/068b172026ef
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 38
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
Attachment #8556800 - Flags: approval-mozilla-beta?
Attachment #8556800 - Flags: approval-mozilla-aurora?
status-firefox33: affected → wontfix
status-firefox35: affected → wontfix
status-firefox38: affected → fixed
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.
Attachment #8556800 - Flags: approval-mozilla-beta?
Attachment #8556800 - Flags: approval-mozilla-beta+
Attachment #8556800 - Flags: approval-mozilla-aurora?
Attachment #8556800 - Flags: approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e7182df6ae9
https://hg.mozilla.org/releases/mozilla-beta/rev/0a9d521bf670
status-firefox36: affected → fixed
status-firefox37: affected → fixed
Comment on attachment 8556800 [details]
MozReview Request: bz://1090287/mcomella
Attachment #8556800 - Attachment is obsolete: true
Attachment #8618491 - Flags: review+
Created attachment 8618491 [details]
MozReview Request: Bug 1090287 - Check that the selected tab is not null before updating progress visibility. r=rnewman
You need to log in before you can comment on or make changes to this bug.