Remove incorrect CHECK in SessionServiceImpl [chromium/src : main]
Daniel Rubery (Gerrit)
New activity on the change![]( "Open in Gerrit")
Related details
- thefrog
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
thefrog (Gerrit)
thefrog voted and added 3 comments![]( "Open in Gerrit")
Votes added by thefrog
| Code-Review | +1 |
3 comments
case kSessionDeletedDuringRefresh:I don't see an issue with this being here, but for my understanding -- we don't actually expect `kSessionDeletedDuringRefresh` in a call to `GetDeletionReason()` right? Or for `IsServerError()` either.
if (!existing_session) {Not required in this case, but please consider adding a test.
Related details
- Daniel Rubery
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. |
Daniel Rubery (Gerrit)
Daniel Rubery voted and added 2 comments![]( "Open in Gerrit")
Votes added by Daniel Rubery
| Auto-Submit | +1 |
2 comments
I don't see an issue with this being here, but for my understanding -- we don't actually expect `kSessionDeletedDuringRefresh` in a call to `GetDeletionReason()` right? Or for `IsServerError()` either.
No, we don't. And I could put it in a the `NOTREACHED` block as a result, but I thought it was conceptually a fatal error in the refresh process.
Not required in this case, but please consider adding a test.
Ah yeah I thought the tests would be hard but they're really not. Done.
Related details
- thefrog
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
thefrog (Gerrit)
thefrog voted![]( "Open in Gerrit")
- Daniel Rubery
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From googleclient/chrome/chromium_gwsq/ipc/config.gwsq:
IPC: ff...@chromium.org
📎 It looks like you’re making a possibly security-sensitive change! 📎 IPC security review isn’t a rubberstamp, so your friendly security reviewer will need a fair amount of context to review your CL effectively. Please review your CL description and code comments to make sure they provide context for someone unfamiliar with your project/area. Pay special attention to where data comes from and which processes it flows between (and their privilege levels). Feel free to point your security reviewer at design docs, bugs, or other links if you can’t reasonably make a self-contained CL description. (Also see https://cbea.ms/git-commit/).
IPC reviewer(s): ff...@chromium.org
Reviewer source(s):
ff...@chromium.org is from context(googleclient/chrome/chromium_gwsq/ipc/config.gwsq)
Related details
- Fred Shih
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Fred Shih (Gerrit)
Fred Shih voted and added 1 comment![]( "Open in Gerrit")
- Daniel Rubery
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Daniel Rubery (Gerrit)
Daniel Rubery voted Commit-Queue+2![]( "Open in Gerrit")
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
Remove incorrect CHECK in SessionServiceImpl
We were too confident here. It is possible for a refresh to start, then
for the user (or site) to clear data. Then when the refresh finishes,
the existing session is gone. This CL bails out safely and logs that to metrics.
- M net/device_bound_sessions/session.cc
- M net/device_bound_sessions/session_error.cc
- M net/device_bound_sessions/session_error.h
- M net/device_bound_sessions/session_service_impl.cc
- M net/device_bound_sessions/session_service_impl_unittest.cc
- M services/network/public/cpp/device_bound_sessions_mojom_traits.h
- M services/network/public/mojom/device_bound_sessions.mojom
- M tools/metrics/histograms/enums.xml
Code-Review: +1 by Fred Shih, +1 by thefrog
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |