[Otp4Bd] Introduce expiration closure to Subscription Manager [chromium/src : main]
Salma Aly (Gerrit)
Salma Aly has uploaded the change for review![]( "Open in Gerrit")
Commit message
[Otp4Bd] Introduce expiration closure to Subscription Manager
The OneTimeTokenServiceImpl currently handles subscriptions to OTPs
(both SMS and Gmail) via the generic ExpiringSubscriptionManager.
When a subscription reaches its timeout, it is silently dropped. This
works fine for SMS, but leaves Gmail OTP fillers (like the AttemptOtpFillingTool)
hanging indefinitely because no explicit action result is reported back.
To fix this without regressing the SMS OTP backend, this CL introduces
an optional `expiration_callback` to `ExpiringSubscriptionManager::Subscribe()`.
If provided, the manager will run it when the subscription is purged
during `ProcessExpirations()`.
Inside `OneTimeTokenServiceImpl::Subscribe`, we leverage this parameter
for Gmail sources by supplying a callback that yields the newly added
`OneTimeTokenRetrievalError::kSubscriptionExpired`. This allows the caller
to cleanly catch timeout signals while keeping the SMS flow untouched.
Bug: b/530535640
TAG=agy
Change diff
Change information
- M components/one_time_tokens/core/browser/one_time_token_retrieval_error.h
- M components/one_time_tokens/core/browser/one_time_token_service_impl.cc
- M components/one_time_tokens/core/browser/one_time_token_service_impl_unittest.cc
- M components/one_time_tokens/core/browser/util/expiring_subscription_manager.cc
- M components/one_time_tokens/core/browser/util/expiring_subscription_manager.h
- M components/one_time_tokens/core/browser/util/expiring_subscription_manager_unittest.cc
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Bekzhan Kassenov
- Ioana Treib
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bekzhan Kassenov (Gerrit)
Bekzhan Kassenov added 1 comment![]( "Open in Gerrit")
expiration, callback,I am not an expert here, but is it fine not std::move-ing the callback?
Related details
- Ioana Treib
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 1 comment![]( "Open in Gerrit")
expiration, callback,I am not an expert here, but is it fine not std::move-ing the callback?
I asked the same question :)
AI told me since we are moving it later in the in line 64 we should not move it or it will be null
@ioa...@chromium.org wdyt?, you should tell us if the AI is just trying to convince me with nonsense
Related details
- Bekzhan Kassenov
- Ioana Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bekzhan Kassenov (Gerrit)
Bekzhan Kassenov added 1 comment![]( "Open in Gerrit")
expiration, callback,Salma AlyI am not an expert here, but is it fine not std::move-ing the callback?
I asked the same question :)
AI told me since we are moving it later in the in line 64 we should not move it or it will be null
@ioa...@chromium.org wdyt?, you should tell us if the AI is just trying to convince me with nonsense
We aren't moving is there either. Both places create copies, and I don't know if callbacks can be copied.
Related details
- Ioana Treib
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib added 4 comments![]( "Open in Gerrit")
expiration, callback,Salma AlyI am not an expert here, but is it fine not std::move-ing the callback?
Bekzhan KassenovI asked the same question :)
AI told me since we are moving it later in the in line 64 we should not move it or it will be null
@ioa...@chromium.org wdyt?, you should tell us if the AI is just trying to convince me with nonsense
We aren't moving is there either. Both places create copies, and I don't know if callbacks can be copied.
A RepeatingCallback is copyable. A OnceCallback is not (for obvious reasons, it needs to ensure it's only called once).
See: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/callback.md#oncecallback_and-repeatingcallback
So, copying is fine, but the code readability here isn't.
```
using CallbackSignature =
void(OneTimeTokenSource,
base::expected<OneTimeToken, OneTimeTokenRetrievalError>);
using Callback = base::RepeatingCallback<CallbackSignature>;
```
This abstracts too much of the type away, so I suggest calling it something else or using the entire type.
One more piece of info about std::move in general: using objects after a move results in undefined behavior.
subscription.expiration_callback.Reset();Does this mean that once a notification goes through, the subscription won't expire? Is it cancelled? I might be misunderstanding something here.
// `callback` should be safe to call during this time (it may be protected via
// a WeakPtr binding). It is called every time `Notify()` is called while the
// subscription did not not expire.Please add some details about the `expiration_callback`, also maybe how it interacts with `callback`.
TEST_F(ExpiringSubscriptionTest, ExpirationCallback) {What about a test that checks that the expiration callback is not invoked if there was a notification?
Related details
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 4 comments![]( "Open in Gerrit")
Salma AlyI am not an expert here, but is it fine not std::move-ing the callback?
Bekzhan KassenovI asked the same question :)
AI told me since we are moving it later in the in line 64 we should not move it or it will be null
@ioa...@chromium.org wdyt?, you should tell us if the AI is just trying to convince me with nonsense
Ioana TreibWe aren't moving is there either. Both places create copies, and I don't know if callbacks can be copied.
A RepeatingCallback is copyable. A OnceCallback is not (for obvious reasons, it needs to ensure it's only called once).
See: https://chromium.googlesource.com/chromium/src/+/HEAD/docs/callback.md#oncecallback_and-repeatingcallbackSo, copying is fine, but the code readability here isn't.
```
using CallbackSignature =
void(OneTimeTokenSource,
base::expected<OneTimeToken, OneTimeTokenRetrievalError>);
using Callback = base::RepeatingCallback<CallbackSignature>;
```This abstracts too much of the type away, so I suggest calling it something else or using the entire type.
One more piece of info about std::move in general: using objects after a move results in undefined behavior.
Thank you for the info @ioa...@chromium.org 😊
Great point about the alias hiding the RepeatingCallback semantics! I completely agree, I will do it in a separate cl though.
subscription.expiration_callback.Reset();Does this mean that once a notification goes through, the subscription won't expire? Is it cancelled? I might be misunderstanding something here.
My understanding that the way the whole code in the `OneTimeTokenService` is built is for the subscriber get all the OTPs received during the subscription period. In addition, the subscription expires when the period of the subscription is over. None of this has changed in this cl
I thought it's only acceptable to return the "subscriptionExpiredError" when we didn't get an OTP to use. But if we return something that can be used no need to return this error, otherwise we would return it at the end of all the subscription periods (we might then consume it or we might have already returned the OTP and tool execution is done)
Now what is going on in this cl is that if there is at least one OTP returned, we don't return "subscriptionExpiredError". I think that after the sender matching is done, we should `Reset` the subscription callback when we return an OTP that matches a sender.
wdyt? We can talk about this f2f for easier discussion
// `callback` should be safe to call during this time (it may be protected via
// a WeakPtr binding). It is called every time `Notify()` is called while the
// subscription did not not expire.Please add some details about the `expiration_callback`, also maybe how it interacts with `callback`.
Done
What about a test that checks that the expiration callback is not invoked if there was a notification?
- Bekzhan Kassenov
- Ioana Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bekzhan Kassenov (Gerrit)
Bekzhan Kassenov added 2 comments![]( "Open in Gerrit")
expiration, callback, std::move(expiration_callback));Shoudln't we now std::move this one instead of making a copy? We made a copy above for the expiration case, so this should be move-able now
subscription.expiration_callback.Reset();Salma AlyDoes this mean that once a notification goes through, the subscription won't expire? Is it cancelled? I might be misunderstanding something here.
My understanding that the way the whole code in the `OneTimeTokenService` is built is for the subscriber get all the OTPs received during the subscription period. In addition, the subscription expires when the period of the subscription is over. None of this has changed in this cl
I thought it's only acceptable to return the "subscriptionExpiredError" when we didn't get an OTP to use. But if we return something that can be used no need to return this error, otherwise we would return it at the end of all the subscription periods (we might then consume it or we might have already returned the OTP and tool execution is done)
Now what is going on in this cl is that if there is at least one OTP returned, we don't return "subscriptionExpiredError". I think that after the sender matching is done, we should `Reset` the subscription callback when we return an OTP that matches a sender.
wdyt? We can talk about this f2f for easier discussion
I agree that this turns the second callback into "NoOtpValuesAvailable" callback instead of "SubscriptionEnded" callback which is at least misleading name-wise.
One approach we could take is:
- Remove this line. This will make the callback really the "subscription ended" one.
- Allow an second expiration callback in OneTimeTokenService::Subscribe.
- Let the client track how to react to the end of subscription - it could check that it didn't receive an OTPs and react accordingly.
WDYT?
Related details
- Ioana Treib
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 1 comment![]( "Open in Gerrit")
subscription.expiration_callback.Reset();Salma AlyDoes this mean that once a notification goes through, the subscription won't expire? Is it cancelled? I might be misunderstanding something here.
Bekzhan KassenovMy understanding that the way the whole code in the `OneTimeTokenService` is built is for the subscriber get all the OTPs received during the subscription period. In addition, the subscription expires when the period of the subscription is over. None of this has changed in this cl
I thought it's only acceptable to return the "subscriptionExpiredError" when we didn't get an OTP to use. But if we return something that can be used no need to return this error, otherwise we would return it at the end of all the subscription periods (we might then consume it or we might have already returned the OTP and tool execution is done)
Now what is going on in this cl is that if there is at least one OTP returned, we don't return "subscriptionExpiredError". I think that after the sender matching is done, we should `Reset` the subscription callback when we return an OTP that matches a sender.
wdyt? We can talk about this f2f for easier discussion
I agree that this turns the second callback into "NoOtpValuesAvailable" callback instead of "SubscriptionEnded" callback which is at least misleading name-wise.
One approach we could take is:
- Remove this line. This will make the callback really the "subscription ended" one.
- Allow an second expiration callback in OneTimeTokenService::Subscribe.
- Let the client track how to react to the end of subscription - it could check that it didn't receive an OTPs and react accordingly.
WDYT?
Yup this will work, but wouldn't a timer in the actor service be much more simpler?
I think what I don't like about this approach is that fact that we will need to track whether we got a matching OTP or not to decide whether or not we should consume the "SubscriptionEnded". The fact that the service has a long live time, makes this tracking complicated and error prone (we need to clean up the tracking when a new retrieval call is received and we will need to clean this subscription expired callback similar to what we do now with the other callback)
WDYT?
Related details
- Bekzhan Kassenov
- Ioana Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib added 1 comment![]( "Open in Gerrit")
subscription.expiration_callback.Reset();Salma AlyDoes this mean that once a notification goes through, the subscription won't expire? Is it cancelled? I might be misunderstanding something here.
Bekzhan KassenovMy understanding that the way the whole code in the `OneTimeTokenService` is built is for the subscriber get all the OTPs received during the subscription period. In addition, the subscription expires when the period of the subscription is over. None of this has changed in this cl
I thought it's only acceptable to return the "subscriptionExpiredError" when we didn't get an OTP to use. But if we return something that can be used no need to return this error, otherwise we would return it at the end of all the subscription periods (we might then consume it or we might have already returned the OTP and tool execution is done)
Now what is going on in this cl is that if there is at least one OTP returned, we don't return "subscriptionExpiredError". I think that after the sender matching is done, we should `Reset` the subscription callback when we return an OTP that matches a sender.
wdyt? We can talk about this f2f for easier discussion
Salma AlyI agree that this turns the second callback into "NoOtpValuesAvailable" callback instead of "SubscriptionEnded" callback which is at least misleading name-wise.
One approach we could take is:
- Remove this line. This will make the callback really the "subscription ended" one.
- Allow an second expiration callback in OneTimeTokenService::Subscribe.
- Let the client track how to react to the end of subscription - it could check that it didn't receive an OTPs and react accordingly.
WDYT?
Yup this will work, but wouldn't a timer in the actor service be much more simpler?
I think what I don't like about this approach is that fact that we will need to track whether we got a matching OTP or not to decide whether or not we should consume the "SubscriptionEnded". The fact that the service has a long live time, makes this tracking complicated and error prone (we need to clean up the tracking when a new retrieval call is received and we will need to clean this subscription expired callback similar to what we do now with the other callback)
WDYT?
Discussed offline with Salma. I think both approaches work. If the subscription expiry callback is part of the subsciption object, then we anyway have logic to clear it when we find a match or get an error. If we do implement this with a second callback, it should be bundled with the original token received callback so that we clear them in one go.
There is one tracking caveat. The subscription ended callback can race with the OTP domain match check, so that will add some overhead, but that will be the case with a timer in the `ActorOneTimeTokenFillingService` as well.
Related details
- Bekzhan Kassenov
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 4 comments![]( "Open in Gerrit")
[[nodiscard]] ExpiringSubscription Subscribe(
OneTimeTokenSource source,Please fix this WARNING reported by ClangTidy: check: google-default-arguments
default arguments on virtual or override method...
check: google-default-arguments
default arguments on virtual or override methods are prohibited (https://clang.llvm.org/extra/clang-tidy/checks/google/default-arguments.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-default-arguments` footer to the CL description to skip the check)
@ioa...@chromium.org, I looked into this and apparently we should not use default values with override methods?!
I can skip the check would that make sense? After some investigations I found out that we can fix it like https://screenshot.googleplex.com/6UcNxFppBRo8EKt, but I'm not sure if it makes sense, wdyt?
expiration, callback, std::move(expiration_callback));Shoudln't we now std::move this one instead of making a copy? We made a copy above for the expiration case, so this should be move-able now
obsolete now
subscription.expiration_callback.Reset();Salma AlyDoes this mean that once a notification goes through, the subscription won't expire? Is it cancelled? I might be misunderstanding something here.
Bekzhan KassenovMy understanding that the way the whole code in the `OneTimeTokenService` is built is for the subscriber get all the OTPs received during the subscription period. In addition, the subscription expires when the period of the subscription is over. None of this has changed in this cl
I thought it's only acceptable to return the "subscriptionExpiredError" when we didn't get an OTP to use. But if we return something that can be used no need to return this error, otherwise we would return it at the end of all the subscription periods (we might then consume it or we might have already returned the OTP and tool execution is done)
Now what is going on in this cl is that if there is at least one OTP returned, we don't return "subscriptionExpiredError". I think that after the sender matching is done, we should `Reset` the subscription callback when we return an OTP that matches a sender.
wdyt? We can talk about this f2f for easier discussion
Salma AlyI agree that this turns the second callback into "NoOtpValuesAvailable" callback instead of "SubscriptionEnded" callback which is at least misleading name-wise.
One approach we could take is:
- Remove this line. This will make the callback really the "subscription ended" one.
- Allow an second expiration callback in OneTimeTokenService::Subscribe.
- Let the client track how to react to the end of subscription - it could check that it didn't receive an OTPs and react accordingly.
WDYT?
Ioana TreibYup this will work, but wouldn't a timer in the actor service be much more simpler?
I think what I don't like about this approach is that fact that we will need to track whether we got a matching OTP or not to decide whether or not we should consume the "SubscriptionEnded". The fact that the service has a long live time, makes this tracking complicated and error prone (we need to clean up the tracking when a new retrieval call is received and we will need to clean this subscription expired callback similar to what we do now with the other callback)
WDYT?
Discussed offline with Salma. I think both approaches work. If the subscription expiry callback is part of the subsciption object, then we anyway have logic to clear it when we find a match or get an error. If we do implement this with a second callback, it should be bundled with the original token received callback so that we clear them in one go.
There is one tracking caveat. The subscription ended callback can race with the OTP domain match check, so that will add some overhead, but that will be the case with a timer in the `ActorOneTimeTokenFillingService` as well.
Done
namespace internal {
ExpiringSubscriptionDataBase::ExpiringSubscriptionDataBase() = default;
ExpiringSubscriptionDataBase::~ExpiringSubscriptionDataBase() = default;
} // namespace internal
@ioa...@chromium.org please confirm that moving the ExpiringSubscriptionDataBase constructor and destructor here is okay
AI seems convinced that having it here is better because now the object contain more complex types that might be expensive to have the deconstruction logic every time the header is included.
Related details
- Bekzhan Kassenov
- Ioana Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib added 2 comments![]( "Open in Gerrit")
[[nodiscard]] ExpiringSubscription Subscribe(
OneTimeTokenSource source,Please fix this WARNING reported by ClangTidy: check: google-default-arguments
default arguments on virtual or override method...
check: google-default-arguments
default arguments on virtual or override methods are prohibited (https://clang.llvm.org/extra/clang-tidy/checks/google/default-arguments.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-default-arguments` footer to the CL description to skip the check)
@ioa...@chromium.org, I looked into this and apparently we should not use default values with override methods?!I can skip the check would that make sense? After some investigations I found out that we can fix it like https://screenshot.googleplex.com/6UcNxFppBRo8EKt, but I'm not sure if it makes sense, wdyt?
Yeah, that is correct and I think in general default params are a bit iffy sometimes. I don't think we need a default value here. The callers can provide base::DoNothing if they don't care about expiration.
namespace internal {
ExpiringSubscriptionDataBase::ExpiringSubscriptionDataBase() = default;
ExpiringSubscriptionDataBase::~ExpiringSubscriptionDataBase() = default;
} // namespace internal
@ioa...@chromium.org please confirm that moving the ExpiringSubscriptionDataBase constructor and destructor here is okay
AI seems convinced that having it here is better because now the object contain more complex types that might be expensive to have the deconstruction logic every time the header is included.
I think this is actually required for non-trivial constructors/destructors. You would see a compilation error if you try to inline it in the header.
Related details
- Bekzhan Kassenov
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 4 comments![]( "Open in Gerrit")
base::DoNothing());Please fix this WARNING reported by autoreview issue finding: To fulfill the goal mentioned in the commit message (preventing Gmail OTP fillers from hanging), this caller should likely provide a callback here that invokes `OnOneTimeTokenReceived` with `kSubscriptionExpired`.
Passing `base::DoNothing()` means the behavior remains identical to before this CL for this specific filler.
------------
This will be added in the child change, split for easier review
base::OnceClosure expiration_callback) = 0;Please fix this WARNING reported by autoreview issue finding: Since most callers currently don't need to handle expiration (as evidenced by the many `base::DoNothing()` additions in this CL), consider making this parameter optional with a default value:
`base::OnceClosure expiration_callback = base::NullCallback()`
This would keep the interface cleaner for callers that only care about token arrival.
------------
We originally had the default value, but we removed it because "ClangTidy: check: google-default-arguments"
[[nodiscard]] ExpiringSubscription Subscribe(
OneTimeTokenSource source,Ioana TreibPlease fix this WARNING reported by ClangTidy: check: google-default-arguments
default arguments on virtual or override method...
check: google-default-arguments
default arguments on virtual or override methods are prohibited (https://clang.llvm.org/extra/clang-tidy/checks/google/default-arguments.html)
(Note: You can add `Skip-Clang-Tidy-Checks: google-default-arguments` footer to the CL description to skip the check)
@ioa...@chromium.org, I looked into this and apparently we should not use default values with override methods?!I can skip the check would that make sense? After some investigations I found out that we can fix it like https://screenshot.googleplex.com/6UcNxFppBRo8EKt, but I'm not sure if it makes sense, wdyt?
Yeah, that is correct and I think in general default params are a bit iffy sometimes. I don't think we need a default value here. The callers can provide base::DoNothing if they don't care about expiration.
Done. I just didn't want to change all the tests and other code paths that call this but don't use the callback
namespace internal {
ExpiringSubscriptionDataBase::ExpiringSubscriptionDataBase() = default;
ExpiringSubscriptionDataBase::~ExpiringSubscriptionDataBase() = default;
} // namespace internal
Ioana Treib@ioa...@chromium.org please confirm that moving the ExpiringSubscriptionDataBase constructor and destructor here is okay
AI seems convinced that having it here is better because now the object contain more complex types that might be expensive to have the deconstruction logic every time the header is included.
I think this is actually required for non-trivial constructors/destructors. You would see a compilation error if you try to inline it in the header.
- Bekzhan Kassenov
- David Bokan
- Ioana Treib
- Matthias Körber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Körber (Gerrit)
Matthias Körber added 1 comment![]( "Open in Gerrit")
base::DoNothing());Pointed out by AI:
The commit message mentions that `expiration_callback` was added specifically to fix `AttemptOtpFillingTool` hanging indefinitely on timeout. However, passing `base::DoNothing()` here means the subscription will still be silently dropped without notifying the caller, and the tool will still hang.
To actually resolve the issue, we should pass a callback that invokes `retrieve_otp_callback_` with an error (e.g. by binding `OnOneTimeTokenReceived` with an unexpected result) so the pending tool request gets an explicit action result.
Related details
- Bekzhan Kassenov
- David Bokan
- Ioana Treib
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Körber (Gerrit)
Matthias Körber added 1 comment![]( "Open in Gerrit")
base::DoNothing());Pointed out by AI:
The commit message mentions that `expiration_callback` was added specifically to fix `AttemptOtpFillingTool` hanging indefinitely on timeout. However, passing `base::DoNothing()` here means the subscription will still be silently dropped without notifying the caller, and the tool will still hang.
To actually resolve the issue, we should pass a callback that invokes `retrieve_otp_callback_` with an error (e.g. by binding `OnOneTimeTokenReceived` with an unexpected result) so the pending tool request gets an explicit action result.
old patchset. closing
Related details
- Bekzhan Kassenov
- David Bokan
- Ioana Treib
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Bekzhan Kassenov (Gerrit)
Bekzhan Kassenov voted Code-Review+1![]( "Open in Gerrit")
- David Bokan
- Ioana Treib
- Matthias Körber
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib voted and added 5 comments![]( "Open in Gerrit")
Votes added by Ioana Treib
| Code-Review | +1 |
5 comments
testing::_, testing::_))`using testing::_ ;` at the top of the file and then you can write only `_` here.
base::DoNothing());Please add a param name comment.
base::DoNothing());Please add param comments here and everywhere else.
base::OnceClosure expiration_callback = base::NullCallback()) {Considering that you're now providing this callback from everywhere, should we get rid of the default value?
Related details
- David Bokan
- Matthias Körber
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Bokan (Gerrit)
David Bokan voted Code-Review+1![]( "Open in Gerrit")
- Bekzhan Kassenov
- Ioana Treib
- Matthias Körber
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Körber (Gerrit)
Matthias Körber voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Bekzhan Kassenov
- Ioana Treib
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Matthias Körber (Gerrit)
Matthias Körber voted and added 1 comment![]( "Open in Gerrit")
Votes added by Matthias Körber
| Code-Review | +0 |
1 comment
UpdateNextExpirationTimer();This was identified by AI and needs adressing:
Calling `UpdateNextExpirationTimer()` after running external callbacks poses a use-after-free risk. If any of the `expiration_callbacks` destroys this manager instance, accessing `this` to update the timer will cause a crash.\n\nSince the expired subscriptions have already been removed from `subscriptions_` on line 70, you should move this `UpdateNextExpirationTimer()` call to be strictly before the loop that runs the callbacks.
Related details
- Bekzhan Kassenov
- Ioana Treib
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 5 comments![]( "Open in Gerrit")
`using testing::_ ;` at the top of the file and then you can write only `_` here.
Done
Please add a param name comment.
Done
Please add a param name.
Done
Please add param comments here and everywhere else.
Done
base::OnceClosure expiration_callback = base::NullCallback()) {Considering that you're now providing this callback from everywhere, should we get rid of the default value?
yes, we can, but I didn't want to because we will need to change the test file and add all of those "DoNothing" callbacks to the subscriptions.
Related details
- Bekzhan Kassenov
- Ioana Treib
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ioana Treib
| Code-Review | +1 |
1 comment
Related details
- Bekzhan Kassenov
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 1 comment![]( "Open in Gerrit")
This was identified by AI and needs adressing:
Calling `UpdateNextExpirationTimer()` after running external callbacks poses a use-after-free risk. If any of the `expiration_callbacks` destroys this manager instance, accessing `this` to update the timer will cause a crash.\n\nSince the expired subscriptions have already been removed from `subscriptions_` on line 70, you should move this `UpdateNextExpirationTimer()` call to be strictly before the loop that runs the callbacks.
- Bekzhan Kassenov
- David Bokan
- Ioana Treib
- Matthias Körber
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ioana Treib (Gerrit)
Ioana Treib voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ioana Treib
| Code-Review | +1 |
- Bekzhan Kassenov
- David Bokan
- Matthias Körber
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly added 1 comment![]( "Open in Gerrit")
Norge I just need your owners approval on otp_manager_impl.cc
Related details
- Bekzhan Kassenov
- David Bokan
- Norge Vizcay
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Norge Vizcay (Gerrit)
Norge Vizcay voted and added 1 comment![]( "Open in Gerrit")
Votes added by Norge Vizcay
| Code-Review | +1 |
1 comment
- Bekzhan Kassenov
- David Bokan
- Salma Aly
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Salma Aly (Gerrit)
Salma Aly voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Bekzhan Kassenov
- David Bokan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[Otp4Bd] Introduce expiration closure to Subscription Manager
The OneTimeTokenServiceImpl currently handles subscriptions to OTPs
(both SMS and Gmail) via the generic ExpiringSubscriptionManager. When a
subscription reaches its timeout, it is silently dropped. This works
fine for SMS, but leaves Gmail OTP fillers (like the
AttemptOtpFillingTool) hanging indefinitely because no explicit action
result is reported back.
To fix this, this CL introduces an optional `expiration_callback` to
`ExpiringSubscriptionManager::Subscribe()`. If provided, the manager
will run it when the subscription is purged during
`ProcessExpirations()`.
In the next commit, we will add an expiration callback from the Gmail
caller.
Bug: b:530535640 TAG=agy
- M chrome/browser/actor/tools/attempt_otp_filling_tool_browsertest.cc
- M chrome/browser/autofill/actor/one_time_tokens/actor_one_time_token_filling_service_impl.cc
- M chrome/browser/autofill/actor/one_time_tokens/actor_one_time_token_filling_service_impl_unittest.cc
- M chrome/browser/glic/actor/glic_actor_attempt_otp_filling_browsertest.cc
- M components/autofill/core/browser/integrators/one_time_tokens/otp_manager_impl.cc
- M components/one_time_tokens/core/browser/gmail_otp_backend.cc
- M components/one_time_tokens/core/browser/mock_one_time_token_service.h
- M components/one_time_tokens/core/browser/one_time_token_service.h
- M components/one_time_tokens/core/browser/one_time_token_service_impl.cc
- M components/one_time_tokens/core/browser/one_time_token_service_impl.h
- M components/one_time_tokens/core/browser/one_time_token_service_impl_unittest.cc
- M components/one_time_tokens/core/browser/util/expiring_subscription_manager.cc
- M components/one_time_tokens/core/browser/util/expiring_subscription_manager.h
- M components/one_time_tokens/core/browser/util/expiring_subscription_manager_unittest.cc
Code-Review: +1 by Norge Vizcay, +1 by Ioana Treib
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |