1826590 - Update to Kotlin coroutines 1.7.1 when released

Status: VERIFIED FIXED

(Fenix :: General, task)

Description

[Jonathan Almeida [:jonalmeida]]

When investigating the crash logged in application-services#5464, Christian and I observed that there was no crash reporter showing up during this crash.

What we found is that the crash reporter was silently crashing:

java.lang.RuntimeException: Parcelable encountered IOException writing serializable object (name = mozilla.appservices.places.uniffi.PlacesApiException$UnexpectedPlacesException)
     at android.os.Parcel.writeSerializable(Parcel.java:1714)
     at android.os.Parcel.writeValue(Parcel.java:1662)
     at android.os.Parcel.writeArrayMapInternal(Parcel.java:875)
     at android.os.BaseBundle.writeToParcelInner(BaseBundle.java:1579)
     at android.os.Bundle.writeToParcel(Bundle.java:1233)
     at android.os.Parcel.writeBundle(Parcel.java:915)
     at android.os.Parcel.writeValue(Parcel.java:1580)
     at android.os.Parcel.writeArrayMapInternal(Parcel.java:875)
     at android.os.BaseBundle.writeToParcelInner(BaseBundle.java:1579)
     at android.os.Bundle.writeToParcel(Bundle.java:1233)
     at android.os.Parcel.writeBundle(Parcel.java:915)
     at android.content.Intent.writeToParcel(Intent.java:9961)
     at android.app.IActivityManager$Stub$Proxy.startService(IActivityManager.java:4287)
     at android.app.ContextImpl.startServiceCommon(ContextImpl.java:1562)
     at android.app.ContextImpl.startForegroundService(ContextImpl.java:1538)
     at android.content.ContextWrapper.startForegroundService(ContextWrapper.java:669)
     at androidx.core.content.ContextCompat$Api26Impl$$ExternalSyntheticApiModelOutline1.m(R8$$SyntheticClass:0)
     at androidx.core.content.ContextCompat$Api26Impl.startForegroundService(ContextCompat.java:1091)
     at androidx.core.content.ContextCompat.startForegroundService(ContextCompat.java:749)
     at mozilla.components.lib.crash.CrashReporter.sendCrashTelemetry$lib_crash_release(CrashReporter.kt:275)
     at mozilla.components.lib.crash.CrashReporter.onCrash$lib_crash_release(CrashReporter.kt:221)
     at mozilla.components.lib.crash.handler.ExceptionHandler.uncaughtException(ExceptionHandler.kt:40)
     at java.lang.ThreadGroup.uncaughtException(ThreadGroup.java:1068)
     at java.lang.ThreadGroup.uncaughtException(ThreadGroup.java:1063)
     at kotlinx.coroutines.CoroutineExceptionHandlerImplKt.handleCoroutineExceptionImpl(CoroutineExceptionHandlerImpl.kt:61)
     at kotlinx.coroutines.CoroutineExceptionHandlerKt.handleCoroutineException(CoroutineExceptionHandler.kt:33)
     at kotlinx.coroutines.StandaloneCoroutine.handleJobException(Builders.common.kt:196)
     at kotlinx.coroutines.JobSupport.finalizeFinishingState(JobSupport.kt:229)
     at kotlinx.coroutines.JobSupport.tryMakeCompletingSlowPath(JobSupport.kt:906)
     at kotlinx.coroutines.JobSupport.tryMakeCompleting(JobSupport.kt:863)
     at kotlinx.coroutines.JobSupport.makeCompletingOnce$kotlinx_coroutines_core(JobSupport.kt:828)
     at kotlinx.coroutines.AbstractCoroutine.resumeWith(AbstractCoroutine.kt:100)
     at kotlin.coroutines.jvm.internal.BaseContinuationImpl.resumeWith(ContinuationImpl.kt:46)
     at kotlinx.coroutines.DispatchedTask.run(DispatchedTask.kt:104)
     at android.os.Handler.handleCallback(Handler.java:873)
     at android.os.Handler.dispatchMessage(Handler.java:99)
     at android.os.Looper.loop(Looper.java:193)
     at android.app.ActivityThread.main(ActivityThread.java:6669)
     at java.lang.reflect.Method.invoke(Native Method)
     at com.android.internal.os.RuntimeInit$MethodAndArgsCaller.run(RuntimeInit.java:493)
     at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:858)
Caused by: java.io.NotSerializableException: kotlinx.coroutines.StandaloneCoroutine
     at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1240)
     at java.io.ObjectOutputStream.writeArray(ObjectOutputStream.java:1434)
     at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1230)
     at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1604)
     at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1565)
     at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1488)
     at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1234)
     at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1604)
     at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1565)
     at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1488)
     at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1234)
     at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:354)
     at java.util.ArrayList.writeObject(ArrayList.java:762)
     at java.lang.reflect.Method.invoke(Native Method)
     at java.io.ObjectStreamClass.invokeWriteObject(ObjectStreamClass.java:1037)
     at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1552)
     at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1488)
     at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1234)
     at java.io.ObjectOutputStream.defaultWriteFields(ObjectOutputStream.java:1604)
     at java.io.ObjectOutputStream.defaultWriteObject(ObjectOutputStream.java:463)
     at java.lang.Throwable.writeObject(Throwable.java:977)
     at java.lang.reflect.Method.invoke(Native Method)
     at java.io.ObjectStreamClass.invokeWriteObject(ObjectStreamClass.java:1037)
     at java.io.ObjectOutputStream.writeSerialData(ObjectOutputStream.java:1552)
     at java.io.ObjectOutputStream.writeOrdinaryObject(ObjectOutputStream.java:1488)
     at java.io.ObjectOutputStream.writeObject0(ObjectOutputStream.java:1234)
     at java.io.ObjectOutputStream.writeObject(ObjectOutputStream.java:354)
     at android.os.Parcel.writeSerializable(Parcel.java:1709)
     ... 40 more

After some (painful) investigation, we found that a kotlin coroutine exception within the stacktrace is not Serializable by default and causes the crash when we try to serialize and send our crash report.

From our analysis, this seems to be limited to Places exceptions that throw UnexpectedPlacesException.

This issue has already been reported and fixed in the coroutine library version 1.7.0 and we were able to test that this does indeed fix the issue with a pre-release version (1.7.0-Beta).

Once a stable release is made, we should update to that version and re-test that we are not taking down the crash reporter any more.

Comment 1

[Jonathan Almeida [:jonalmeida]]

The kotlin coroutine library 1.7.0 is released!

Comment 2

[Jonathan Almeida [:jonalmeida]]

ryanvm has been working on a patch on the side with the RC version that requires some more changes to get CI working with some of the breaking changes in the release.

Comment 3

[Ryan VanderMeulen [:RyanVM]]

Only one issue I'm seeing so far:

w: file:///builds/worker/checkouts/vcs/android-components/components/support/test/src/main/java/mozilla/components/support/test/rule/Helpers.kt:56:5 'runTest(CoroutineContext = ..., Long, suspend TestScope.() -> Unit): TestResult /* = Unit */' is deprecated. Define a total timeout for the whole test instead of using dispatchTimeoutMs. Warning: the proposed replacement is not identical as it uses 'dispatchTimeoutMs' as the timeout for the whole test!

Comment 4

[Ryan VanderMeulen [:RyanVM]]

Mihai, do you have any thoughts on the above failure? This looks like fallout from:

Add the timeout parameter to runTest for the whole-test timeout, 10 seconds by default (#3270). This replaces the configuration of quiescence timeouts, which is now deprecated (#3603).

Seems like we need to refactor Helpers.kt a bit to account for that. And then find out what impact it has on tests afterwards.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

BTW, v1.7.1 is out now but the failures are the same.

Comment 6

[Mihai Adrian Carare [:mcarare]]

Attachment: coroutines patch

The error just hides other possible errors. See the comment below for replacing the deprecated method.

Comment 7

[Mihai Adrian Carare [:mcarare]]

The replacing method asks for the total test time timeout. We can first keep the current timeout and change

dispatchTimeoutMs: Long = DEFAULT_DISPATCH_TIMEOUT_MS to
testTimeoutMs: Duration = DEFAULT_DISPATCH_TIMEOUT_SECONDS

and

private const val DEFAULT_DISPATCH_TIMEOUT_MS = 60_000L to
private val DEFAULT_DISPATCH_TIMEOUT_SECONDS = 60.seconds

After that, we should get to see other test failures ( if any).

Comment 8

[Ryan VanderMeulen [:RyanVM]]

Attachment: test failures

Mihai's suggestion gets the builds working \m/. There are a handful of unit test failures, though. One Fenix and five in AC.

Comment 9

[BMO Github Automation]

Attachment: [mozilla-mobile/firefox-android] Bug 1826590 - Update Kotlin Coroutines to version 1.7.1 (#2262)

Comment 10

[BMO Github Automation]

Authored by https://github.com/rvandermeulen
https://github.com/mozilla-mobile/firefox-android/commit/1e466516e3f86a5f94882a492124aa3cc176ce26
[main] Bug 1826590 - Update Kotlin Coroutines to version 1.7.1

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/9d1d42b0cb98b8af04389f3149faeff6aea81c5b
[main] Bug 1826590 - Run test job on Main.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/f6c626b26d3cd97f4883c5494b45c9e6a36cc0c3
[main] Bug 1826590 - Fix UncaughtExceptionsBeforeTest in MediaSessionFullscreenFeatureTest.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/fcb54f05a41c3dc83dbbe0a7475598651854e7ce
[main] Bug 1826590 - Fix ClassCastException in ShareControllerTest.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/4d06652830c9c1e971b5e2ef72244b5761d20d54
[main] Bug 1826590 - Fix exceptions in TabsTrayInfoBannerBindingTest.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/bf0dce1095c9d704616f07f8172aea6c0631de6c
[main] Bug 1826590 - Use ShadowFileProvider for all tests.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/7ec517e0183e5c10408962118741dff21d0fae56
[main] Bug 1826590 - Mock download response.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/0a45cd5bb013aebd2b6260a376edad610227556c
[main] Bug 1826590, 1794861 - Do not reset Dispatchers.Main.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/49653d82b2c353c770fc8d3a74aecbec4a14851a
[main] Bug 1826590 - Use gleanTestRule as RuleChain outerRule.

Authored by https://github.com/mcarare
https://github.com/mozilla-mobile/firefox-android/commit/8e8deb148317b3772e436644dcdb76d7f9de654f
[main] Bug 1826590 - Mock service with spy BrowserStore.

Comment 11

[Mihai Adrian Carare [:mcarare]]

For QA, there is nothing specific to test related to this bug, a full run would be the only choice.
This change is not expected to modify app behavior.
Also, any particular bug would be hard to pin on this upgrade, as it would involve analyzing the stack trace.

Comment 12

[Oana Horvath [:ohorvath]]

QA: Based on the automation coverage, there is no serious concern about this. We'll wait for the Beta 116 full-functional manual testing and report back if we find anything else.

Comment 13

[Oana Horvath [:ohorvath]]

QA has finished the full-functional testing on Beta with no critical issues found. I'll close this as verified.