[SyncPreview] Add callback triggered when all accounts are fetched [chromium/src : main]
Ryan Sultanem (Gerrit)
Ryan Sultanem added 1 comment![]( "Open in Gerrit")
Related details
- Pâris Meuleman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Pâris Meuleman (Gerrit)
Pâris Meuleman voted and added 3 comments![]( "Open in Gerrit")
Votes added by Pâris Meuleman
| Code-Review | +1 |
3 comments
LGTM with a question on the risk of races between network delay helper and the barrier.
all_accounts_fetched_barrier_.Run();I wonder if this might crash if either accounts are removed while the fetch is in network_Delay_helper_, or if the periodic fetch resets the barrier in the same situation?
in that case just counting the successful fetches might not be resilient enough?
AllDataAvailableWaiter waiter(service_.get());optional nit: can the lifetime of the waiter be a problem here if e.g. it gets deleted before the callback is invoked, maybe by an early returning assertion ? that'd lead to flaky failures
Related details
- Ryan Sultanem
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. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 3 comments![]( "Open in Gerrit")
all_accounts_fetched_barrier_.Run();I wonder if this might crash if either accounts are removed while the fetch is in network_Delay_helper_, or if the periodic fetch resets the barrier in the same situation?
in that case just counting the successful fetches might not be resilient enough?
That is actually a good point - since the active fetcher is not set yet.
We can do one of:
- Create the fetcher but not activate it until the network is established, and therefore validating that the fetcher is still alive (not destroyed by the account being removed).
- Before creating the fetcher and after the network is established, ensure that the gaia_id is still linked to a signed in account.
I believe I will follow the second option, as it is slightly more straightforward. Let me knnow what you think!
AllDataAvailableWaiter waiter(service_.get());optional nit: can the lifetime of the waiter be a problem here if e.g. it gets deleted before the callback is invoked, maybe by an early returning assertion ? that'd lead to flaky failures
Do you mean in this test or in general? I believe that in this test or any test that will call `Wait()` that this should not be possible.
I can add an assertion at the destructor that checks whether `is_all_data_available_` is `true` and that `run_loop_` is not currently running. Is this what you meant?
Related details
- Pâris Meuleman
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. |
Pâris Meuleman (Gerrit)
Pâris Meuleman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Pâris Meuleman
| Code-Review | +1 |
2 comments
all_accounts_fetched_barrier_.Run();Ryan SultanemI wonder if this might crash if either accounts are removed while the fetch is in network_Delay_helper_, or if the periodic fetch resets the barrier in the same situation?
in that case just counting the successful fetches might not be resilient enough?
That is actually a good point - since the active fetcher is not set yet.
We can do one of:
- Create the fetcher but not activate it until the network is established, and therefore validating that the fetcher is still alive (not destroyed by the account being removed).
- Before creating the fetcher and after the network is established, ensure that the gaia_id is still linked to a signed in account.
I believe I will follow the second option, as it is slightly more straightforward. Let me knnow what you think!
Both seem viable yes, going with the second SGTM.
AllDataAvailableWaiter waiter(service_.get());Ryan Sultanemoptional nit: can the lifetime of the waiter be a problem here if e.g. it gets deleted before the callback is invoked, maybe by an early returning assertion ? that'd lead to flaky failures
Do you mean in this test or in general? I believe that in this test or any test that will call `Wait()` that this should not be possible.
I can add an assertion at the destructor that checks whether `is_all_data_available_` is `true` and that `run_loop_` is not currently running. Is this what you meant?
I think: if an assertion fails before wait(), destructing the waiter, then AccountPreviewDataService might call OnAllDataAvailable on a dangling pointer, i.e. UAF.
we could in the dtor cleanup the services' callback or use a weak pointer in SetAllDataAvailableCallbackForTesting
Related details
- Ryan Sultanem
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. |
Ryan Sultanem (Gerrit)
Ryan Sultanem added 3 comments![]( "Open in Gerrit")
all_accounts_fetched_barrier_.Run();Ryan SultanemI wonder if this might crash if either accounts are removed while the fetch is in network_Delay_helper_, or if the periodic fetch resets the barrier in the same situation?
in that case just counting the successful fetches might not be resilient enough?
Pâris MeulemanThat is actually a good point - since the active fetcher is not set yet.
We can do one of:
- Create the fetcher but not activate it until the network is established, and therefore validating that the fetcher is still alive (not destroyed by the account being removed).
- Before creating the fetcher and after the network is established, ensure that the gaia_id is still linked to a signed in account.
I believe I will follow the second option, as it is slightly more straightforward. Let me knnow what you think!
Both seem viable yes, going with the second SGTM.
Acknowledged
AllDataAvailableWaiter waiter(service_.get());Ryan Sultanemoptional nit: can the lifetime of the waiter be a problem here if e.g. it gets deleted before the callback is invoked, maybe by an early returning assertion ? that'd lead to flaky failures
Pâris MeulemanDo you mean in this test or in general? I believe that in this test or any test that will call `Wait()` that this should not be possible.
I can add an assertion at the destructor that checks whether `is_all_data_available_` is `true` and that `run_loop_` is not currently running. Is this what you meant?
I think: if an assertion fails before wait(), destructing the waiter, then AccountPreviewDataService might call OnAllDataAvailable on a dangling pointer, i.e. UAF.
we could in the dtor cleanup the services' callback or use a weak pointer in SetAllDataAvailableCallbackForTesting
Oh I see now, thanks! This would happen in case of failures. Yes then clearing the callback in the destructor would fix it!
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |