Skip to content

Finish message broadcast after sending Intent to bound Service. #3919

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
Merged
merged 2 commits into from
Jul 20, 2022
Merged

Finish message broadcast after sending Intent to bound Service. #3919

merged 2 commits into from
Jul 20, 2022

Conversation

gsakakihara
Copy link
Contributor

  • Changed to finish the message broadcast as soon as the Intent is sent to the FirebaseMessagingService when binding to the Service. This should reduce the chance that the BroadcastReceiver ANRs in certain circumstances.
  • Acquired a WakeLock when using bindService() to send an Intent to FirebaseMessagingService to prevent the device from sleeping while processing the Intent now that the broadcast is finished before the Service has finished handling it.
    • Holding the WakeLock for up to 60 seconds for high priority messages to match the behavior for high priority messages when using startService().
    • Holding the WakeLock for up to 9 seconds for normal priority messages to match the previous behavior where the broadcast was not forced to finish for 9 seconds.

@gsakakihara
Finish message broadcast after sending Intent to bound Service.
aa5785a 
* Changed to finish the message broadcast as soon as the Intent is sent to the FirebaseMessagingService when binding to the Service. This should reduce the chance that the BroadcastReceiver ANRs in certain circumstances.
* Acquired a WakeLock when using bindService() to send an Intent to FirebaseMessagingService to prevent the device from sleeping while processing the Intent now that the broadcast is finished before the Service has finished handling it.
  - Holding the WakeLock for up to 60 seconds for high priority messages to match the behavior for high priority messages when using startService().
  - Holding the WakeLock for up to 9 seconds for normal priority messages to match the previous behavior where the broadcast was not forced to finish for 9 seconds.
@github-actions
Copy link
Contributor

github-actions bot commented Jul 19, 2022 •
edited
Loading

Unit Test Results

474 tests  +474   474 ✔️ +474   4m 6s ⏱️ + 4m 6s
  27 suites +  27       0 💤 ±    0 
  27 files   +  27       0 ❌ ±    0 

Results for commit 11a0089. ± Comparison against base commit 905eeb8.

♻️ This comment has been updated with latest results.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 19, 2022 •
edited
Loading

Coverage Report 1

Affected Products

  • firebase-messaging

    Overall coverage changed from 84.79% (ef964ec) to 85.11% (9ee6d80) by +0.32%.

    FilenameBase (ef964ec)Merge (9ee6d80)Diff
    FcmBroadcastProcessor.java83.33%93.18%+9.85%
    MessagingAnalytics.java81.38%82.19%+0.81%
    WakeLockHolder.java95.00%96.08%+1.08%
    WithinAppServiceConnection.java83.75%84.15%+0.40%

Test Logs

Notes

  • Commit (9ee6d80) is created by Prow via merging PR base commit (ef964ec) and head commit (11a0089).
  • Run gradle <product>:checkCoverage to produce HTML coverage reports locally. After gradle commands finished, report files can be found under <product-build-dir>/reports/jacoco/.

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/FwtDdo166h.html

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Jul 19, 2022 •
edited
Loading

Size Report 1

Affected Products

  • firebase-messaging

    TypeBase (ef964ec)Merge (9ee6d80)Diff
    aar142 kB142 kB+865 B (+0.6%)
    apk (aggressive)438 kB438 kB+248 B (+0.1%)
    apk (release)1.15 MB1.15 MB+168 B (+0.0%)

Test Logs

Notes

  • Commit (9ee6d80) is created by Prow via merging PR base commit (ef964ec) and head commit (11a0089).

  1. https://storage.googleapis.com/firebase-sdk-metric-reports/6xbnCtVzyM.html

James201311
James201311 approved these changes Jul 19, 2022
View reviewed changes
Copy link
Member

@James201311 James201311 left a comment

Choose a reason for hiding this comment

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

LGTM!! 🚢🚢🚢

@gsakakihara
Acquire WakeLock before sending Intent to bound Service.
11a0089 
* Acquired the WakeLock in sendWakefulServiceIntent() before sending the Intent to the Service to avoid a race condition where the service could finish handling the Intent and call completeWakefulIntent() (trying to release the WakeLock) before the WakeLock was acquired.
@gsakakihara gsakakihara merged commit fe17099 into master Jul 20, 2022
@gsakakihara gsakakihara deleted the gsakakihara/anr branch July 20, 2022 20:52
@shashachu
Copy link

Do we know if this is related to either of these ANR issues reported? #2014 #3052

@gsakakihara
Copy link
Contributor Author

#3052 No, that one was happening during Application initialization, before the FCM SDK has a chance to do anything.

#2014 That issue has a lot of different reports that seem to have a number of causes. This would not help the original report for example (the ANR happened during FirebaseAppImpl.getInstance(), which is also before the FCM SDK does anything), but might improve some of the other reports. It's difficult to tell without more information.

lfkellogg pushed a commit that referenced this pull request Aug 5, 2022
@gsakakihara @lfkellogg
Finish message broadcast after sending Intent to bound Service. (#3919)
9db64b8 
* Changed to finish the message broadcast as soon as the Intent is sent to the FirebaseMessagingService when binding to the Service. This should reduce the chance that the BroadcastReceiver ANRs in certain circumstances.
* Acquired a WakeLock when using bindService() to send an Intent to FirebaseMessagingService to prevent the device from sleeping while processing the Intent now that the broadcast is finished before the Service has finished handling it.
  - Holding the WakeLock for up to 60 seconds for high priority messages to match the behavior for high priority messages when using startService().
  - Holding the WakeLock for up to 9 seconds for normal priority messages to match the previous behavior where the broadcast was not forced to finish for 9 seconds.
@firebase firebase locked and limited conversation to collaborators Aug 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@James201311 James201311 James201311 approved these changes

@willxu-google willxu-google Awaiting requested review from willxu-google

@welishr welishr Awaiting requested review from welishr

Assignees
No one assigned
Labels
size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gsakakihara @google-oss-bot @shashachu @James201311