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)
Tracking
(fennec+, firefox56 disabled, firefox57 verified, firefox58 fixed)
VERIFIED
FIXED
Firefox 58
People
(Reporter: mcomella, Assigned: mcomella)
Details
Crash Data
Attachments
(4 files)
109.05 KB,
image/png
|
Details | |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
|
Details |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
liuche
:
review+
Sylvestre
:
approval-mozilla-beta+
|
Details |
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.
Assignee | ||
Comment 1•7 years ago
|
||
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 | ||
Updated•7 years ago
|
Assignee: nobody → michael.l.comella
Assignee | ||
Comment 2•7 years ago
|
||
No crash anymore but the scrolling behavior isn't great: that's bug 1403139.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 6•7 years ago
|
||
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=
Assignee | ||
Updated•7 years ago
|
tracking-fennec: ? → +
Comment 7•7 years ago
|
||
mozreview-review |
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 8•7 years ago
|
||
mozreview-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 9•7 years ago
|
||
mozreview-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+
Comment 10•7 years ago
|
||
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
Assignee | ||
Comment 11•7 years ago
|
||
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?
Assignee | ||
Comment 12•7 years ago
|
||
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?
Assignee | ||
Comment 13•7 years ago
|
||
#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 14•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5bfd1cac74e5 https://hg.mozilla.org/mozilla-central/rev/ccb464644741 https://hg.mozilla.org/mozilla-central/rev/28c4a0eaad7d
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Comment 15•7 years ago
|
||
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+
Comment 16•7 years ago
|
||
bugherder uplift |
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
status-firefox57:
--- → fixed
Updated•7 years ago
|
Attachment #8912456 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 17•7 years ago
|
||
(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.
Comment 18•7 years ago
|
||
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
Updated•7 years ago
|
status-firefox56:
--- → disabled
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•