Skip to content

Store registered context for sync task unregistrations #6752

New issue

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

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

Already on GitHub? # to your account

Merged
merged 2 commits into from
Mar 10, 2025

Conversation

welishr
Copy link
Contributor

@welishr welishr commented Mar 10, 2025

For issue #6558, this is an attempt at fixing the IllegalArgumentException by ensuring that the context we use for registering the SyncTask is the same context we use to unregister the task. Race conditions dont seem like a culprit here since unregister is only triggered by the Receiver itself, which should be only executed synchronously on the main thread.

New PR needed to fix CLA issue

Copy link
Contributor

github-actions bot commented Mar 10, 2025

📝 PRs merging into main branch

Our main branch should always be in a releasable state. If you are working on a larger change, or if you don't want this change to see the light of the day just yet, consider using a feature branch first, and only merge into the main branch when the code complete and ready to be released.

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 10, 2025

Coverage Report 1

Affected Products

  • firebase-messaging

    Overall coverage changed from 84.15% (aa2bab8) to 84.13% (c0a632f) by -0.02%.

    FilenameBase (aa2bab8)Merge (c0a632f)Diff
    SyncTask.java70.83%70.67%-0.17%

Test Logs

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

@google-oss-bot
Copy link
Contributor

google-oss-bot commented Mar 10, 2025

Size Report 1

Affected Products

  • firebase-messaging

    TypeBase (aa2bab8)Merge (c0a632f)Diff
    aar149 kB149 kB+49 B (+0.0%)
    apk (aggressive)578 kB578 kB-60 B (-0.0%)
    apk (release)5.31 MB5.31 MB+32 B (+0.0%)

Test Logs

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

Copy link
Contributor

github-actions bot commented Mar 10, 2025

Test Results

 64 files  + 56   64 suites  +56   6m 17s ⏱️ + 5m 58s
487 tests +465  487 ✅ +465  0 💤 ±0  0 ❌ ±0 
980 runs  +936  980 ✅ +936  0 💤 ±0  0 ❌ ±0 

Results for commit 678ad02. ± Comparison against base commit aa2bab8.

This pull request removes 22 and adds 487 tests. Note that renamed tests count towards both.
com.google.firebase.crashlytics.internal.common.CrashlyticsControllerRobolectricTest ‑ testDoCloseSession_disabledAnrs_doesNotPersistsAppExitInfo
com.google.firebase.crashlytics.internal.common.CrashlyticsControllerRobolectricTest ‑ testDoCloseSession_enabledAnrs_doesNotPersistsAppExitInfoIfItDoesntExist
com.google.firebase.crashlytics.internal.common.CrashlyticsControllerRobolectricTest ‑ testDoCloseSession_enabledAnrs_persistsAppExitInfoIfItExists
com.google.firebase.crashlytics.internal.common.DataCollectionArbiterRobolectricTest ‑ testDefaultDataCollection_usedWhenNoOverrideOrManifestSetting
com.google.firebase.crashlytics.internal.common.DataCollectionArbiterRobolectricTest ‑ testManifestMetadata_respectedWhenNoOverride
com.google.firebase.crashlytics.internal.common.DataCollectionArbiterRobolectricTest ‑ testSetCrashlyticsDataCollectionEnabled_overridesOtherSettings
com.google.firebase.crashlytics.internal.common.SessionReportingCoordinatorRobolectricTest ‑ testAppExitInfoEvent_notPersistIfAnrBeforeSession
com.google.firebase.crashlytics.internal.common.SessionReportingCoordinatorRobolectricTest ‑ testAppExitInfoEvent_notPersistIfAppExitInfoNotAnrButWithinSession
com.google.firebase.crashlytics.internal.common.SessionReportingCoordinatorRobolectricTest ‑ testAppExitInfoEvent_persistIfAnrWithinSession
com.google.firebase.crashlytics.internal.common.SessionReportingCoordinatorRobolectricTest ‑ testAppExitInfoEvent_persistIfAnrWithinSession_multipleAppExitInfo
…
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testNoWrappedIntent
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testNullIntent
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[21]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[22]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[23]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_notOButTargetingO[24]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_preO
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_preO[21]
com.google.firebase.iid.FirebaseInstanceIdWithFcmReceiverRoboTest ‑ testStartsService_preO[22]
…

♻️ This comment has been updated with latest results.

@welishr welishr merged commit af5fd66 into main Mar 10, 2025
37 checks passed
@welishr welishr deleted the messaging/fm_sync_task_fix branch March 10, 2025 19:56
tejasd pushed a commit that referenced this pull request Apr 1, 2025
For issue #6558, this is an attempt at fixing the
IllegalArgumentException by ensuring that the context we use for
registering the SyncTask is the same context we use to unregister the
task. Race conditions dont seem like a culprit here since unregister is
only triggered by the Receiver itself, which should be only executed
synchronously on the main thread.
@firebase firebase locked and limited conversation to collaborators Apr 10, 2025
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants