Closed Bug 1410221 Opened 7 years ago Closed 7 years ago

Crash in java.lang.NullPointerException: Attempt to invoke virtual method ''android.content.Context android.view.ViewGroup.getContext()'' on a null object reference at android.support.design.widget.Snackbar.<init>(Unknown Source)

Categories

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

Product:
Firefox for Android Graveyard
Component:
Activity Stream
Version:
Firefox 57
Platform:
Unspecified
Android
Type:
defect

Tracking

(firefox56 unaffected, firefox57 wontfix, firefox58 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox56 --- unaffected
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: philipp, Assigned: mcomella)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(6 files)

Bug 1410221: Pass in parent snackbarAnchor for WebpageItemRow.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1410221: Rm unused StreamHighlightItemRowContextMenuListener.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1410221: Move TopSite openContextMenu to StreamRecyclerAdapter with listener.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1410221: Rm unnecessary context param from ActivityStreamContextMenu.show.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1410221: Rm duplicated code in openContextMenu.
59 bytes, text/x-review-board-request
liuche
: review+
Details
Bug 1410221 - review: Make names of openContextMenu unique.
59 bytes, text/x-review-board-request
mcomella
: review+
Details 
This bug was filed from the Socorro interface and is 
report bp-4f3a62aa-83ad-440e-9f9d-87e650171019.
=============================================================
Java Stack Trace 	

java.lang.NullPointerException: Attempt to invoke virtual method 'android.content.Context android.view.ViewGroup.getContext()' on a null object reference
	at android.support.design.widget.Snackbar.<init>(Unknown Source)
	at android.support.design.widget.Snackbar.make(Unknown Source)
	at android.support.design.widget.Snackbar.make(Unknown Source)
	at org.mozilla.gecko.activitystream.homepanel.menu.ActivityStreamContextMenu$4.run(ActivityStreamContextMenu.java:251)
	at android.os.Handler.handleCallback(Handler.java:739)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:148)
	at org.mozilla.gecko.util.GeckoBackgroundThread.run(GeckoBackgroundThread.java:43)

this signature is starting to show up more often in fennec 57 - overall it's quite low-level though...
Component: Theme and Visual Design → Activity Stream
Conclusion: the view that we're using as an anchor must have been removed from the view hierarchy (assuming I found the right source version of the Android design library).

---

Analysis: make [1] calls into make [2], which calls into init [3]. (note: I don't know this is the right version of the source. I also looked at the O tree, which was not the right version) Notably, in the second make:

    Snackbar snackbar = new Snackbar(findSuitableParent(view));

findSuitableParent [4] walks up the view hierarchy, looking for a parent view on which to add the Snackbar. Unfortunately, it can return null, which is why we'd get this NPE. Note that the code changed in Android O so updating the library could fix this problem.

findSuitableParent returns null if view.getParent() returns null, which can happen if view is neither a coordinatorlayout or a framelayout. This should eventually happen if the view is part of the view hierarchy (e.g. the decor view is a framelayout) so the view that we're using as an anchor must have been removed from the view hierarchy.

[1]: http://androidxref.com/6.0.1_r10/xref/frameworks/support/design/src/android/support/design/widget/Snackbar.java#230
[2]: http://androidxref.com/6.0.1_r10/xref/frameworks/support/design/src/android/support/design/widget/Snackbar.java#204
[3]: http://androidxref.com/6.0.1_r10/xref/frameworks/support/design/src/android/support/design/widget/Snackbar.java#178

[4]: http://androidxref.com/6.0.1_r10/xref/frameworks/support/design/src/android/support/design/widget/Snackbar.java#234

---

See this analysis for why our view can never be null. The code [1]:

    Snackbar.make(ActivityStreamContextMenu.this.snackbarAnchor, R.string.bookmark_added, Snackbar.LENGTH_LONG).show();

ActivityStreamContextMenu.snackbarAnchor is final, so it must be coming from the constructor. Both superclasses get the same anchor in ActivityStreamContextMenu.show, which is called from:
1. TopSitesCard() (setOnLongClickListener)
2. StreamRecyclerAdapter.openContextMenu

1) can't be null because we previously called methods on it (like setting the long click listener). In 2), the anchor comes from WebpageItemRow.getContextMenuAnchor. WebpageItemRow can't be null, or else the get would throw, so we need to figure out why WebpageItemRow is getting a null view. `getContextMenuAnchor` returns `menuButton`, which is an final view that we call prior methods on, so it also can't be null.

[1]: http://searchfox.org/mozilla-central/rev/1c4da216e00ac95b38a3f236e010b31cdfaae03b/mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/menu/ActivityStreamContextMenu.java#251
I'm able to reproduce this by:
1) Opening the context menu on the bottom-most Placeholder item (long press or 3-dot)
2) Rotating the device
3) Clicking "bookmark"

RecyclerView is re-using the views sometimes so this doesn't crash if you open the context menu for a higher up view.

