Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[Fix] Network Delayed Activity Trampoline #1583

Merged
merged 3 commits into from
May 14, 2022
Merged

[Fix] Network Delayed Activity Trampoline #1583

merged 3 commits into from
May 14, 2022

Conversation

jkasten2
Copy link
Member

@jkasten2 jkasten2 commented May 8, 2022 •
edited
Loading

Description

One Line Summary

Removes a network call dependency on notification open logic which effects foregrounding the app.

Details

Motivation

  • After the changes in PR [Change] Notification Activity Trampoline forward instead of reverse #1581 this becomes critical this is resolved so the app still foregrounds when tapping on a notification while offline.
  • This also resolves a Xiaomi compatibility issue where if the app is swiped away then a notification is opened it won't foreground either. unless something starts the app process in the background later and is still running when you open the notification oddly enough

Scope

Target scope is the fix the bug noted above.
A secondary fix was made to improve the app shutdown process to stop retrying to get remote params when its thread is interrupted. This fix was only required in this PR to resolve a test carry over issue with the test added in this PR.

Background

The fixes in this PR fixes a discovered issue in 4.0.0 to 4.4.x, but is not an issue in 4.5.0 to 4.7.2. However this fix needs to be shipped with #1581 as it reverts most of the changes in #1377 that was shipped in 4.5.0.

Xiaomi - Background Restrictions

The following was observed through testing on a Xiaomi Redmi 6A running Android 9 on MIUI Global 11.0.8 | Stable 11.0.8.0 (PCBMIXM):

  • Activity Trampling - Must keep an Activity alive while trampolining
    • If the app was swiped away AND the app process is not currently running, then a notification is tapped on that has a PendingIntent to start an Activity and then you trampoline to another Activity you MUST call startActivity BEFORE finishing the first Activity. Otherwise the startActivity is ignored.

Testing

Unit testing

  • A new test was added.

Manual testing

Tested on:

  • Android 12 - Samsung S21 - OneUI 4.1
  • Android 9 - Xiaomi Redmi 6A - MIUI 11

Test both offline and online.

Affected code checklist

  • Notifications
    • Display
    • Open
    • Push Processing
    • Confirm Deliveries
  • Outcomes
  • Sessions
  • In-App Messaging
  • REST API requests
    • Remote Parameters
  • Public API changes

Checklist

Overview

  • I have filled out all REQUIRED sections above
  • PR does one thing
  • Any Public API changes are explained in the PR details and conform to existing APIs

Testing

  • I have included test coverage for these changes, or explained why they are not needed
  • All automated tests pass, or I explained why that is not possible
  • I have personally tested this on my device, or explained why that is not possible

Final pass

  • Code is as readable as possible.
  • I have reviewed this PR myself, ensuring it meets each checklist item

This change is Reviewable

@jkasten2 jkasten2 requested a review from a team May 8, 2022 06:51
@jkasten2 jkasten2 changed the title Fix Network Delayed Activity Trampoline [Fix] Network Delayed Activity Trampoline May 8, 2022
@jkasten2 jkasten2 mentioned this pull request May 8, 2022
Merged
29 tasks
nan-li
nan-li approved these changes May 14, 2022
View reviewed changes
Copy link
Contributor

@nan-li nan-li left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can understand the phone offline -> tap notification fix.

I can't comprehend the Xiaomi part of the fix. Is it something like handleNotificationOpen is added to the task queue, but it's removed before it gets to run?

@jkasten2
Copy link
Member Author

jkasten2 commented May 14, 2022 •
edited
Loading

I can't comprehend the Xiaomi part of the fix. Is it something like handleNotificationOpen is added to the task queue, but it's removed before it gets to run?

I added a new section " Xiaomi - Background Restrictions" above to explain the Xiaomi limitation in more detail.

The task queue is ok, you can call startActivity from another thread. The issue is that doing so means startActivity is called AFTER finish() is called in NotificationOpenedReceiverBase.kt.

@nan-li
Copy link
Contributor

nan-li commented May 14, 2022

startActivity is called AFTER finish() is called in NotificationOpenedReceiverBase.kt

Ahhhh got it, thanks!

@jkasten2 jkasten2 force-pushed the change/notification_activity_trampoline_forward_instead_of_reverse branch from 41c6ab8 to 1c3e69e Compare May 14, 2022 19:50
@jkasten2
Test reproducing delay on a network request
c067ae4 
We mock fail the network request to reproduce the issue where the app
does not get foregrounded.
@jkasten2
Don't retry get params on thread interruption
89fb595 
If the app process is shutting down we don't want to keep retrying the
remote params network request. No real world problem have been
discovered but it's possible it could be create some unexpected states
while the app process is being shutdown.

In the context of our tests, this also fixes an issue where this
retrying does not stop when we mock fail this network request. This
oddly enough didn't seem to be an issue for the one pre-existing test
using failGetParams but became a consistent test carry over issue when a
2nd test used this, which was added in the last commit.
@jkasten2
rm waiting on params network call on notifi open
23cc934 
This fixes a bug where the app would not be brought to the foreground
when tapping on a notification if the app could not connect to
onesignal.com or was offline during the full life time of the app
process.
This also fixes another a compatibility issue with Xiaomi devices where
the app would not foreground if the app process was dead and also no
task exists in the recent list.

It is critical we don't wait for a network call for the notification
open logic this method does the work to bring the app to the foreground.
This get params network call isn't needed before we handle the
notification open logic anyway.

This network wait was present in 4.0.0 through 4.4.x but was not an
issue in 4.5.0 when a "Reverse Activity Trampoline" was setup. However
in PR #1581 we are switching back to a standard Activity Trampoline and
we don't want to reintroduce this bug.

A failing test was added for this in a previous commit, which now
passes.
@jkasten2 jkasten2 force-pushed the fix/delayed_activity_trampoline branch from 3f6712b to 23cc934 Compare May 14, 2022 21:04
@jkasten2
Copy link
Member Author

This branch is based on change/notification_activity_trampoline_forward_instead_of_reverse, however it was rebased to --autosquash a --fixup commit. That rebase caused this branch to become out of date, with github showing a merge conflict and commits from both branches showing as the diff. To fix this the following was run:

git checkout fix/delayed_activity_trampoline
git rebase --onto change/notification_activity_trampoline_forward_instead_of_reverse origin/change/notification_activity_trampoline_forward_instead_of_reverse@{1}

origin and @{1} are important here as they are the previous known git reflog on github before it was rebased.

Base automatically changed from change/notification_activity_trampoline_forward_instead_of_reverse to main May 14, 2022 21:31
@jkasten2 jkasten2 merged commit 5f40a60 into main May 14, 2022
@jkasten2 jkasten2 deleted the fix/delayed_activity_trampoline branch May 14, 2022 21:32
This was referenced May 16, 2022
Closed
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nan-li nan-li nan-li approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jkasten2 @nan-li