[HOLD for payment 2023-05-03] [$1000] [Crash] Investigate EventEmitter.addEvent on Android

mountiny commented 1 year ago

Problem

coming from https://console.firebase.google.com/u/0/project/expensify-chat/crashlytics/app/android:com.expensify.chat/issues/4bd625f86a63554f09e632520a29dfe5?time=last-seven-days&sessionEventKey=643856AC016C00010CA3E2063908E8E8_1800062918688317541

in version 1.2.99-6 we are running into this error on Android, on mostly Sony devices related to the urban airship

Fatal Exception: java.lang.NoSuchMethodError: No interface method putIfAbsent(Ljava/lang/Object;Ljava/lang/Object;)Ljava/lang/Object; in class Ljava/util/Map; or its super classes (declaration of 'java.util.Map' appears in /system/framework/core-libart.jar)
       at com.urbanairship.android.framework.proxy.events.EventEmitter.addEvent(EventEmitter.kt:32)
       at com.urbanairship.android.framework.proxy.AirshipListener.onInboxUpdated(AirshipListener.kt:123)
       at com.urbanairship.messagecenter.Inbox$9.run(Inbox.java:763)
       at android.os.Handler.handleCallback(Handler.java:739)
       at android.os.Handler.dispatchMessage(Handler.java:95)
       at android.os.Looper.loop(Looper.java:211)
       at android.app.ActivityThread.main(ActivityThread.java:5371)
       at java.lang.reflect.Method.invoke(Method.java)
       at java.lang.reflect.Method.invoke(Method.java:372)
       at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:945)
       at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:740)

Why is it important

Its causing app to crash

solution

Investigate and try to fix this crash

Upwork Automation - Do Not Edit

Upwork Job URL: https://www.upwork.com/jobs/~01ef230aad8534b92a
Upwork Job ID: 1648082798749863936
Last Price Increase: 2023-04-17

MelvinBot commented 1 year ago

Job added to Upwork: https://www.upwork.com/jobs/~01ef230aad8534b92a

MelvinBot commented 1 year ago

Triggered auto assignment to @twisterdotcom (Bug), see https://stackoverflow.com/c/expensify/questions/14418 for more details.

MelvinBot commented 1 year ago

Triggered auto assignment to @twisterdotcom (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

MelvinBot commented 1 year ago

Bug0 Triage Checklist (Main S/O)

[ ] This "bug" occurs on a supported platform (ensure Platforms in OP are ✅)
[ ] This bug is not a duplicate report (check E/App issues and #expensify-bugs)
- If it is, comment with a link to the original report, close the issue and add any novel details to the original issue instead
[ ] This bug is reproducible using the reproduction steps in the OP. S/O
- If the reproduction steps are clear and you're unable to reproduce the bug, check with the reporter and QA first, then close the issue.
- If the reproduction steps aren't clear and you determine the correct steps, please update the OP.
[ ] This issue is filled out as thoroughly and clearly as possible
- Pay special attention to the title, results, platforms where the bug occurs, and if the bug happens on staging/production.
[ ] I have reviewed and subscribed to the linked Slack conversation to ensure Slack/Github stay in sync

MelvinBot commented 1 year ago

Triggered auto assignment to Contributor-plus team member for initial proposal review - @rushatgabhane (External)

MelvinBot commented 1 year ago

Triggered auto assignment to @youssef-lr (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

twisterdotcom commented 1 year ago

I do not know how to reproduce this. @mountiny should I just trust this and make it external?

twisterdotcom commented 1 year ago

Oh wait, you already did. Okay, cool. I'm just a money man. 😄

youssef-lr commented 1 year ago

@twisterdotcom External label has been applied. So we're good to go for now! :)

akinwale commented 1 year ago

Proposal

Please re-state the problem that we are trying to solve in this issue.

There is an app crash on mostly Sony devices related to urban airship.

What is the root cause of that problem?

The source of the problem is the putIfAbsent method being called by the airship-mobile-framework package.

https://github.com/urbanairship/airship-mobile-framework-proxy/blob/3e9cf017e4b878f21afccaf103658e9b882dc13f/android/airship-framework-proxy/src/main/java/com/urbanairship/android/framework/proxy/events/EventEmitter.kt#L32

The putIfAbsent method was introduced in API Level 24 (Android 7) according to the documentation for java.util.Map which is the base interface for maps. Trying to execute this on a device which is running a version of Android prior to 7 will result in this error. Thus, it makes sense that mostly Sony devices would be affected they haven't been diligent with major Android software updates. To verify, check the crash version being reported by Firebase.

What changes do you think we should make in order to solve the problem?

Update: With the release of react-native-airship 15.2.3 which incorporates a fix for the problem, we can simply update package.json with the new version number.

https://github.com/Expensify/App/blob/f54a8bd23018644d33114d1820f7e428f18c74f9/package.json#L67

We can either propose a change upstream to fix the issue. Changing the putIfAbsent call to just put should be adequate, since it's simply adding a key/value pair to a map, and if the key already exists, it will be overwritten. If it's absolutely necessary to check that the key exists, then we can use a combination of a !containsKey or get(key) == null check, and if the key doesn't exist or the key value is null, use the put method to add the key/value pair to the map.

Alternatively, if the change cannot be applied upstream, maintain a fork of the airship-mobile-framework-proxy package. This is a dependency for the react-native-airship package, so a fork of this would have to be maintained as well.

https://github.com/urbanairship/react-native-airship/blob/main/android/build.gradle#L153

What alternative solutions did you explore? (Optional)

This would be more restrictive, but it's possible to set 24 as the minSdkVersion for Android builds. This would prevent the app from being installed on devices running older versions of Android.

rushatgabhane commented 1 year ago

Asked if we support < Android 7, and if we should fix this issue https://expensify.slack.com/archives/C01GTK53T8Q/p1681782751971599

rushatgabhane commented 1 year ago

@akinwale Great find! Just to confirm, https://github.com/urbanairship/airship-mobile-framework-proxy is the repo where the bug is present?

The repo seems really active. It should be an easy fix.

akinwale commented 1 year ago

@akinwale Great find! Just to confirm, https://github.com/urbanairship/airship-mobile-framework-proxy is the repo where the bug is present?

The repo seems really active. It should be an easy fix.

Yes. From line 32 of the EventEmitter class.

Julesssss commented 1 year ago

Thanks all. I shared this in our Airship support channel 🤞

rlepinski commented 1 year ago

Thanks for the report, ill get this resolved this week.

MelvinBot commented 1 year ago

📣 @rlepinski! 📣

Hey, it seems we don’t have your contributor details yet! You'll only have to do this once, and this is how we'll hire you on Upwork. Please follow these steps:

Get the email address used to login to your Expensify account. If you don't already have an Expensify account, create one here. If you have multiple accounts (e.g. one for testing), please use your main account email.
Get the link to your Upwork profile. It's necessary because we only pay via Upwork. You can access it by logging in, and then clicking on your name. It'll look like this. If you don't already have an account, sign up for one here.
Copy the format below and paste it in a comment on this issue. Replace the placeholder text with your actual details.

Screen Shot 2022-11-16 at 4 42 54 PM

Format:

Contributor details
Your Expensify account email: <REPLACE EMAIL HERE>
Upwork Profile Link: <REPLACE LINK HERE>

Julesssss commented 1 year ago

Thanks, @rlepinski!!

Also sorry about our bot, please ignore 😄

akinwale commented 1 year ago

Anything I need to do here regarding my proposal or do I wait until the airship code updates are completed / shipped?

rlepinski commented 1 year ago

@akinwale I am updating the code to remove putIfAbsent so it will be backwards compatible.

We cant just do a simple put since the code is tracking pending events that the plugin is unable to send yet (no listener). The list is needed since we might have multiple events of the same type. Your proposal of doing a check for null first would of worked, but since we are in Kotlin I used a handy getOrPut method. Here is the change - https://github.com/urbanairship/airship-mobile-framework-proxy/pull/7/files

I will have a fix out today, so I think the path forward is to just wait for the release and update to resolve this issue.

rlepinski commented 1 year ago

Airship 15.2.3 release is out

Julesssss commented 1 year ago

Thanks for the speedy update Ryan!

Okay, @akinwale would you like to update your proposal to simply update the library?

akinwale commented 1 year ago

Thanks for the speedy update Ryan!

Okay, @akinwale would you like to update your proposal to simply update the library?

Done.

MelvinBot commented 1 year ago

📣 @akinwale You have been assigned to this job by @Julesssss! Please apply to this job in Upwork and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻 Keep in mind: Code of Conduct | Contributing 📖

akinwale commented 1 year ago

@mountiny Are there any exact steps to reproducing the crash? I'm assuming since Airship is meant for push messaging, it has to do with notifications, but I am not familiar enough with the system to be sure.

Julesssss commented 1 year ago

In this case we'll just have to monitor Firebase to ensure the exception stops occuring.

akinwale commented 1 year ago

In this case we'll just have to monitor Firebase to ensure the exception stops occuring.

Thanks for the response.

I have applied to the Upwork job and the PR is ready for review.

mountiny commented 1 year ago

What Jules said, thanks for handling and thanks for the fix!

MelvinBot commented 1 year ago

Reviewing label has been removed, please complete the "BugZero Checklist".

MelvinBot commented 1 year ago

The solution for this issue has been :rocket: deployed to production :rocket: in version 1.3.5-6 and is now subject to a 7-day regression period :calendar:. Here is the list of pull requests that resolve this issue:

https://github.com/Expensify/App/pull/17655

If no regressions arise, payment will be issued on 2023-05-03. :confetti_ball:

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

[x] External issue reporter
[x] Contributor that fixed the issue
[x] Contributor+ that helped on the issue and/or PR

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

Merged PR within 3 business days of assignment - 50% bonus
Merged PR more than 9 business days after assignment - 50% penalty

MelvinBot commented 1 year ago

BugZero Checklist: The PR fixing this issue has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

[x] [@rushatgabhane] The PR that introduced the bug has been identified. Link to the PR:
[x] [@rushatgabhane] The offending PR has been commented on, pointing out the bug it caused and why, so the author and reviewers can learn from the mistake. Link to comment:
[x] [@rushatgabhane] A discussion in #expensify-bugs has been started about whether any other steps should be taken (e.g. updating the PR review checklist) in order to catch this type of bug sooner. Link to discussion:
[x] [@rushatgabhane] Determine if we should create a regression test for this bug.
[x] [@rushatgabhane] If we decide to create a regression test for the bug, please propose the regression test steps to ensure the same bug will not reach production again.
[x] [@twisterdotcom] Link the GH issue for creating/updating the regression test once above steps have been agreed upon:

akinwale commented 1 year ago

@Julesssss @rushatgabhane @twisterdotcom Just checking in here regarding next steps and payment. Thanks.

Julesssss commented 1 year ago

Thanks for the bump @akinwale. Looks like this was unblocked for payment yesterday.

@twisterdotcom @rushatgabhane I checked off all the regression items because the regression occurred on an external library.

rushatgabhane commented 1 year ago

thanks :) @Julesssss

twisterdotcom commented 1 year ago

Sorry paid @akinwale. Offer sent to @rushatgabhane: https://www.upwork.com/nx/wm/pre-hire/c/8577561/offer/24422081

youssef-lr commented 1 year ago

Are we good to close this @rushatgabhane?

rushatgabhane commented 1 year ago

@twisterdotcom https://expensify.slack.com/archives/C02NK2DQWUX/p1683614947522329

twisterdotcom commented 1 year ago

Offer resent to the correct profile!

twisterdotcom commented 1 year ago

Paid!

Expensify / App