Closed Bug 1403347 Opened 7 years ago Closed 7 years ago

Crash when opening "Top Sites" panel preference in landscape on Nexus 5

Categories

(Firefox for Android Graveyard :: Activity Stream, defect, P1)

All
Android
defect

Tracking

(fennec+, firefox56 disabled, firefox57 verified, firefox58 fixed)

VERIFIED FIXED
Firefox 58
Tracking Status
fennec + ---
firefox56 --- disabled
firefox57 --- verified
firefox58 --- fixed

People

(Reporter: mcomella, Assigned: mcomella)

Details

Crash Data

Attachments

(4 files)

Stack trace:

09-26 14:10:55.786 E/GeckoCrashHandler(16249): java.lang.NullPointerException: Attempt to invoke virtual method 'void android.widget.TextView.setText(java.lang.CharSequence)' on a null object reference
09-26 14:10:55.786 E/GeckoCrashHandler(16249):  at org.mozilla.gecko.preferences.PanelsPreference.configureShownDialog(PanelsPreference.java:162)
09-26 14:10:55.786 E/GeckoCrashHandler(16249):  at org.mozilla.gecko.preferences.CustomListPreference$3.onShow(CustomListPreference.java:135)
09-26 14:10:55.786 E/GeckoCrashHandler(16249):  at android.app.Dialog$ListenersHandler.handleMessage(Dialog.java:1329)
09-26 14:10:55.786 E/GeckoCrashHandler(16249):  at android.os.Handler.dispatchMessage(Handler.java:102)
09-26 14:10:55.786 E/GeckoCrashHandler(16249):  at android.os.Looper.loop(Looper.java:148)
09-26 14:10:55.786 E/GeckoCrashHandler(16249):  at android.app.ActivityThread.main(ActivityThread.java:5417)
09-26 14:10:55.786 E/GeckoCrashHandler(16249):  at java.lang.reflect.Method.invoke(Native Method)

---

We're calling ViewGroup.getChildAt(index) so I'm guessing that, because the ListView is smart and removes offscreen children, the child we're looking for is actually off-screen. We should look for the specific item rather than the index.
Note: with a larger device, I think we get bug 1403139 instead (view is not scrollable).

Confirmed: in REPL, getListView() has one child, "Set as default", while getListView().getAdapter() has three children, "Set as default", "Hide", and "Change order". Presumably the bottom of this dialog is pushing the rest of the contents off the screen.

I wonder if overwriting the value directly in the adapter (notifyChanged?) would have the same end result...
Assignee: nobody → michael.l.comella
Attached image Screenshot: post patch
No crash anymore but the scrolling behavior isn't great: that's bug 1403139.
Note: there are a few other places where bugs like this could potentially happen [1] but I opted not to fix those until they're actually problems.

[1]: http://searchfox.org/mozilla-central/search?q=path%3Amobile%2Fandroid+getlistview().getchildat&path=
Priority: -- → P1
tracking-fennec: ? → +
Comment on attachment 8912454 [details]
Bug 1403347: Move initial setHidden to panel preference constructor.

https://reviewboard.mozilla.org/r/183774/#review189346

lgtm, passing an extra parameter into constructors.
Attachment #8912454 - Flags: review?(liuche) → review+
Comment on attachment 8912455 [details]
Bug 1403347: Set dialog titles in createDialogItems.

https://reviewboard.mozilla.org/r/183776/#review189382
Attachment #8912455 - Flags: review?(liuche) → review+
Comment on attachment 8912456 [details]
Bug 1403347: Don't cache CustomListPreference dialog items.

https://reviewboard.mozilla.org/r/183778/#review189420
Attachment #8912456 - Flags: review?(liuche) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/5bfd1cac74e5
Move initial setHidden to panel preference constructor. r=liuche
https://hg.mozilla.org/integration/autoland/rev/ccb464644741
Set dialog titles in createDialogItems. r=liuche
https://hg.mozilla.org/integration/autoland/rev/28c4a0eaad7d
Don't cache CustomListPreference dialog items. r=liuche
Comment on attachment 8912455 [details]
Bug 1403347: Set dialog titles in createDialogItems.

Approval Request Comment
[Feature/Bug causing the regression]: Activity Stream. We added the ability to customize the top sites panels, which added this dialog.
[User impact if declined]: On slightly smaller than most modern devices (e.g. Nexus 5), the Top Sites panel settings dialog will crash if you open it from landscape mode.

[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No
[Needs manual test from QE? If yes, steps to reproduce]: Ideally. Bogdan can verify if he has an appropriate device. STR:
- On a smaller modern device (e.g. Nexus 5)
- go to Home settings (3-dot -> General -> Home)
- Rotate to landscape
- Click Top Sites preference; this should cause the crash.
- (Note: we changed how the "Hide"/"Show" labels are set so if anything is now broken from this dialog, it's probably these labels)

[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: Low to Medium.
[Why is the change risky/not risky?]: In theory, this change is simple and low risk: rather than modifying the views to change the displayed text, which crashes because sometimes the views we want to modify are not present, modify the data that generates the views (and remove the cache for that the data so it's generated each time). However, this code is quite entangled so I'm concerned I could be missing something. The things that make me feel it's lower risk:
- It's a small change
- There's only one way to enter this dialog and only a few ways for the state change to modify the "Hide"/"Show" label so I feel like I've tested the major ones
- This dialog is low traffic: it probably only needs to be used once per user

[String changes made/needed]: None.
Attachment #8912455 - Flags: approval-mozilla-beta?
Comment on attachment 8912456 [details]
Bug 1403347: Don't cache CustomListPreference dialog items.

Approval Request Comment: see comment 11.

---

Note that the first patch in this stack, the one set in comment 3 that does not have a beta request flag, is not necessary to uplift: it's purely code readability clean-up and should be functionally equivalent to the existing code. It's also separated enough from these changes that it should not cause a merge error. I chose not to uplift it in order to simplify the changes we're attempting to uplift.
Attachment #8912456 - Flags: approval-mozilla-beta?
#21 top crasher on 57 Beta: it's crashed 6 times since 57 has been released.
Crash Signature: [@ java.lang.NullPointerException: Attempt to invoke virtual method ''void android.widget.TextView.setText(java.lang.CharSequence)'' on a null object reference at org.mozilla.gecko.preferences.PanelsPreference.configureShownDialog(PanelsPreference.java)]
Comment on attachment 8912455 [details]
Bug 1403347: Set dialog titles in createDialogItems.

Fix a crash, it is early in the cycle, taking it.
Should be in 57b4
Attachment #8912455 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #8912456 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Ryan VanderMeulen [:RyanVM] from comment #16)
> https://hg.mozilla.org/releases/mozilla-beta/rev/cfed66643774
> https://hg.mozilla.org/releases/mozilla-beta/rev/7d38c920f81a
> https://hg.mozilla.org/releases/mozilla-beta/rev/04e03b0726f9

As I mentioned in my comment 12, uplifting the first patch isn't necessary but since it's done, maybe it's better to avoid churn.
Verified as fixed in Beta 57.0b4.
Devices:
LG Nexus 5 (Android 6.0.1)
Huawei Honor 5X (Android 5.1.1)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: