Get ride of chrome.storage.* data when removing an application.

Get ride of chrome.storage.* data when removing an application.

This is only fixing packaged apps, the bug is not affecting hosted apps.

BUG=314803
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243887

[benwells, 2014-01-07 00:13:59]

I'm not a good reviewer for this. Assigning fsamuel who was going to look at it
(so I assume he has knowledge about it).

[benwells, 2014-01-08 02:27:04]

Actually removing myself this time.

Also adding kalman who might be more comfortable reviewing this.

[not at google - send to devlin, 2014-01-08 02:49:17]

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1316: ExtensionTestMessageListener
launched_listener("Launched", false);
perhaps give the first half of the test the {} scoping?

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1317:
LoadAndLaunchPlatformApp("reinstall_data_cleanup");
assert that this gets the same ID as before

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/data_deleter.cc (right):

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
chrome/browser/extensions/data_deleter.cc:47: DeleteNonIsolatedStorage(profile,
extension_id, launch_web_url_origin);
interesting that we do this

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
chrome/browser/extensions/data_deleter.cc:58: void
DataDeleter::DeleteNonIsolatedStorage(Profile* profile,
nit: name is a bit misleading. this deletes the storage on |storage_origin| for
the storage partition corresponding to |extension_id| - which since this isn't a
platform app is just the global storage partition for the profile.

anyway, my nit would be to call this DeleteOrigin and take the StoragePartition
rather than the extension ID. also saves looking up the storage partition twice
for hosted apps.

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2781: void
ExtensionService::OnNeedsToGarbageCollectIsolatedStorage() {
move this into DataDeleter.

do you still need to friend DataDeleter after that?

https://codereview.chromium.org/123733003/diff/70001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js
(right):

https://codereview.chromium.org/123733003/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:6: var
gFailed = false;
instead of all this global state can testChromeStorage take a callback for once
the test has finished? that would let you chain these together more easily

https://codereview.chromium.org/123733003/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:24:
backend.set( { 'foo': 'bar'}, function() {
nit: whitespace

[mlamouri (slow - plz ping), 2014-01-08 17:42:31]

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/apps/app_...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1316: ExtensionTestMessageListener
launched_listener("Launched", false);
On 2014/01/08 02:49:17, kalman wrote:
> perhaps give the first half of the test the {} scoping?

It does not provide much advantage but why not.

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/apps/app_...
chrome/browser/apps/app_browsertest.cc:1317:
LoadAndLaunchPlatformApp("reinstall_data_cleanup");
On 2014/01/08 02:49:17, kalman wrote:
> assert that this gets the same ID as before

OK.

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/data_deleter.cc (right):

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
chrome/browser/extensions/data_deleter.cc:58: void
DataDeleter::DeleteNonIsolatedStorage(Profile* profile,
On 2014/01/08 02:49:17, kalman wrote:
> nit: name is a bit misleading. this deletes the storage on |storage_origin|
for
> the storage partition corresponding to |extension_id| - which since this isn't
a
> platform app is just the global storage partition for the profile.
> 
> anyway, my nit would be to call this DeleteOrigin and take the
StoragePartition
> rather than the extension ID. also saves looking up the storage partition
twice
> for hosted apps.

Done.

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2781: void
ExtensionService::OnNeedsToGarbageCollectIsolatedStorage() {
On 2014/01/08 02:49:17, kalman wrote:
> move this into DataDeleter.
> 
> do you still need to friend DataDeleter after that?

I would have to get the extension_prefs from the extension_service and then make
DataDeleter inherits from base::SupportsWeakPtr<DataDeleter>. Given the
relationship between DataDeleter and ExtensionService, it seems fair to have
DataService be a friend of ExtensionService. Though, if you prefer not to, I can
probably make the change.

https://codereview.chromium.org/123733003/diff/70001/chrome/test/data/extensi...
File chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js
(right):

https://codereview.chromium.org/123733003/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:6: var
gFailed = false;
On 2014/01/08 02:49:17, kalman wrote:
> instead of all this global state can testChromeStorage take a callback for
once
> the test has finished? that would let you chain these together more easily

Sure. I did the change, it is quite better.

https://codereview.chromium.org/123733003/diff/70001/chrome/test/data/extensi...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:24:
backend.set( { 'foo': 'bar'}, function() {
On 2014/01/08 02:49:17, kalman wrote:
> nit: whitespace 

Done.

[mlamouri (slow - plz ping), 2014-01-08 17:43:50]

kalman@, PTAL

[not at google - send to devlin, 2014-01-08 18:01:36]

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
File chrome/browser/extensions/extension_service.cc (right):

https://codereview.chromium.org/123733003/diff/70001/chrome/browser/extension...
chrome/browser/extensions/extension_service.cc:2781: void
ExtensionService::OnNeedsToGarbageCollectIsolatedStorage() {
On 2014/01/08 17:42:31, Mounir Lamouri wrote:
> On 2014/01/08 02:49:17, kalman wrote:
> > move this into DataDeleter.
> > 
> > do you still need to friend DataDeleter after that?
> 
> I would have to get the extension_prefs from the extension_service and then
make
> DataDeleter inherits from base::SupportsWeakPtr<DataDeleter>. Given the
> relationship between DataDeleter and ExtensionService, it seems fair to have
> DataService be a friend of ExtensionService. Though, if you prefer not to, I
can
> probably make the change.

Yeah I think it would be better to reduce the footprint of ES (i.e. make this
change), and make the co-dependency implicit rather than explicit.

extension_prefs() is public on ES so you don't need to friend DataDeleter to
have access to that.

Note that the more popular pattern (to hide the weakptr-ness of this class'
implementation) is to have a WeakPtrFactory<DataDeleter> as a member of this
class.

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest.cc:1322: const Extension* reloaded_extension
=
might as well call this "extension"?

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest.cc:1325: ASSERT_EQ(reloaded_extension->id(),
extension_id);
nit: expected value should be 1st argument.

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/data_deleter.cc (right):

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/data_deleter.cc:32: std::string extension_id =
extension->id();
should be a const std::string&, however, IMO this variable doesn't improve the
code

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/data_deleter.h (right):

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/data_deleter.h:16: };
no ;

https://codereview.chromium.org/123733003/diff/270001/chrome/test/data/extens...
File chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js
(right):

https://codereview.chromium.org/123733003/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:6:
backend.get('foo', function(result) {
wrap this function(result) in a chrome.test.callbackPass to make sure this never
goes onto the next test, and automatically assert that there was no
chrome.runtime.lastError

https://codereview.chromium.org/123733003/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:14:
backend.get('foo', function(result) {
likewise here and the line above. often people alias chrome.test.callbackPass to
"pass".

https://codereview.chromium.org/123733003/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:16:
chrome.test.notifyFail('chrome storage unexpected failure');
use chrome.test.assertEq('bar', result.foo) for a better failure message and
less code.

the test will exit on failure, so you can just run callback() straight
afterwards:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/renderer/re...

https://codereview.chromium.org/123733003/diff/270001/chrome/test/data/extens...
chrome/test/data/extensions/platform_apps/reinstall_data_cleanup/test.js:25:
testChromeStorage(chrome.storage.local, callback);
callback flow much nicer, and TBH not even sure if these methods are necessary
anymore.

[mlamouri (slow - plz ping), 2014-01-08 23:44:08]

For the tests, given your comments, I simply used chrome.test.runTests(). This
is what you actually want I think ;)

Regarding the WeakPtr-ness of DataDeleter, it is worth pointing that DataDeleter
is a collection of static functions. If we want to make the callback for GC in
DataDeleter it will have to be a proper object instantiated in the stack when
needed. Again, it sounds a bit overkill given what we want to do but if you
think it is worth it, I will do the change.

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest.cc:1322: const Extension* reloaded_extension
=
On 2014/01/08 18:01:36, kalman wrote:
> might as well call this "extension"?

Indeed. I did the scoping of the other block after. It explains why.

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest.cc:1325: ASSERT_EQ(reloaded_extension->id(),
extension_id);
On 2014/01/08 18:01:36, kalman wrote:
> nit: expected value should be 1st argument.

Done. But it's not that intuitive given that the coding style says that
conditions should be |if (foo == 42)| instead of |if (42 == foo)|.

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/data_deleter.cc (right):

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/data_deleter.cc:32: std::string extension_id =
extension->id();
On 2014/01/08 18:01:36, kalman wrote:
> should be a const std::string&, however, IMO this variable doesn't improve the
> code

Agreed. It's not that needed. That code changed a bit so maybe extension_id was
used more when I added it.

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
File chrome/browser/extensions/data_deleter.h (right):

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/extensio...
chrome/browser/extensions/data_deleter.h:16: };
On 2014/01/08 18:01:36, kalman wrote:
> no ;

Done.

[mlamouri (slow - plz ping), 2014-01-08 23:48:58]

benwells@, I need you to review the app_browsertest.cc changes (at least, I need
an official stamping it seems).

[not at google - send to devlin, 2014-01-08 23:59:02]

On 2014/01/08 23:44:08, Mounir Lamouri wrote:
> For the tests, given your comments, I simply used chrome.test.runTests(). This
> is what you actually want I think ;)
> 
> Regarding the WeakPtr-ness of DataDeleter, it is worth pointing that
DataDeleter
> is a collection of static functions. If we want to make the callback for GC in
> DataDeleter it will have to be a proper object instantiated in the stack when
> needed. Again, it sounds a bit overkill given what we want to do but if you
> think it is worth it, I will do the change.
> 

I think it can be done more simply than that. You could make
OnNeedsToGarbageCollectIsolatedStorage take a WeakPtr<ExtensionService> as its
first argument, then retrieve the ExtensionPrefs from that if it's still alive.

namespace {

void OnNeedsToGarbageCollectIsolatedStorage(WeakPtr<ExtensionService> es) {
  if (!es) return;
  es->extension_prefs()->SetNeedsStorageGarbageCollection(true);
}

...

}

[not at google - send to devlin, 2014-01-08 23:59:52]

lgtm with a couple more comments

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
File chrome/browser/apps/app_browsertest.cc (right):

https://codereview.chromium.org/123733003/diff/270001/chrome/browser/apps/app...
chrome/browser/apps/app_browsertest.cc:1325: ASSERT_EQ(reloaded_extension->id(),
extension_id);
On 2014/01/08 23:44:08, Mounir Lamouri wrote:
> On 2014/01/08 18:01:36, kalman wrote:
> > nit: expected value should be 1st argument.
> 
> Done. But it's not that intuitive given that the coding style says that
> conditions should be |if (foo == 42)| instead of |if (42 == foo)|.

It's just one of those things:
https://code.google.com/p/chromium/codesearch#chromium/src/testing/gtest/incl...

https://codereview.chromium.org/123733003/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/data_deleter.h (right):

https://codereview.chromium.org/123733003/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/data_deleter.h:33: static void DeleteOrigin(
just realised that this doesn't need to be on the interface, it can be in an
anonymous namespace within the .cc file.

[not at google - send to devlin, 2014-01-09 00:00:06]

On 2014/01/08 23:59:52, kalman wrote:
> lgtm with a couple more comments
> 


(and that last one)

[mlamouri (slow - plz ping), 2014-01-09 00:08:17]

On 2014/01/08 23:59:02, kalman wrote:
> On 2014/01/08 23:44:08, Mounir Lamouri wrote:
> > For the tests, given your comments, I simply used chrome.test.runTests().
This
> > is what you actually want I think ;)
> > 
> > Regarding the WeakPtr-ness of DataDeleter, it is worth pointing that
> DataDeleter
> > is a collection of static functions. If we want to make the callback for GC
in
> > DataDeleter it will have to be a proper object instantiated in the stack
when
> > needed. Again, it sounds a bit overkill given what we want to do but if you
> > think it is worth it, I will do the change.
> > 
> 
> I think it can be done more simply than that. You could make
> OnNeedsToGarbageCollectIsolatedStorage take a WeakPtr<ExtensionService> as its
> first argument, then retrieve the ExtensionPrefs from that if it's still
alive.
> 
> namespace {
> 
> void OnNeedsToGarbageCollectIsolatedStorage(WeakPtr<ExtensionService> es) {
>   if (!es) return;
>   es->extension_prefs()->SetNeedsStorageGarbageCollection(true);
> }
> 
> ...
> 
> }

Isn't Bind doing that for us here? This is my understanding of that at least:
http://www.chromium.org/developers/design-documents/threading#base_WeakPtr_an...

[mlamouri (slow - plz ping), 2014-01-09 00:09:29]

https://codereview.chromium.org/123733003/diff/350001/chrome/browser/extensio...
File chrome/browser/extensions/data_deleter.h (right):

https://codereview.chromium.org/123733003/diff/350001/chrome/browser/extensio...
chrome/browser/extensions/data_deleter.h:33: static void DeleteOrigin(
On 2014/01/08 23:59:53, kalman wrote:
> just realised that this doesn't need to be on the interface, it can be in an
> anonymous namespace within the .cc file.

Sure. But it makes me wonder why we keep this as a class instead of having a
namespace.

[benwells, 2014-01-09 00:12:46]

lgtm stamp

(kalman you should be an owner in apps)

[not at google - send to devlin, 2014-01-09 00:24:57]

On 2014/01/09 00:08:17, Mounir Lamouri wrote:
> On 2014/01/08 23:59:02, kalman wrote:
> > On 2014/01/08 23:44:08, Mounir Lamouri wrote:
> > > For the tests, given your comments, I simply used chrome.test.runTests().
> This
> > > is what you actually want I think ;)
> > > 
> > > Regarding the WeakPtr-ness of DataDeleter, it is worth pointing that
> > DataDeleter
> > > is a collection of static functions. If we want to make the callback for
GC
> in
> > > DataDeleter it will have to be a proper object instantiated in the stack
> when
> > > needed. Again, it sounds a bit overkill given what we want to do but if
you
> > > think it is worth it, I will do the change.
> > > 
> > 
> > I think it can be done more simply than that. You could make
> > OnNeedsToGarbageCollectIsolatedStorage take a WeakPtr<ExtensionService> as
its
> > first argument, then retrieve the ExtensionPrefs from that if it's still
> alive.
> > 
> > namespace {
> > 
> > void OnNeedsToGarbageCollectIsolatedStorage(WeakPtr<ExtensionService> es) {
> >   if (!es) return;
> >   es->extension_prefs()->SetNeedsStorageGarbageCollection(true);
> > }
> > 
> > ...
> > 
> > }
> 
> Isn't Bind doing that for us here? This is my understanding of that at least:
>
http://www.chromium.org/developers/design-documents/threading#base_WeakPtr_an...

not sure what you're asking. Yes Bind can also take a WeakPtr as the target
object of an invocation, but my point is that you can have the ES as an argument
to the Bound function too, which would let you pull that method out of ES and
into data_deleter.

[commit-bot: I haz the power, 2014-01-09 11:07:31]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mlamouri@chromium.org/123733003/460001

[commit-bot: I haz the power, 2014-01-09 14:09:41]

Change committed as 243887