We could fix this by specifying a view that will definitely be shown when the device is rotated (e.g. the parent) or fetching the view when we need to show the snackbar.
Assignee: nobody → michael.l.comella
Priority: -- → P1
Two solutions:
- Dismiss the view on rotation: simpler to implement, but a user may be rotating the device to see more of the context menu so this defeats the purpose. It'll still crash if the background scrolls (for whatever reason), invalidating the anchor we're using: this is unlikely, however. Note: Chrome does this.
- Use a different view for the anchor. This is turning out to be non-trivial because we need to use a view we know will be around after the device is rotated. This would be a view that is outside the RecyclerView items (e.g. the RecyclerView itself), but it's hard, and probably incorrect, to access these from where the click listeners are set, e.g. inside the constructor of a recycler view item view. For correctness, the click listeners should then be set from the Adapter (with a callback to the recyclerView) or the RecyclerView itself, but then it's hard to access specific items from the view we're clicking on. tbh, I don't know the "correct" way to set RecyclerView click listeners, so this is confusing.
(In reply to Michael Comella (:mcomella) from comment #3)
> Two solutions:

Other questionable solutions:
- If we can't find the anchor, we just don't show the Snackbar.
- We use a toast
Note: using views inside the dialog don't work, probably because the dialog is dismissed when we want to display the snackbar.
Uplift thoughts: let's keep this in 58. The theory of this change is simple but because there are so many layers of abstraction, I could have easily made a mistake. The crash shouldn't happen very often – just when a user rotates the device in the context menu when they selected an item at the bottom of the screen – so I'm not too concerned for 57 but I decided to fix it sooner, rather than later, because it is an annoying crash.
WONTFIX 57 (unless this becomes critical enough for a dot release).
status-firefox57: affected → wontfix
Comment on attachment 8921249 [details]
Bug 1410221: Pass in parent snackbarAnchor for WebpageItemRow.

https://reviewboard.mozilla.org/r/192256/#review199746

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:329
(Diff revision 1)
> +    /**
> +     * @param snackbarAnchor See {@link ActivityStreamContextMenu#show(Context, View, ActivityStreamTelemetry.Extras.Builder, ActivityStreamContextMenu.MenuMode, WebpageModel, boolean, HomePager.OnUrlOpenListener, HomePager.OnUrlOpenInBackgroundListener, int, int)}
> +     *                       for additional details.
> +     */
>      @Override
> -    public void openContextMenu(final WebpageItemRow webpageItemRow, final int position, @NonNull final String interactionExtra) {
> +    public void openContextMenu(final WebpageItemRow webpageItemRow, final int position, final View snackbarAnchor,

Does this still need to be a public openContextMenu, since you're removing the usage of "this" as a handler? e.g., is it sufficient that the class has an openContextMenu method, instead of it needing to be a public method? (caveat, I didn't check to see where else this is used, so maybe WebpageItemRow still uses it as a "this" handler)
Attachment #8921249 - Flags: review?(liuche) → review+
Comment on attachment 8921250 [details]
Bug 1410221: Rm unused StreamHighlightItemRowContextMenuListener.

https://reviewboard.mozilla.org/r/192258/#review199752

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:328
(Diff revision 1)
>  
>      /**
>       * @param snackbarAnchor See {@link ActivityStreamContextMenu#show(Context, View, ActivityStreamTelemetry.Extras.Builder, ActivityStreamContextMenu.MenuMode, WebpageModel, boolean, HomePager.OnUrlOpenListener, HomePager.OnUrlOpenInBackgroundListener, int, int)}
>       *                       for additional details.
>       */
> -    @Override
> +    private void openContextMenu(final WebpageItemRow webpageItemRow, final int position, final View snackbarAnchor,

ah, I see, you did make it private here :)
Attachment #8921250 - Flags: review?(liuche) → review+
Comment on attachment 8921251 [details]
Bug 1410221: Move TopSite openContextMenu to StreamRecyclerAdapter with listener.

https://reviewboard.mozilla.org/r/192260/#review199804

::: mobile/android/base/java/org/mozilla/gecko/activitystream/homepanel/StreamRecyclerAdapter.java:379
(Diff revision 1)
>  
> +    /**
> +     * @param snackbarAnchor See {@link ActivityStreamContextMenu#show(Context, View, ActivityStreamTelemetry.Extras.Builder, ActivityStreamContextMenu.MenuMode, WebpageModel, boolean, HomePager.OnUrlOpenListener, HomePager.OnUrlOpenInBackgroundListener, int, int)}
> +     *                       for additional details.
> +     */
> +    private void openContextMenu(final TopSite topSite, final int absolutePosition, final View snackbarAnchor,

I don't love that this and the method above are both called openContextMenu but with slightly different params. I guess they are pretty similar, so if you wanted to combine them I'd support that. Or maybe these could be named open TopSite or Row ContextMenu?
Attachment #8921251 - Flags: review?(liuche) → review+
Comment on attachment 8921252 [details]
Bug 1410221: Rm unnecessary context param from ActivityStreamContextMenu.show.

https://reviewboard.mozilla.org/r/192262/#review199806
Attachment #8921252 - Flags: review?(liuche) → review+
Comment on attachment 8921253 [details]
Bug 1410221: Rm duplicated code in openContextMenu.

https://reviewboard.mozilla.org/r/192264/#review199808

ah okay, this works too :)
Attachment #8921253 - Flags: review?(liuche) → review+
Comment on attachment 8924042 [details]
Bug 1410221 - review: Make names of openContextMenu unique.

https://reviewboard.mozilla.org/r/195260/#review200318

r=trivial (IDE assisted)
Attachment #8924042 - Flags: review?(michael.l.comella) → review+
Pushed by michael.l.comella@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/35600a494a24
Pass in parent snackbarAnchor for WebpageItemRow. r=liuche
https://hg.mozilla.org/integration/autoland/rev/6923f1c704e2
Rm unused StreamHighlightItemRowContextMenuListener. r=liuche
https://hg.mozilla.org/integration/autoland/rev/e010ad5fb52a
Move TopSite openContextMenu to StreamRecyclerAdapter with listener. r=liuche
https://hg.mozilla.org/integration/autoland/rev/c71c194a8765
Rm unnecessary context param from ActivityStreamContextMenu.show. r=liuche
https://hg.mozilla.org/integration/autoland/rev/457d6f0b0789
Rm duplicated code in openContextMenu. r=liuche
https://hg.mozilla.org/integration/autoland/rev/95e97bbdccdb
review: Make names of openContextMenu unique. r=mcomella
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

Creator:
Created:
Updated:
Size: