Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2966763003:
[Prototype] Delete Google service worker caches on Android signout (Closed)
Description[Prototype] Delete Google service worker caches on Android signout.
Android Signin Manager already has the functionality to delete all data
when a managed user signs out.
This CL reuses that functionality to delete service worker caches when
any user signs out.
BUG=739982
Review-Url: https://codereview.chromium.org/2966763003
Cr-Commit-Position: refs/heads/master@{#486619}
Committed: https://chromium.googlesource.com/chromium/src/+/0e575f680905fdd810dd4e5a8af2f120d03450da
Patch Set 1 #Patch Set 2 : Rebase #Patch Set 3 : Moved some logic to google_util, added a unittest. #Patch Set 4 : Fix the test. #Patch Set 5 : Fix the test. #
Total comments: 8
Patch Set 6 : Addressed comments. #Patch Set 7 : Typo #Patch Set 8 : Only delete the cache. #
Total comments: 4
Patch Set 9 : Addressed nits. #
Messages
Total messages: 49 (32 generated)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msramek@chromium.org changed reviewers: + maxbogue@chromium.org
Hi Max, Could you take a pass at this CL? I still need to write tests etc., I just wonder if you'd be fine with putting this code into SigninManager. The context is in this doc: https://docs.google.com/document/d/1bQGVPLNRbrqQy2JY5gfo97NiruoKMt9GenPV9d95DEA/ Thanks! Martin
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
maxbogue@chromium.org changed reviewers: + gogerald@chromium.org
Looks fine to me. +gogerald to also take a look though, since I haven't touched signin for a long time.
lgtm, cc'ed bsazonov@ for who is actively working on these codes,
Thanks! I'm not landing yet, because I need to clarify first whether we want to do this directly or behind a flag, and of course, write/fix the tests.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Prototype] Delete service workers on Android signout. Android Signin Manager already has the functionality to delete all data when a managed user signs out. This CL reuses that functionality to delete service workers (and their caches) when any user signs out. BUG=TBD ========== to ========== [Prototype] Delete service workers on Android signout. Android Signin Manager already has the functionality to delete all data when a managed user signs out. This CL reuses that functionality to delete service workers (and their caches) when any user signs out. BUG=739982 ==========
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
msramek@chromium.org changed reviewers: + pkasting@chromium.org
I have made some improvements. - Added a unittest. Ganggui, can you take another pass? - Moved some logic to google_util. +Peter, please have a look! - We now have a launch bug (linked from the CL description). Thanks! Martin
This makes me very uncomfortable. Chrome for many years had a teamwide philosophy that we would bend over backwards not to put Google-specific behaviors into the product. Anything a Google site wanted needed to be accomplished with a standard behavior that would be equally available and effective for any other site. If that meant that Google products did not ship features until Chrome standardized a mechanism, that was OK. The line has gotten more blurry in recent years, which is IMO a bad thing and something I try hard to push back against when I see it. We need to be vigilant on not making Google any different than any other site(s) w.r.t Chrome. This CL both adds a very Google-specific magic behavior and adds the framework that makes it easy for future people to add yet more such behaviors. I really want to avoid that. Reading the original docs here it seems as if this could be done in other ways that are less magic, but may take longer to implement or are less reliable. I urge you to go one of those routes instead. If you override me, please devise a mechanism to ensure that this solution is timebombed and will be removed from the codebase in a few months or something, in favor of one of the other solutions. Persisting this forever seems very bad.
Thanks for the feedback, Peter. I am completely on your side when it comes to not special-casing Google. The reason why I considered this acceptable is that we already have the connection between signing in to Chrome and signing in to google.com. We already delete some google.com cookies when you sign out of Chrome. And with this CL, we just expand the scope of deletion to service workers. The original plan was to use the new Clear-Site-Data header, but that turned out to be unsuitable. However, let me put this on hold for a moment and try out one idea that I had...
On 2017/07/10 19:03:16, msramek (slow) wrote: > Thanks for the feedback, Peter. > > I am completely on your side when it comes to not special-casing Google. The > reason why I considered this acceptable is that we already have the connection > between signing in to Chrome and signing in to http://google.com. We already delete > some http://google.com cookies when you sign out of Chrome. And with this CL, we just > expand the scope of deletion to service workers. Yeah, and to be fair, I find that connection really confusing and troublesome. However, I also want to note that a lot of smart people have put a lot more thought than me into this space, and I'm not the final word on this. If you're already on-board with my concerns, then I trust you to make the right decisions. I'll LGTM the actual code change here so you have the freedom to move forward once said decisions are made. https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.cc:293: std::vector<std::string> tlds({GOOGLE_TLD_LIST}); Nit: Are () necessary here? https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.cc:295: std::string domain = base::StringPrintf("google.%s", tld.c_str()); Nit: Easier to read, doesn't require an #include, probably more performant: std::string domain = "google." + tld; https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.cc:297: // Our list of domains may be slightly out of date. I kinda don't think you should do this check. It's not clear to me exactly what sort of out-of-dateness it's supposed to solve, especially since the items on GOOGLE_TLD_LIST should be using ccTLDs. If a ccTLD went away, I think this check wouldn't fail? Anyway, if we wanted to keep this, I'd rather it be more clearly motivated. Also, without this, this whole loop basically just becomes a call to std::transform with a back_inserter, or even maybe you just set kGoogleRegisterableDomains = {GOOGLE_TLD_LIST} and then transform in-place. https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... File components/google/core/browser/google_util.h (right): https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.h:116: // Returns the list of all Google's registerable domains, i.e. domains named Nit: I think the correct spelling is "registrable" (multiple places)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks, Peter! I addressed your comments. I discussed this with a few involved people, and it seems that this is currently the only viable solution, as the problem I'm solving itself is a result of special-casing Google... The closest web-based solution on the horizon is the proposed cookies API for service workers, which could allow the service worker to discover that the user has signed out of Chrome. I have added a TODO to my name to GetGoogleRegistrableDomains(). https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... File components/google/core/browser/google_util.cc (right): https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.cc:293: std::vector<std::string> tlds({GOOGLE_TLD_LIST}); On 2017/07/10 19:26:05, Peter Kasting wrote: > Nit: Are () necessary here? Done. No, and I actually didn't know this was possible :) https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.cc:295: std::string domain = base::StringPrintf("google.%s", tld.c_str()); On 2017/07/10 19:26:05, Peter Kasting wrote: > Nit: Easier to read, doesn't require an #include, probably more performant: > > std::string domain = "google." + tld; Done. https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.cc:297: // Our list of domains may be slightly out of date. On 2017/07/10 19:26:05, Peter Kasting wrote: > I kinda don't think you should do this check. It's not clear to me exactly what > sort of out-of-dateness it's supposed to solve, especially since the items on > GOOGLE_TLD_LIST should be using ccTLDs. If a ccTLD went away, I think this > check wouldn't fail? Anyway, if we wanted to keep this, I'd rather it be more > clearly motivated. > > Also, without this, this whole loop basically just becomes a call to > std::transform with a back_inserter, or even maybe you just set > kGoogleRegisterableDomains = {GOOGLE_TLD_LIST} and then transform in-place. Sorry, the comment was unhelpful, because I wrote it before I fully investigated the problem. The browsing_data/ machinery for deleting registrable domains has a DCHECK() that all inputs are really registrable domains. ...and this DCHECK() fired. Turns out that the TLD list, generated from our DNS records, contained something that was not actually a ccTLD. See here: https://bugs.chromium.org/p/chromium/issues/detail?id=674712#c14 and here: b/63446999 This particular case can be resolved, but I wanted to avoid hardcoding an assumption that the two lists are in sync for future's sake. https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... File components/google/core/browser/google_util.h (right): https://codereview.chromium.org/2966763003/diff/80001/components/google/core/... components/google/core/browser/google_util.h:116: // Returns the list of all Google's registerable domains, i.e. domains named On 2017/07/10 19:26:05, Peter Kasting wrote: > Nit: I think the correct spelling is "registrable" (multiple places) Done. (I think you're right, I named it to be consistent with the method in browsing_data, which I should rename).
Ganggui, friendly ping :) Are you happy with the unittest?
Actually, one final correction - we'll only delete the SW cache, not the worker itself. Let me do that in another patchset... Ganggui, maybe you want to wait with the review until that :)
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [Prototype] Delete service workers on Android signout. Android Signin Manager already has the functionality to delete all data when a managed user signs out. This CL reuses that functionality to delete service workers (and their caches) when any user signs out. BUG=739982 ========== to ========== [Prototype] Delete Google service worker caches on Android signout. Android Signin Manager already has the functionality to delete all data when a managed user signs out. This CL reuses that functionality to delete service worker caches when any user signs out. BUG=739982 ==========
Uploaded a new patchset, update the test and the CL description. Sorry for the confusion. Only the service worker caches are deleted now.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
still lgtm https://codereview.chromium.org/2966763003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/2966763003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:543: wipeGoogleServiceWorkers(wipeDataHooks); nit:WipeGoogleServiceWorkerCaches? https://codereview.chromium.org/2966763003/diff/140001/chrome/browser/android... File chrome/browser/android/signin/signin_manager_android.cc (right): https://codereview.chromium.org/2966763003/diff/140001/chrome/browser/android... chrome/browser/android/signin/signin_manager_android.cc:238: WipeData(profile_, false /* only service worker caches */, nit: * only Google service worker caches*?
The CQ bit was checked by msramek@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2966763003/diff/140001/chrome/android/java/sr... File chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java (right): https://codereview.chromium.org/2966763003/diff/140001/chrome/android/java/sr... chrome/android/java/src/org/chromium/chrome/browser/signin/SigninManager.java:543: wipeGoogleServiceWorkers(wipeDataHooks); On 2017/07/13 17:08:22, gogerald1 wrote: > nit:WipeGoogleServiceWorkerCaches? Done. Missed this :) https://codereview.chromium.org/2966763003/diff/140001/chrome/browser/android... File chrome/browser/android/signin/signin_manager_android.cc (right): https://codereview.chromium.org/2966763003/diff/140001/chrome/browser/android... chrome/browser/android/signin/signin_manager_android.cc:238: WipeData(profile_, false /* only service worker caches */, On 2017/07/13 17:08:22, gogerald1 wrote: > nit: * only Google service worker caches*? Done, here and in the unittest.
Alright, proceeding to land this so that I can play with it before branchpoint. Peter, if you have additional suggestions w.r.t. the usage of net::domain_controlled_registry, happy to address them in a follow-up CL.
The CQ bit was unchecked by msramek@chromium.org
The CQ bit was checked by msramek@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from pkasting@chromium.org, gogerald@chromium.org Link to the patchset: https://codereview.chromium.org/2966763003/#ps160001 (title: "Addressed nits.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1499974814705300,
"parent_rev": "4f47f5adab253e87960648f64e15bf5246a4958d", "commit_rev":
"0e575f680905fdd810dd4e5a8af2f120d03450da"}
Message was sent while issue was closed.
Description was changed from ========== [Prototype] Delete Google service worker caches on Android signout. Android Signin Manager already has the functionality to delete all data when a managed user signs out. This CL reuses that functionality to delete service worker caches when any user signs out. BUG=739982 ========== to ========== [Prototype] Delete Google service worker caches on Android signout. Android Signin Manager already has the functionality to delete all data when a managed user signs out. This CL reuses that functionality to delete service worker caches when any user signs out. BUG=739982 Review-Url: https://codereview.chromium.org/2966763003 Cr-Commit-Position: refs/heads/master@{#486619} Committed: https://chromium.googlesource.com/chromium/src/+/0e575f680905fdd810dd4e5a8af2... ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://chromium.googlesource.com/chromium/src/+/0e575f680905fdd810dd4e5a8af2... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
