Closed Bug 1826590 Opened 1 year ago Closed 1 year ago

Update to Kotlin coroutines 1.7.1 when released

Categories

(Fenix :: General, task)

All
Android

Tracking

(firefox116 verified)

VERIFIED FIXED
116 Branch
Tracking Status
firefox116 --- verified

People

(Reporter: jonalmeida, Assigned: mcarare)

References

(Regressed 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

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.

See Also: → 1826591

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.

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!

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.

Flags: needinfo?(mcarare)

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

Summary: Update to Kotlin coroutines 1.7.0 when released → Update to Kotlin coroutines 1.7.1 when released
Attached file coroutines patch (obsolete) —
The error just hides other possible errors. See the comment below for replacing the deprecated method.

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).

Flags: needinfo?(mcarare)
Attachment #9334782 - Attachment is obsolete: true
Attached file 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.

Assignee: nobody → mcarare
See Also: → 1836491

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.

Status: NEW → RESOLVED
Closed: 1 year ago
Flags: qe-verify+
Resolution: --- → FIXED
Target Milestone: --- → 116 Branch

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.

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.

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Regressions: 1844964
No longer regressions: 1844964
Regressions: 1844964
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: