|
|
Created:
6 years, 8 months ago by tzik Modified:
6 years, 7 months ago CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org, kinuko+watch Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
Description[Extension] Protect all installed extension from extension storage GC
ExtensionGarbageCollector collects isolated storages of disabled apps as garbages,
this may cause unexpected data loss for temporarily disabled extensions.
This CL protects non-enabled installed app's isolated storage from GC.
BUG=328637
R=ajwong@chromium.org, mek@chromium.org, rdevlin.cronin@chromium.org
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266473
Patch Set 1 #Patch Set 2 : update .h comment #
Total comments: 7
Patch Set 3 : drop comma #
Messages
Total messages: 24 (0 generated)
Hi, Marijn and Albert. Please take a look. Regarding crbug.com/328637, I'd like to protect storage for disabled extension from GC. I'm not fully sure whether it's the root cause of the issue, but probably this also causes unexpected data lost.
On 2014/04/22 10:16:41, tzik wrote: > Hi, Marijn and Albert. Please take a look. > > Regarding crbug.com/328637, I'd like to protect storage for disabled extension > from GC. > I'm not fully sure whether it's the root cause of the issue, but probably this > also causes unexpected data lost. This seems to be the exact same patch I put up a few days ago as https://codereview.chromium.org/242203002/ :) But I'll gladly let you take this over since I really don't know that code anyway. Since this is the same patch, probably the same comments from Albert apply; the comment in the .h file should be updated to reflect that it doesn't only keep enabled extensions (and that comment seems to suggest that at some point this code was deliberately written to only account for enabled extensions, but I have no idea for what reason...)
On 2014/04/22 16:35:20, Marijn Kruisselbrink wrote: > On 2014/04/22 10:16:41, tzik wrote: > > Hi, Marijn and Albert. Please take a look. > > > > Regarding crbug.com/328637, I'd like to protect storage for disabled extension > > from GC. > > I'm not fully sure whether it's the root cause of the issue, but probably this > > also causes unexpected data lost. > This seems to be the exact same patch I put up a few days ago as > https://codereview.chromium.org/242203002/ :) But I'll gladly let you take this > over since I really don't know that code anyway. Since this is the same patch, > probably the same comments from Albert apply; the comment in the .h file should > be updated to reflect that it doesn't only keep enabled extensions (and that > comment seems to suggest that at some point this code was deliberately written > to only account for enabled extensions, but I have no idea for what reason...) Yep...repeating the crux of my comment again is that this looks right from a mechanical point of view, but I don't really understand extensions (never did...even when writing this code) so I don't know what the correct policy decision is. However, it does seem incorrect to remove storage from extensions that are not enabled but still "installed" for whatever reason.
On 2014/04/22 16:35:20, Marijn Kruisselbrink wrote: > On 2014/04/22 10:16:41, tzik wrote: > > Hi, Marijn and Albert. Please take a look. > > > > Regarding crbug.com/328637, I'd like to protect storage for disabled extension > > from GC. > > I'm not fully sure whether it's the root cause of the issue, but probably this > > also causes unexpected data lost. > This seems to be the exact same patch I put up a few days ago as > https://codereview.chromium.org/242203002/ :) But I'll gladly let you take this > over since I really don't know that code anyway. Since this is the same patch, > probably the same comments from Albert apply; the comment in the .h file should > be updated to reflect that it doesn't only keep enabled extensions (and that > comment seems to suggest that at some point this code was deliberately written > to only account for enabled extensions, but I have no idea for what reason...) I didn't notice, yes it's exactly same! Following the history, the comment was written to follow its behavior, in https://codereview.chromium.org/204983020/
On 2014/04/22 18:13:39, awong wrote: > On 2014/04/22 16:35:20, Marijn Kruisselbrink wrote: > > On 2014/04/22 10:16:41, tzik wrote: > > > Hi, Marijn and Albert. Please take a look. > > > > > > Regarding crbug.com/328637, I'd like to protect storage for disabled > extension > > > from GC. > > > I'm not fully sure whether it's the root cause of the issue, but probably > this > > > also causes unexpected data lost. > > This seems to be the exact same patch I put up a few days ago as > > https://codereview.chromium.org/242203002/ :) But I'll gladly let you take > this > > over since I really don't know that code anyway. Since this is the same patch, > > probably the same comments from Albert apply; the comment in the .h file > should > > be updated to reflect that it doesn't only keep enabled extensions (and that > > comment seems to suggest that at some point this code was deliberately written > > to only account for enabled extensions, but I have no idea for what reason...) > > Yep...repeating the crux of my comment again is that this looks right from a > mechanical point of view, but I don't really understand extensions (never > did...even when writing this code) so I don't know what the correct policy > decision is. However, it does seem incorrect to remove storage from extensions > that are not enabled but still "installed" for whatever reason. Hmm, we need an expert of the extension system to ask about it. Adding Rdevlin and Yoyo since they recently touched GC code. Rdevlin, Yoyo: Could you take a look to this or introduce the best person to ask?
On 2014/04/23 04:15:12, tzik wrote: > On 2014/04/22 16:35:20, Marijn Kruisselbrink wrote: > > On 2014/04/22 10:16:41, tzik wrote: > > > Hi, Marijn and Albert. Please take a look. > > > > > > Regarding crbug.com/328637, I'd like to protect storage for disabled > extension > > > from GC. > > > I'm not fully sure whether it's the root cause of the issue, but probably > this > > > also causes unexpected data lost. > > This seems to be the exact same patch I put up a few days ago as > > https://codereview.chromium.org/242203002/ :) But I'll gladly let you take > this > > over since I really don't know that code anyway. Since this is the same patch, > > probably the same comments from Albert apply; the comment in the .h file > should > > be updated to reflect that it doesn't only keep enabled extensions (and that > > comment seems to suggest that at some point this code was deliberately written > > to only account for enabled extensions, but I have no idea for what reason...) > > I didn't notice, yes it's exactly same! > Following the history, the comment was written to follow its behavior, in > https://codereview.chromium.org/204983020/ Ah yes, I hadn't noticed that particular part. In that case it seems like this code was indeed written with the incorrect assumption that the "extensions_" field at the time contained all extensions, instead of just all that happened to be enabled. In which case I think this is definitely the right thing to change. so LGTM
https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_garbage_collector.cc (right): https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.cc:224: ExtensionRegistry::Get(context_)->GenerateInstalledExtensionsSet(); It's a shame we have to keep around storage for all these extensions, since some of them might actually stay disabled indefinitely, but it's probably worse to get rid of it prematurely. What we could use is a disable time, and if they're disabled, for, say, > 7 days, delete storage, but I don't think we have anything like that (do we, Yoyo?). That said, I think we probably _should_ still delete storage of any extensions that wind up on the Blacklist. So only include Enabled, Disabled, and Terminated.
https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_garbage_collector.cc (right): https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.cc:224: ExtensionRegistry::Get(context_)->GenerateInstalledExtensionsSet(); It's a shame we have to keep around storage for all these extensions, since some of them might actually stay disabled indefinitely, but it's probably worse to get rid of it prematurely. What we could use is a disable time, and if they're disabled, for, say, > 7 days, delete storage, but I don't think we have anything like that (do we, Yoyo?). That said, I think we probably _should_ still delete storage of any extensions that wind up on the Blacklist. So only include Enabled, Disabled, and Terminated.
https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_garbage_collector.cc (right): https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.cc:224: ExtensionRegistry::Get(context_)->GenerateInstalledExtensionsSet(); On 2014/04/23 21:16:53, D Cronin wrote: > It's a shame we have to keep around storage for all these extensions, since some > of them might actually stay disabled indefinitely, but it's probably worse to > get rid of it prematurely. What we could use is a disable time, and if they're > disabled, for, say, > 7 days, delete storage, but I don't think we have anything > like that (do we, Yoyo?). It still feels wrong to delete data of an app just because the app is disabled... Suppose somebody only uses an app once a month, an update with increased permissions might cause the app to be disabled, and the user won't necessarily notice or care to re-enable the app until the next time it is used. If a user really wants to get rid of data they can always just uninstall the app, but doing so automatically just seems wrong to me. > That said, I think we probably _should_ still delete storage of any extensions > that wind up on the Blacklist. So only include Enabled, Disabled, and > Terminated. I'm not sure we really should do that immediately either. Right now putting an extension on a blacklist is something that is reversible. If we immediately delete all its data this is no longer the case. So I don't think we should treat this any different than otherwise disabled extensions.
https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_garbage_collector.cc (right): https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.cc:224: ExtensionRegistry::Get(context_)->GenerateInstalledExtensionsSet(); On 2014/04/23 23:09:51, Marijn Kruisselbrink wrote: > On 2014/04/23 21:16:53, D Cronin wrote: > > It's a shame we have to keep around storage for all these extensions, since > some > > of them might actually stay disabled indefinitely, but it's probably worse to > > get rid of it prematurely. What we could use is a disable time, and if > they're > > disabled, for, say, > 7 days, delete storage, but I don't think we have > anything > > like that (do we, Yoyo?). > It still feels wrong to delete data of an app just because the app is > disabled... Suppose somebody only uses an app once a month, an update with > increased permissions might cause the app to be disabled, and the user won't > necessarily notice or care to re-enable the app until the next time it is used. > If a user really wants to get rid of data they can always just uninstall the > app, but doing so automatically just seems wrong to me. > > > That said, I think we probably _should_ still delete storage of any extensions > > that wind up on the Blacklist. So only include Enabled, Disabled, and > > Terminated. > I'm not sure we really should do that immediately either. Right now putting an > extension on a blacklist is something that is reversible. If we immediately > delete all its data this is no longer the case. So I don't think we should treat > this any different than otherwise disabled extensions. Hmm... maybe I need more knowledge about what goes into isolated storage. I thought it was a very small (usually unimportant) amount, and that most data was stored in either prefs or through the chrome.storage API (which this shouldn't affect) -- which would mean that cleaning up isolated storage isn't a big deal. What is it, exactly, that goes into isolated storage? (By the way, I don't feel strongly about this, so don't feel blocked on my review).
Chrome Apps all have "Isolated storage." Thus this is all the storage for the app (cookies, local storage, indexdb, etc...) -Albert On 23 Apr 2014 16:21, <rdevlin.cronin@chromium.org> wrote: > > https://codereview.chromium.org/247113002/diff/20001/ > chrome/browser/extensions/extension_garbage_collector.cc > File chrome/browser/extensions/extension_garbage_collector.cc (right): > > https://codereview.chromium.org/247113002/diff/20001/ > chrome/browser/extensions/extension_garbage_collector.cc#newcode224 > chrome/browser/extensions/extension_garbage_collector.cc:224: > ExtensionRegistry::Get(context_)->GenerateInstalledExtensionsSet(); > On 2014/04/23 23:09:51, Marijn Kruisselbrink wrote: > >> On 2014/04/23 21:16:53, D Cronin wrote: >> > It's a shame we have to keep around storage for all these >> > extensions, since > >> some >> > of them might actually stay disabled indefinitely, but it's probably >> > worse to > >> > get rid of it prematurely. What we could use is a disable time, and >> > if > >> they're >> > disabled, for, say, > 7 days, delete storage, but I don't think we >> > have > >> anything >> > like that (do we, Yoyo?). >> It still feels wrong to delete data of an app just because the app is >> disabled... Suppose somebody only uses an app once a month, an update >> > with > >> increased permissions might cause the app to be disabled, and the user >> > won't > >> necessarily notice or care to re-enable the app until the next time it >> > is used. > >> If a user really wants to get rid of data they can always just >> > uninstall the > >> app, but doing so automatically just seems wrong to me. >> > > > That said, I think we probably _should_ still delete storage of any >> > extensions > >> > that wind up on the Blacklist. So only include Enabled, Disabled, >> > and > >> > Terminated. >> I'm not sure we really should do that immediately either. Right now >> > putting an > >> extension on a blacklist is something that is reversible. If we >> > immediately > >> delete all its data this is no longer the case. So I don't think we >> > should treat > >> this any different than otherwise disabled extensions. >> > > Hmm... maybe I need more knowledge about what goes into isolated > storage. I thought it was a very small (usually unimportant) amount, > and that most data was stored in either prefs or through the > chrome.storage API (which this shouldn't affect) -- which would mean > that cleaning up isolated storage isn't a big deal. What is it, > exactly, that goes into isolated storage? > > (By the way, I don't feel strongly about this, so don't feel blocked on > my review). > > https://codereview.chromium.org/247113002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
ah, okay, thanks for the explanations. lgtm.
LGTM w/ 2 small comment nits. https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_garbage_collector.h (right): https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.h:63: // Garbage collects apps/extensions isolated storage, if it is uninstalled. nit: storage, if -> storage if https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.h:64: // There is an exception for ephemeral apps, because they can outlive their nit: apps, because -> apps because
https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... File chrome/browser/extensions/extension_garbage_collector.h (right): https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.h:63: // Garbage collects apps/extensions isolated storage, if it is uninstalled. On 2014/04/24 16:42:11, awong wrote: > nit: storage, if -> storage if Done. https://codereview.chromium.org/247113002/diff/20001/chrome/browser/extension... chrome/browser/extensions/extension_garbage_collector.h:64: // There is an exception for ephemeral apps, because they can outlive their On 2014/04/24 16:42:11, awong wrote: > nit: apps, because -> apps because Done.
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/247113002/30001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on linux_chromium_gn_rel
The CQ bit was checked by tzik@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/tzik@chromium.org/247113002/30001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.chromium on win_chromium_rel
Message was sent while issue was closed.
Committed patchset #3 manually as r266473 (presubmit successful). |