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.

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

Initial spec text for bounce tracking deletion timer. #37

Merged
merged 1 commit into from
Apr 5, 2023

Conversation

wanderview
Copy link
Collaborator

@wanderview wanderview commented Mar 30, 2023

This text roughs in the general shape of how deletion will occur based on a timer comparing against timestamps stored in a global map. Some details still need to be resolved.


Preview | Diff

@wanderview wanderview requested a review from jyasskin March 30, 2023 20:55
@wanderview
Copy link
Collaborator Author

@jyasskin PTAL. This is parented to the other PR. I'm not sure github handles that well so you may see changes from that other PR in this one as well. If you could give me a directional review of the new text that would be great. I'll be OOO till Tuesday and will try to address feedback then.

Copy link
Collaborator

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

This looks pretty good.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 371 to 377
1. If |bounceTime| + [=bounce tracking grace period=] is less than or equal to
|now|, then [=iteration/continue=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these are wall times, what happens if the user adjusts their system clock during the bounce tracking grace period? Since bounces would happen within a single user agent session, maybe some of this should use monotonic moments?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

While a single bounce may be within a single browsing session, the interactions may come in different browsing sessions. For that reason we can't really use monotonic time.

As a consequence if the user change's their clock significantly it could trigger deletion. The lifetime of user activation protection is expected to be much longer than the grace period or timer period, though. So only large changes are likely to cause this to happen.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the user sets their clock to earlier, I think all the activations and candidate bounces live longer than they're supposed to. So ... maybe we should have the bounce tracking timer check for |bounceTime| or |activationTime| being after |now|, and ... reset those to |now|? It's fine to leave that as a TODO in this PR.

@wanderview wanderview force-pushed the dev-timer branch 2 times, most recently from 0c2fe15 to f6aacbf Compare April 4, 2023 21:25
@wanderview
Copy link
Collaborator Author

I've tried to address action items. PTAL. Thanks!

Copy link
Collaborator

@jyasskin jyasskin left a comment

Choose a reason for hiding this comment

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

Looks good after the below comments.

index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated Show resolved Hide resolved
index.bs Outdated
Comment on lines 371 to 377
1. If |bounceTime| + [=bounce tracking grace period=] is less than or equal to
|now|, then [=iteration/continue=].
Copy link
Collaborator

Choose a reason for hiding this comment

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

If the user sets their clock to earlier, I think all the activations and candidate bounces live longer than they're supposed to. So ... maybe we should have the bounce tracking timer check for |bounceTime| or |activationTime| being after |now|, and ... reset those to |now|? It's fine to leave that as a TODO in this PR.

@wanderview
Copy link
Collaborator Author

If the user sets their clock to earlier, I think all the activations and candidate bounces live longer than they're supposed to. So ... maybe we should have the bounce tracking timer check for |bounceTime| or |activationTime| being after |now|, and ... reset those to |now|? It's fine to leave that as a TODO in this PR.

Hmm, I'm unsure if it's worth doing anything about this. I'll add a TODO item to consider it, but it seems unlikely users will be doing this at scale. And in terms of a single user its not catastrophic for deletion to be delayed based on a rare event.

This text roughs in the general shape of how deletion will occur based
on a timer comparing against timestamps stored in a global map.  Some
details still need to be resolved.
@wanderview wanderview merged commit 28b3e64 into privacycg:main Apr 5, 2023
github-actions bot added a commit that referenced this pull request Apr 5, 2023
SHA: 28b3e64
Reason: push, by wanderview

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants