Context Menu on newtab page crashes on API16

RESOLVED FIXED in Firefox 57

Status

()

Firefox for Android
General
P1
normal
Rank:
2
RESOLVED FIXED
a month ago
a month ago

People

(Reporter: Grisha, Assigned: sebastian)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [MobileAS])

MozReview Requests

()

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

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

a month ago
Created attachment 8906132 [details]
Screenshot_1504906810.png

Additionally, pressing "Content Menu" button for any of the placeholder items crashes the app. I _think_ this is the crashlog:

W/dalvikvm( 4300): threadid=1: thread exiting with uncaught exception (group=0xa666f228)
E/GeckoCrashHandler( 4300): >>> REPORTING UNCAUGHT EXCEPTION FROM THREAD 1 ("main")
E/GeckoCrashHandler( 4300): android.content.res.Resources$NotFoundException: Resource is not a Drawable (color or path): TypedValue{t=0x1/d=0x7f020069 a=-1 r=0x7f020069}
E/GeckoCrashHandler( 4300): 	at android.content.res.Resources.loadDrawable(Resources.java:1897)
E/GeckoCrashHandler( 4300): 	at android.content.res.TypedArray.getDrawable(TypedArray.java:601)
E/GeckoCrashHandler( 4300): 	at android.view.View.<init>(View.java:3336)
E/GeckoCrashHandler( 4300): 	at android.view.View.<init>(View.java:3273)
E/GeckoCrashHandler( 4300): 	at java.lang.reflect.Constructor.constructNative(Native Method)
E/GeckoCrashHandler( 4300): 	at java.lang.reflect.Constructor.newInstance(Constructor.java:417)
E/GeckoCrashHandler( 4300): 	at android.view.LayoutInflater.createView(LayoutInflater.java:587)
E/GeckoCrashHandler( 4300): 	at com.android.internal.policy.impl.PhoneLayoutInflater.onCreateView(PhoneLayoutInflater.java:56)
E/GeckoCrashHandler( 4300): 	at android.view.LayoutInflater.onCreateView(LayoutInflater.java:660)
E/GeckoCrashHandler( 4300): 	at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:685)
E/GeckoCrashHandler( 4300): 	at android.view.LayoutInflater.rInflate(LayoutInflater.java:746)
E/GeckoCrashHandler( 4300): 	at android.view.LayoutInflater.inflate(LayoutInflater.java:489)
E/GeckoCrashHandler( 4300): 	at android.view.LayoutInflater.inflate(LayoutInflater.java:396)
E/GeckoCrashHandler( 4300): 	at android.support.design.internal.NavigationMenuPresenter$SeparatorViewHolder.<init>(NavigationMenuPresenter.java:295)
E/GeckoCrashHandler( 4300): 	at android.support.design.internal.NavigationMenuPresenter$NavigationMenuAdapter.onCreateViewHolder(NavigationMenuPresenter.java:383)
E/GeckoCrashHandler( 4300): 	at android.support.design.internal.NavigationMenuPresenter$NavigationMenuAdapter.onCreateViewHolder(NavigationMenuPresenter.java:328)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.RecyclerView$Adapter.createViewHolder(RecyclerView.java:5482)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:4707)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(RecyclerView.java:4617)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.LinearLayoutManager$LayoutState.next(LinearLayoutManager.java:1994)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.LinearLayoutManager.layoutChunk(LinearLayoutManager.java:1390)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.LinearLayoutManager.fill(LinearLayoutManager.java:1353)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.LinearLayoutManager.onLayoutChildren(LinearLayoutManager.java:574)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.RecyclerView.dispatchLayoutStep2(RecyclerView.java:3028)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.RecyclerView.onMeasure(RecyclerView.java:2625)
E/GeckoCrashHandler( 4300): 	at android.view.View.measure(View.java:15172)
E/GeckoCrashHandler( 4300): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:4816)
E/GeckoCrashHandler( 4300): 	at android.widget.FrameLayout.onMeasure(FrameLayout.java:310)
E/GeckoCrashHandler( 4300): 	at android.support.design.widget.NavigationView.onMeasure(NavigationView.java:223)
E/GeckoCrashHandler( 4300): 	at android.view.View.measure(View.java:15172)
E/GeckoCrashHandler( 4300): 	at android.support.v4.widget.NestedScrollView.measureChildWithMargins(NestedScrollView.java:1415)
E/GeckoCrashHandler( 4300): 	at android.widget.FrameLayout.onMeasure(FrameLayout.java:310)
E/GeckoCrashHandler( 4300): 	at android.support.v4.widget.NestedScrollView.onMeasure(NestedScrollView.java:480)
E/GeckoCrashHandler( 4300): 	at android.view.View.measure(View.java:15172)
E/GeckoCrashHandler( 4300): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:4816)
E/GeckoCrashHandler( 4300): 	at android.widget.LinearLayout.measureChildBeforeLayout(LinearLayout.java:1390)
E/GeckoCrashHandler( 4300): 	at android.widget.LinearLayout.measureVertical(LinearLayout.java:681)
E/GeckoCrashHandler( 4300): 	at android.widget.LinearLayout.onMeasure(LinearLayout.java:574)
E/GeckoCrashHandler( 4300): 	at android.view.View.measure(View.java:15172)
E/GeckoCrashHandler( 4300): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:4816)
E/GeckoCrashHandler( 4300): 	at android.widget.FrameLayout.onMeasure(FrameLayout.java:310)
E/GeckoCrashHandler( 4300): 	at android.view.View.measure(View.java:15172)
E/GeckoCrashHandler( 4300): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:4816)
E/GeckoCrashHandler( 4300): 	at android.support.design.widget.CoordinatorLayout.onMeasureChild(CoordinatorLayout.java:668)
E/GeckoCrashHandler( 4300): 	at android.support.design.widget.CoordinatorLayout.onMeasure(CoordinatorLayout.java:735)
E/GeckoCrashHandler( 4300): 	at android.view.View.measure(View.java:15172)
E/GeckoCrashHandler( 4300): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:4816)
E/GeckoCrashHandler( 4300): 	at android.widget.FrameLayout.onMeasure(FrameLayout.java:310)
E/GeckoCrashHandler( 4300): 	at android.support.v7.widget.ContentFrameLayout.onMeasure(ContentFrameLayout.java:135)
E/GeckoCrashHandler( 4300): 	at android.view.View.measure(View.java:15172)
E/GeckoCrashHandler( 4300): 	at android.view.ViewGroup.measureChildWithMargins(ViewGroup.java:4816)
E/GeckoCrashHandler( 4300): 	at androi
(Reporter)

Comment 1

a month ago
Actually: I only see this on local builds running in emulator (I probably don't have dev keys to fetch content?).

And the crash happens on API16, but does not happen on API26.
(Reporter)

Updated

a month ago
Summary: Pocket recommendations display "Placeholder 0", "Placeholder 1", etc after clearing application data → Pocket recommendations context menu for "Placeholder" items crashes on API16
Yeah, you need an API key to fetch from Pocket. Let me know if you need/want access to the keys, make sure you don't push it anywhere. The keys are on the builders for Nightly, Beta, and Release.

I'll flag this for triage - I'm also seeing this in the emulator for any long-press for context menu, e.g. top sites. Maybe something we're doing with the Bottom Sheet is using the wrong APIs (though the linter should catch that). fwiw there don't seem to be any problems on a physical 4.4 (r19) device.
tracking-fennec: --- → ?

Updated

a month ago
tracking-fennec: ? → ---
Rank: 1
Priority: -- → P2

Updated

a month ago
Iteration: --- → 1.30
Rank: 1
Priority: P2 → P1

Updated

a month ago
Rank: 2

Updated

a month ago
Summary: Pocket recommendations context menu for "Placeholder" items crashes on API16 → Context Menu on newtab page crashes on API16
(Assignee)

Updated

a month ago
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
(Assignee)

Comment 3

a month ago
I'm unable to reproduce this on API 16 emulators. Does this still happen for you?
Flags: needinfo?(liuche)
Flags: needinfo?(gkruglov)
(Reporter)

Comment 4

a month ago
Still happens for me with current checkout of m-c.
Flags: needinfo?(gkruglov)
(Assignee)

Comment 5

a month ago
Now I can reproduce it.... :)
Flags: needinfo?(liuche)
(Assignee)

Comment 6

a month ago
as_contextmenu_divider.xml exists in drawable-ldltr and drawable-ldrtl. RTL support was introduced with API 17. API 16 devices do not know about those folders and therefore can't load the resource.
Comment hidden (mozreview-request)

Comment 8

a month ago
mozreview-review
Comment on attachment 8907613 [details]
Bug 1398361 - as_contextmenu_divider: Remove symlink and move ltr drawable from drawable-ldlrtl/ to drawable/.

https://reviewboard.mozilla.org/r/179296/#review184568

I think it's unclear how the drawable folders will resolve but if you tested it on API 16, API 17+ LTR, and API 17+ RTL, wfm. To be clear, the problem this ldltr/ldrtl divider was intended to solve is that the AS context menu divider (3-dot on Top Stories/highlights), which is right-aligned in LTR, needs to be left-aligned in RTL.

fwiw, I tested this myself on API 17+ and it works as expected.
Attachment #8907613 - Flags: review?(michael.l.comella) → review+
Landing this myself so that we can get it in before the deadline.

Comment 10

a month ago
hg error in cmd: hg push -r tip ssh://hg.mozilla.org/integration/autoland: pushing to ssh://hg.mozilla.org/integration/autoland
searching for changes
remote: adding changesets
remote: adding manifests
remote: adding file changes
remote: added 1 changesets with 1 changes to 1 files
remote: 
remote: ******************************* ERROR *******************************
remote: b01487f5f6d8 adds or modifies the following symlinks:
remote: 
remote:   mobile/android/app/src/main/res/drawable/as_contextmenu_divider.xml
remote: 
remote: Symlinks aren't allowed in this repo. Convert these paths to regular
remote: files and try your push again.
remote: *********************************************************************
remote: 
remote: transaction abort!
remote: rollback completed
remote: pretxnchangegroup.mozhooks hook failed
abort: push failed on remote
(Assignee)

Comment 11

a month ago
Ouch, why is this a symlink. This explains why I could only find one instance via DXR.
(Assignee)

Comment 12

a month ago
I'll prepare a new patch and remove the existing symlink.
Comment hidden (mozreview-request)

Comment 14

a month ago
mozreview-review
Comment on attachment 8907613 [details]
Bug 1398361 - as_contextmenu_divider: Remove symlink and move ltr drawable from drawable-ldlrtl/ to drawable/.

https://reviewboard.mozilla.org/r/179296/#review184644

I don't love the duplication but I'm sure you tried to fix it and perhaps comment are our best solution.

::: mobile/android/app/src/main/res/drawable-ldrtl/as_contextmenu_divider.xml:5
(Diff revision 2)
>  <?xml version="1.0" encoding="utf-8"?>
>  <!-- This Source Code Form is subject to the terms of the Mozilla Public
>     - License, v. 2.0. If a copy of the MPL was not distributed with this
>     - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> -
> +<inset xmlns:android="http://schemas.android.com/apk/res/android"

nit: add a comment that this mirors drawable/...

::: mobile/android/app/src/main/res/drawable/as_contextmenu_divider.xml:5
(Diff revision 2)
> +<?xml version="1.0" encoding="utf-8"?>
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<inset xmlns:android="http://schemas.android.com/apk/res/android"

nit: Same: similar comment.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

a month ago
mozreview-review
Comment on attachment 8907800 [details]
Bug 1398361 - as_contextmenu_divider: Remove symlink and move ltr drawable from drawable-ldlrtl/ to drawable/.

https://reviewboard.mozilla.org/r/179484/#review184650

I repushed these commits so I can edit and land them. However, reviewboard duplicated these commits. I already reviewed this one.
Attachment #8907800 - Flags: review?(michael.l.comella) → review+

Comment 18

a month ago
mozreview-review
Comment on attachment 8907801 [details]
Bug 1398361 - review: Add comment how as_contextmenu_divider is similar for ltr and rtl.

https://reviewboard.mozilla.org/r/179486/#review184652

r=trivial. This fixes my nits from Sebastian's initial patch.
Attachment #8907801 - Flags: review?(michael.l.comella) → review+
Comment on attachment 8907613 [details]
Bug 1398361 - as_contextmenu_divider: Remove symlink and move ltr drawable from drawable-ldlrtl/ to drawable/.

Obsoleting Sebastian's push, now that mine (with his commit) overrode his.
Attachment #8907613 - Attachment is obsolete: true

Comment 20

a month ago
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a9e97f048dff
as_contextmenu_divider: Remove symlink and move ltr drawable from drawable-ldlrtl/ to drawable/. r=mcomella
https://hg.mozilla.org/integration/autoland/rev/fc25613b26e0
review: Add comment how as_contextmenu_divider is similar for ltr and rtl. r=mcomella

Comment 21

a month ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a9e97f048dff
https://hg.mozilla.org/mozilla-central/rev/fc25613b26e0
Status: ASSIGNED → RESOLVED
Last Resolved: a month ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(Assignee)

Comment 22

a month ago
(In reply to Michael Comella (:mcomella) from comment #14)
> nit: add a comment that this mirors drawable/...

I'm not always sure that those comments add much. The IDE always shows me that there are multiple versions of this file with different resource qualifiers. But the comment is not necessarily updated automatically. :)
You need to log in before you can comment on or make changes to this bug.