1401733 - Crash in java.lang.NoSuchMethodException: <init> [class android.content.Context, interface android.util.AttributeSet] at java.lang.Class.getConstructor0(Class.java)

Status: RESOLVED FIXED

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

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

This bug was filed from the Socorro interface and is 
report bp-d952a0fa-cd6e-4db1-9648-e39fe0170920.
=============================================================

Crash first started with the 9-19 Nightly,

> Trace 	
> 
> java.lang.NoSuchMethodException: <init> [class android.content.Context, interface android.util.AttributeSet]
> 	at java.lang.Class.getConstructor0(Class.java:2320)
> 	at java.lang.Class.getConstructor(Class.java:1725)
> 	at android.view.LayoutInflater.createView(LayoutInflater.java:615)
> 	at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:790)
> 	at android.view.LayoutInflater.createViewFromTag(LayoutInflater.java:730)
> 	at android.view.LayoutInflater.rInflate(LayoutInflater.java:863)
> 	at android.view.LayoutInflater.rInflateChildren(LayoutInflater.java:824)
> 	at android.view.LayoutInflater.rInflate(LayoutInflater.java:866)
> 	at android.view.LayoutInflater.rInflateChildren(LayoutInflater.java:824)
> 	at android.view.LayoutInflater.inflate(LayoutInflater.java:515)
> 	at android.view.LayoutInflater.inflate(LayoutInflater.java:423)
> 	at org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter.onCreateViewHolder(StreamRecyclerAdapter.java:131)
> 	at org.mozilla.gecko.activitystream.homepanel.StreamRecyclerAdapter.onCreateViewHolder(StreamRecyclerAdapter.java:47)
> 	at android.support.v7.widget.RecyclerView$Adapter.createViewHolder(Unknown Source:6)
> 	at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(Unknown Source:358)
> 	at android.support.v7.widget.RecyclerView$Recycler.getViewForPosition(Unknown Source:1)
> 	at android.support.v7.widget.LinearLayoutManager$LayoutState.next(Unknown Source:11)
> 	at android.support.v7.widget.LinearLayoutManager.layoutChunk(Unknown Source:3)
> 	at android.support.v7.widget.LinearLayoutManager.fill(Unknown Source:47)
> 	at android.support.v7.widget.LinearLayoutManager.onLayoutChildren(Unknown Source:374)
> 	at android.support.v7.widget.RecyclerView.dispatchLayoutStep2(Unknown Source:44)
> 	at android.support.v7.widget.RecyclerView.dispatchLayout(Unknown Source:51)
> 	at android.support.v7.widget.RecyclerView.onLayout(Unknown Source:6)
> 	at android.view.View.layout(View.java:19586)
> 	at android.view.ViewGroup.layout(ViewGroup.java:6053)

Can you take a look, Mike?

Comment 1

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Regression from bug 1400397: in that bug we changed the (Context, Attributeset) constructor to three arguments for StreamOverridablePageIconLayout [1]. This constructor is expected to get called by `BrowserApp.onCreateView`, but it apparently isn't for a small number of users.

[1]: http://searchfox.org/mozilla-central/rev/d08b24e613cac8c9c5a4131452459241010701e0/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/stream/StreamOverridablePageIconLayout.java#81

Comment 2

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

It looks like all crashes are API 26 (android O).

Comment 3

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Things I noticed:

1) `onCreateView(String, Context, AttributeSet)` is apparently for pre-HC devices: "This implementation does nothing and is for pre-HONEYCOMB apps. Newer apps should use onCreateView(View, String, Context, AttributeSet)." [1] is the most recent constructor.

2) I can't reproduce this on an Android O emulator.

[1]: https://developer.android.com/reference/android/support/v4/app/FragmentActivity.html#onCreateView(android.view.View,%20java.lang.String,%20android.content.Context,%20android.util.AttributeSet)

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I checked with Snorp, who has a physical 6P and Pixel with Android O: he is also unable to reproduce.

Perhaps these views are getting inflated through an alternate code path (e.g. on rotation? when resuming from OOM?).

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Looking at the last invocation of createViewFromTag (in the stack trace), here's the source [1]:

770            View view;
771            if (mFactory2 != null) {
772                view = mFactory2.onCreateView(parent, name, context, attrs);
773            } else if (mFactory != null) {
774                view = mFactory.onCreateView(name, context, attrs);
775            } else {
776                view = null;
777            }
778
779            if (view == null && mPrivateFactory != null) {
780                view = mPrivateFactory.onCreateView(parent, name, context, attrs);
781            }
782
783            if (view == null) {
784                final Object lastContext = mConstructorArgs[0];
785                mConstructorArgs[0] = context;
786                try {
787                    if (-1 == name.indexOf('.')) {
788                        view = onCreateView(parent, name, attrs);
789                    } else {
790                        view = createView(name, null, attrs);

We're throwing in createView, at the bottom, but on 772 and 774, our `onCreateView` methods should get called first. Either:

1) mFactory is not set properly (or the intended behavior has changed), for whatever reason
2) Our use of onCreateView is incorrect so it's not finding the class name correctly (Proguard?).

We may be able to resolve 1) by using the mFactory2 form of onCreateView.

[1]: http://androidxref.com/8.0.0_r4/xref/frameworks/base/core/java/android/view/LayoutInflater.java#790

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

ProGuard doesn't seem like it'd be the problem: neither BrowserToolbar or TabsPanel.TabsLayout does anything special for ProGuard.

Also, for the other names that use onCreateView:
- BrowserToolbar is an abstract class (but has a Context, AttributeSet constructor)
- TabsPanel.TabsLayout is an interface

Comment 7

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Continuing with the source code, mFactory2 gets set in:
- LayoutInflater.setFactory2 [1]
- LayoutInflater(LayoutInflater, Context) [2] (a copy constructor)

Assuming we're not dealing with a copy, setFactory2 is called from:
- LayoutInflaterCompatBaseImpl.setFactory2 [3]
- LayoutInflaterCompatApi21Impl.setFactory2 & setFactory (overrides previous) [4]
- LayoutInflaterCompat.setFactory2 [5] (calls out to ^ above) [5]

Looking at the Nougat code (one API level down), setFactory2 is called from completely different places! [6]
- LayoutInflaterCompatHC.setFactory [7]
- LayoutInflaterCompatLollipop.setFactory [8]

Because the code path has changed, I think it's possible there could be a bug in the new Android O implementation (the deprecated method would likely be less tested) and that switching to the other onCreateView could solve this crash.

[1]: http://androidxref.com/8.0.0_r4/xref/frameworks/base/core/java/android/view/LayoutInflater.java#312
[2]: http://androidxref.com/8.0.0_r4/xref/frameworks/base/core/java/android/view/LayoutInflater.java#219

[3]: http://androidxref.com/8.0.0_r4/xref/frameworks/support/compat/java/android/support/v4/view/LayoutInflaterCompat.java#102
[4]: http://androidxref.com/8.0.0_r4/xref/frameworks/support/compat/java/android/support/v4/view/LayoutInflaterCompat.java#130
[5]: http://androidxref.com/8.0.0_r4/xref/frameworks/support/compat/java/android/support/v4/view/LayoutInflaterCompat.java#179

[6]: http://androidxref.com/7.1.1_r6/search?q=setFactory2&defs=&refs=&path=&hist=&project=frameworks
[7]: http://androidxref.com/7.1.1_r6/xref/frameworks/support/compat/honeycomb/android/support/v4/view/LayoutInflaterCompatHC.java#51
[8]: http://androidxref.com/7.1.1_r6/xref/frameworks/support/compat/api21/android/support/v4/view/LayoutInflaterCompatLollipop.java#24

Comment 8

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Without doing more source diving (and we may be hitting the point of diminishing returns), I think updating onCreateView to the new version could fix this problem.

That being said, considering the imminent merge and risk it carries to make speculative fixes, we could fix this in a "less correct way" by either:
- calling `setCache` after construction. It's messy to pass the instances into all of these views, however.
- Using a static cache for all instances. This would be fairly similar to the current implementation, except that the cache will live past the Activity, until the class is unloaded.

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

I decided to not make the speculative fix - I moved the upgrade to the new onCreateView to bug 1401779 so it can be independently reviewed and backed out in case of issues.

Comment 10

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Bug 1401733: Make StreamOverridablePageIconLayout caches static.

Unfortunately, I can't reproduce so I'm unable to verify this fix works for
sure. However, I verified the behavior of the StreamOverridablePageIconLayout
remained the same after these changes.

As per my bug comments, using the new version of onCreateView may fix this
problem and is "more correct" than this fix but it's speculative and it's too
dangerous to make speculative fixes this close to the 57 merge. Instead, we do
this surefire fix and lose a little correctness.

The onCreateView fix moved to bug 1401779.

Review commit: https://reviewboard.mozilla.org/r/181978/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/181978/

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

NI self to verify this gets into 57.

Comment 12

[Chenxia Liu [:liuche]]

Comment on attachment 8910539 [details]
Bug 1401733: Make StreamOverridablePageIconLayout caches static.

https://reviewboard.mozilla.org/r/181978/#review187360

Okay, I don't love the static cache either, but hopefully this is pretty small cache bc it's just url strings. Making it static *does* fix this problem, and since we're close to merge, a sure fix for a crash sounds good to me.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7bb527a1748f
Make StreamOverridablePageIconLayout caches static. r=liuche

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

FWIW, bug 1377819 is another unrelated crash where `BrowserApp.onCreateView` is apparently never called.

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/7bb527a1748f

Comment 16

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

(In reply to Michael Comella (:mcomella) from comment #11)
> NI self to verify this gets into 57.

Done.