Closed Bug 1398361 Opened 2 years ago Closed 2 years ago

Context Menu on newtab page crashes on API16

Categories

(Firefox for Android :: General, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 57
Iteration:
1.30
Tracking Status
firefox57 --- fixed

People

(Reporter: Grisha, Assigned: sebastian)

Details

(Whiteboard: [MobileAS])

Attachments

(3 files, 1 obsolete file)

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
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.
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: --- → ?
tracking-fennec: ? → ---
Rank: 1
Priority: -- → P2
Iteration: --- → 1.30
Rank: 1
Priority: P2 → P1
Rank: 2
Summary: Pocket recommendations context menu for "Placeholder" items crashes on API16 → Context Menu on newtab page crashes on API16
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
I'm unable to reproduce this on API 16 emulators. Does this still happen for you?
Flags: needinfo?(liuche)
Flags: needinfo?(gkruglov)
Still happens for me with current checkout of m-c.
Flags: needinfo?(gkruglov)
Now I can reproduce it.... :)
Flags: needinfo?(liuche)
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 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.
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
Ouch, why is this a symlink. This explains why I could only find one instance via DXR.
I'll prepare a new patch and remove the existing symlink.
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 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 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
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
https://hg.mozilla.org/mozilla-central/rev/a9e97f048dff
https://hg.mozilla.org/mozilla-central/rev/fc25613b26e0
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
(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.