Open Bug 1817999 Opened 2 years ago Updated 3 months ago

Crash in [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor]

Categories

(Firefox for Android :: Bookmarks, defect, P5)

Product:
Firefox for Android
Component:
Bookmarks
Platform:
All
Android
Type:
defect

Tracking

()

Status:
REOPENED
Tracking Flags:
Tracking Status
firefox112 --- wontfix
firefox113 --- wontfix
firefox114 --- wontfix
firefox115 --- wontfix
firefox116 --- affected
firefox117 --- affected

People

(Reporter: aaronmt, Assigned: royang)

Details

(Keywords: crash, ui-test-bug-auto-found, Whiteboard: robo-test)

Crash Data

Attachments

(1 file)

[mozilla-mobile/firefox-android] Bug 1817999 - Use weak view reference in bookmarks fragment (#1812)
59 bytes, text/x-github-pull-request
Details | Review

Robo test, Pixel 2, API Level 27 (running latest main debug, 02/21/2023)

java.lang.NullPointerException: Attempt to invoke interface method 'java.lang.String java.lang.CharSequence.toString()' on a null object reference

java.lang.NullPointerException
     FATAL EXCEPTION: main
Process: org.mozilla.fenix.debug, PID: 21047
java.lang.NullPointerException
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:71)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.refreshBookmarks(BookmarkFragment.kt:288)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.access$refreshBookmarks(BookmarkFragment.kt:64)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment$refreshBookmarks$1.invokeSuspend(Unknown Source:14)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:106)
	at android.os.Handler.handleCallback(Handler.java:790)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at androidx.test.espresso.base.Interrogator.loopAndInterrogate(Interrogator.java:10)
	at androidx.test.espresso.base.UiControllerImpl.loopUntil(UiControllerImpl.java:7)
	at androidx.test.espresso.base.UiControllerImpl.loopUntil(UiControllerImpl.java:1)
	at androidx.test.espresso.base.UiControllerImpl.loopMainThreadForAtLeast(UiControllerImpl.java:7)
	at androidx.test.espresso.action.Tap$1.sendTap(Tap.java:4)
	at androidx.test.espresso.action.GeneralClickAction.perform(GeneralClickAction.java:4)
	at androidx.test.espresso.ViewInteraction$SingleExecutionViewAction.perform(ViewInteraction.java:2)
	at androidx.test.espresso.ViewInteraction.doPerform(ViewInteraction.java:23)
	at androidx.test.espresso.ViewInteraction.-$$Nest$mdoPerform(Unknown Source:0)
	at androidx.test.espresso.ViewInteraction$1.call(ViewInteraction.java:6)
	at androidx.test.espresso.ViewInteraction$1.call(ViewInteraction.java:1)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at android.os.Handler.handleCallback(Handler.java:790)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loop(Looper.java:164)
	at android.app.ActivityThread.main(ActivityThread.java:6494)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:438)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:807)

Caught via Robo Test https://console.firebase.google.com/u/0/project/moz-fenix/testlab/histories/bh.66b7091e15d53d45/matrices/7156314739697343015/executions/bs.aaf622fcb123d34c

Screenshots, video, logs available in Matrix above.

Severity: -- → S3
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:71)]
status-firefox112: --- → affected
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:71)] → [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor]
Summary: Crash in [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:71)] → Crash in [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor]
Crash Signature: [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor] → [@ java.lang.NullPointerException: at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:71)]
Whiteboard: robo-test

Based on the comment in refreshBookmarks I wonder if this is https://github.com/mozilla-mobile/fenix/issues/4671 again or something similar?

Flags: needinfo?(royang)

Can't reproduce but implemenation around getBookmarkInteractor looks risky. I'll look into this more and clean up the implementation.

Flags: needinfo?(royang)
Assignee: nobody → royang

Robo test, Pixel 2, API Level 27

https://console.firebase.google.com/u/0/project/moz-fenix/testlab/histories/bh.66b7091e15d53d45/matrices/8247937899439655322/executions/bs.1d3abbe50163cf10

java.lang.NullPointerException
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:71)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.refreshBookmarks(BookmarkFragment.kt:288)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.access$refreshBookmarks(BookmarkFragment.kt:64)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment$refreshBookmarks$1.invokeSuspend(Unknown Source:14)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)

I've hidden some of the comments with long stack traces as duplicates because it's harder to grok this thread with them in there too and we have enough information to start some investigation.

Roger and I spoke about this briefly and it's better to document our discussion and understanding so far here, because the solutions in the linked PR are subjective.

What is the bug?

There is a reference to the bookmarkInteractor that uses a hacky pattern to hide that there is a nullable object backing this one:

private var _bookmarkInteractor: BookmarkFragmentInteractor? = null
private val bookmarkInteractor: BookmarkFragmentInteractor
    get() = _bookmarkInteractor!!

The backing _bookmarkInteractor is initialized in the BookmarkFragment's onCreateView and destroyed in the onDestroyView. This is symmetrical and seems fine on the surface.

However, since we are hiding this nullable reference behind a non-null one AND there are cases where we can use the interactor after a the view is destroyed, it makes this a perfect place to have race conditions lead to crashes.

The stack traces show that we are coming from refreshBookmarks which is called from two places that are via accessed via the undo Snackbar. What I believe is happening, is that, we are performing a large IO task with bookmarks, the view is destroyed, but the undo action has yet to be performed. By the time it does perform, the backing interactor is nullified and we crash.

We can reproduce this stack trace with a simple patch and some STR to demonstrate the behaviour:

diff --git a/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt b/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt
index 9ddd6f9fdd..0d267d2c56 100644
--- a/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt
+++ b/fenix/app/src/main/java/org/mozilla/fenix/library/bookmarks/BookmarkFragment.kt
@@ -285,6 +285,7 @@ class BookmarkFragment : LibraryPageFragment<BookmarkNode>(), UserInteractionHan
         val currentGuid = bookmarkStore.state.tree?.guid ?: return
         loadBookmarkNode(currentGuid)
             ?.let { node ->
+                _bookmarkInteractor = null
                 val rootNode = node - pendingBookmarksToDelete
                 bookmarkInteractor.onBookmarksChanged(rootNode)
             }
  1. Delete a bookmark from the Bookmarks list.
  2. Quickly, leave the view.
  3. Click on the 'Undo' snackbar before it disappears.

6/26 Firebase Link

► 07/13 Firebase link
► 07/14 Firebase link

status-firefox117: --- → affected

► 08/01 Firebase link

► 08/10 Firebase link

Slight adjustment in the stack

java.lang.NullPointerException
     FATAL EXCEPTION: main
Process: org.mozilla.fenix.debug, PID: 2754
java.lang.NullPointerException
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:72)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.refreshBookmarks(BookmarkFragment.kt:297)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.access$refreshBookmarks(BookmarkFragment.kt:65)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment$refreshBookmarks$1.invokeSuspend(Unknown Source:14)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:108)
	at android.os.Handler.handleCallback(Handler.java:883)
	at android.os.Handler.dispatchMessage(Handler.java:100)
	at androidx.test.espresso.base.Interrogator.loopAndInterrogate(Interrogator.java:10)
	at androidx.test.espresso.base.UiControllerImpl.loopUntil(UiControllerImpl.java:7)
	at androidx.test.espresso.base.UiControllerImpl.loopUntil(UiControllerImpl.java:1)
	at androidx.test.espresso.base.UiControllerImpl.loopMainThreadForAtLeast(UiControllerImpl.java:7)
	at androidx.test.espresso.action.MotionEvents.sendDown(MotionEvents.java:9)
	at androidx.test.espresso.action.Tap.sendSingleTap(Tap.java:4)
	at androidx.test.espresso.action.Tap.-$$Nest$smsendSingleTap(Unknown Source:0)
	at androidx.test.espresso.action.Tap$1.sendTap(Tap.java:1)
	at androidx.test.espresso.action.GeneralClickAction.perform(GeneralClickAction.java:4)
	at androidx.test.espresso.ViewInteraction$SingleExecutionViewAction.perform(ViewInteraction.java:2)
	at androidx.test.espresso.ViewInteraction.doPerform(ViewInteraction.java:23)
	at androidx.test.espresso.ViewInteraction.-$$Nest$mdoPerform(Unknown Source:0)
	at androidx.test.espresso.ViewInteraction$1.call(ViewInteraction.java:6)
	at androidx.test.espresso.ViewInteraction$1.call(ViewInteraction.java:1)
	at java.util.concurrent.FutureTask.run(FutureTask.java:266)
	at android.os.Handler.handleCallback(Handler.java:883)
	at android.os.Handler.dispatchMessage(Handler.java:100)
	at android.os.Looper.loop(Looper.java:237)
	at android.app.ActivityThread.main(ActivityThread.java:8016)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:496)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1076)
	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@28eab1e, Dispatchers.Main]
Status: NEW → ASSIGNED

► 09/18 Firebase link

► 10/6
Pixel 6, API Level 31 Firebase link

Closing because no crashes reported for 12 weeks.

Status: ASSIGNED → RESOLVED
Closed: 1 years ago
Resolution: --- → WORKSFORME

Robo test, Pixel 6, API Level 31

Updated stack:

java.lang.NullPointerException
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.getBookmarkInteractor(BookmarkFragment.kt:72)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.refreshBookmarks(BookmarkFragment.kt:297)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment.access$refreshBookmarks(BookmarkFragment.kt:65)
	at org.mozilla.fenix.library.bookmarks.BookmarkFragment$refreshBookmarks$1.invokeSuspend(Unknown Source:14)
	at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:33)
	at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:104)
	at android.os.Handler.handleCallback(Handler.java:938)
	at android.os.Handler.dispatchMessage(Handler.java:99)
	at android.os.Looper.loopOnce(Looper.java:201)
	at android.os.Looper.loop(Looper.java:288)
	at android.app.ActivityThread.main(ActivityThread.java:7839)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:548)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:1003)
	Suppressed: kotlinx.coroutines.internal.DiagnosticCoroutineContextException: [StandaloneCoroutine{Cancelling}@332bb7a, Dispatchers.Main]
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---

1 failures in 3805 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

P5 because this is a real bug, but we've only seen 1 exception out of 3805 test pushes in the last 7 days and 0 crash reports from real users.

Priority: -- → P5
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: