Migrate to UserDefaults based heartbeat on tvOS #7893

ryanwilson · 2021-04-14T02:16:40Z

This should mitigate heartbeat files being wiped due to tvOS cache being
invalidated. Fixes #6662

#no-changelog

ryanwilson · 2021-04-14T02:18:09Z

Package.swift

-      "7.2.1" ..< "8.0.0"
+      // TODO: Move from `revision` to version below before publishing.
+      .revision("a59fd0181d8f7c2b4c815cd6cddf4c38a4ba35ee")
+//      "7.4.0" ..< "8.0.0"


This tag won't be available until we publish GoogleUtilities.

cc @paulb777 - how did we handle this in the past?

We either need to publish GoogleUtilities sooner or update this later in the release cycle.

There are some options to explore from https://forums.swift.org/t/best-practices-for-integration-testing-and-releasing-multiple-packages/47006/11, but nothing too clear or easy.

Happy to hold off temporarily and cherry-pick this when it's published. We should also probably set up a presubmit that prevents .revision from being checked in to prevent failures in the future

ryanwilson · 2021-04-14T02:19:31Z

FirebaseCore/Sources/FIRHeartbeatInfo.m

+    NSString *const kHeartbeatStorageName = @"HEARTBEAT_INFO_STORAGE";
+    id<GULHeartbeatDateStorable> dataStorage;
+#if TARGET_OS_TV
+    NSString *const kFIRCoreSuiteName = @"com.firebase.core";


I can stick with standardUserDefaults but felt like I should move away from it. Thoughts on this domain?

A custom domain looks reasonable as there is less probability that it will be modified by developers for their purposes. "com.firebase.core" is pretty generic name, so I would either declare a more specific one for the heartbeat use only or at least define a constant available for the rest of Core if we would like to reuse the same defaults for other purposes. I would prefer the more specific name.

I did intentionally name it so Core could use it in the future for other things, since I don't think we need a new suite name for every piece of data we need to store (that's what the keys are for). Breaking it out into a constant would make sense in that case then, and other SDKs could follow suit eventually (com.firebase.auth, .firestore, etc).

Does that sound reasonable?

SGTM. The only potential issue with reuse may be that the same defaults may potentially be modified from different parts of the code that may be easy to overlook. It seems to me that having extra user defaults is a relatively cheap mitigation of this issue. I'm fine with either for this PR.

Yeah, we currently use a single user defaults (standardUserDefaults) anyways so this wouldn't be anything new, and we should provide a unique key for each usage point as we currently do.

yeah, makes sense. Looks safe for now.

maksymmalyhin

LGTM

maksymmalyhin · 2021-04-14T13:54:12Z

FirebaseCore/Sources/FIRHeartbeatInfo.m

+    NSString *const kHeartbeatStorageName = @"HEARTBEAT_INFO_STORAGE";
+    id<GULHeartbeatDateStorable> dataStorage;
+#if TARGET_OS_TV
+    NSString *const kFIRCoreSuiteName = @"com.firebase.core";


A custom domain looks reasonable as there is less probability that it will be modified by developers for their purposes. "com.firebase.core" is pretty generic name, so I would either declare a more specific one for the heartbeat use only or at least define a constant available for the rest of Core if we would like to reuse the same defaults for other purposes. I would prefer the more specific name.

ryanwilson · 2021-04-14T14:56:35Z

Will wait until GoogleUtilities 7.4.0 publishes so we can have the proper Package.swift land here, then I'll merge. Thanks for the reviews!

google-oss-bot · 2021-04-14T15:36:38Z

Coverage Report

Affected SDKs

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

SDK overall coverage did not change between base commit (d937b2f) and head commit (dd6c93c). However there are changes in individual files.

Filename	Base (`d937b2f`)	Head (`dd6c93c`)	Diff
exception.cc	65.85%	29.27%	-36.59%
leveldb_key.cc	96.79%	98.39%	+1.61%
ordered_code.cc	93.24%	93.80%	+0.56%
task.cc	95.21%	97.60%	+2.40%
write_stream.cc	95.65%	93.48%	-2.17%

Test Logs

Base (d937b2f5): https://github.com/firebase/firebase-ios-sdk/actions/runs/789512447
Head (dd6c93c0): https://github.com/firebase/firebase-ios-sdk/actions/runs/789966653

paulb777 · 2021-04-16T16:29:27Z

I moved the milestone to 8.0.0 since the update did not make it into 7.11.0.

ncooke3 · 2021-04-27T14:26:52Z

Kicked off a rerun of CI to test with GoogleUtils 7.4 (that is staged in SpecsStaging atm)

This should mitigate heartbeat files being wiped due to tvOS cache being invalidated.

ncooke3 · 2021-04-27T17:44:28Z

Package.swift

      name: "GoogleUtilities",
      url: "https://github.com/google/GoogleUtilities.git",
-      "7.3.0" ..< "8.0.0"
+      "7.4.0" ..< "8.0.0"


@paulb777 @ryanwilson tagged GoogleUtils 7.4.0 for SwiftPM (on GoogleUtils repo) and am now pointing to GoogleUtils 7.4 in the Package manifest here.

ryanwilson

LGTM - thanks for the update!

google-cla bot added the cla: yes label Apr 14, 2021

google-oss-bot added the api: core label Apr 14, 2021

ryanwilson commented Apr 14, 2021

View reviewed changes

ryanwilson requested a review from maksymmalyhin April 14, 2021 13:02

ryanwilson added this to the 7.11.0 - M94 milestone Apr 14, 2021

maksymmalyhin approved these changes Apr 14, 2021

View reviewed changes

paulb777 modified the milestones: 7.11.0 - M94, 8.0.0 - M95 Apr 16, 2021

ncooke3 approved these changes Apr 19, 2021

View reviewed changes

ryanwilson added 2 commits April 27, 2021 12:12

Migrate to UserDefaults based heartbeat on tvOS

b7841ce

This should mitigate heartbeat files being wiped due to tvOS cache being invalidated.

Move defaults suite name to widely available constant.

b879789

ncooke3 force-pushed the rw-heartbeat-userdefaults branch from 5907aa6 to b879789 Compare April 27, 2021 16:14

Point to GULs 7.4.0+ in Package.swift

dd6c93c

ncooke3 requested a review from paulb777 April 27, 2021 17:42

ncooke3 reviewed Apr 27, 2021

View reviewed changes

ryanwilson commented Apr 27, 2021

View reviewed changes

paulb777 approved these changes Apr 27, 2021

View reviewed changes

ncooke3 merged commit 84a2a2c into master Apr 27, 2021

ncooke3 deleted the rw-heartbeat-userdefaults branch April 27, 2021 19:19

This was referenced Apr 28, 2021

FirebaseMessaging: Mock FIRHeartbeatInfo in test #7995

Merged

Implement app attest key ID storage #8014

Merged

firebase locked and limited conversation to collaborators May 28, 2021

Migrate to UserDefaults based heartbeat on tvOS #7893

Migrate to UserDefaults based heartbeat on tvOS #7893

Uh oh!

Conversation

ryanwilson commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maksymmalyhin Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanwilson Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maksymmalyhin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanwilson commented Apr 14, 2021

Uh oh!

google-oss-bot commented Apr 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Coverage Report

Affected SDKs

FirebaseFirestore-iOS-FirebaseFirestore.framework

Test Logs

Uh oh!

paulb777 commented Apr 16, 2021

Uh oh!

ncooke3 commented Apr 27, 2021

Uh oh!

ncooke3 Apr 27, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanwilson left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ryanwilson commented Apr 14, 2021 •

edited

Loading

maksymmalyhin Apr 14, 2021 •

edited

Loading

ryanwilson Apr 14, 2021 •

edited

Loading

google-oss-bot commented Apr 14, 2021 •

edited

Loading

`FirebaseFirestore-iOS-FirebaseFirestore.framework`

ncooke3 Apr 27, 2021 •

edited

Loading