Don't prematurely delete script resources when registration is deleted

Don't prematurely delete script resources when registration is deleted

A page must be controlled by the same worker for its lifetime, so as long as a
worker controls a page its resources shouldn't be deleted.

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

[falken, 2014-06-27 12:55:37]

Michael,

Just sharing an early patch. It's not ready for review but comments are welcome.

The big changes are:
- SW::Storage DeleteRegistration doesn't purge version resources
- SWUnregisterJob and SWVersion call DeleteVersionResources when appropriate

On thing I'm wondering about this design if you DeleteRegistration, then a
browser crash/shutdown happens before DeleteVersionResources is called, does
SWStorage already know to purge those resources on restart? (there's a comment
for the current DeleteRegistration hinting that they're discarded on browser
restart)

[michaeln, 2014-06-28 01:47:25]

On 2014/06/27 12:55:37, falken wrote:
> Michael,
> 
> Just sharing an early patch. It's not ready for review but comments are
welcome.
> 
> The big changes are:
> - SW::Storage DeleteRegistration doesn't purge version resources
> - SWUnregisterJob and SWVersion call DeleteVersionResources when appropriate

i'll take a look

> On thing I'm wondering about this design if you DeleteRegistration, then a
> browser crash/shutdown happens before DeleteVersionResources is called, does
> SWStorage already know to purge those resources on restart? (there's a comment
> for the current DeleteRegistration hinting that they're discarded on browser
> restart)

That cleanup is partly handled. Upon return from DeleteRegistration the
resourceids have been moved to the purgeable list in the database (that code is
written). However there is no code start the process of purging things in that
list (but there is code to incrementally purge things from other inputs). There
is other cleanup that is also only partly handled. Suppose that during an
install prior to the new registration/version being added to the database, the
browser crashes. There can be resources on disk in that case, but no database
entries referring to them. They should be cleaned up. The ids for those
resources are in an 'uncommitted' list in the database. The contents of that
list needs to be moved to the 'purgeable' list and then purged sometime after
new browser startup.

The class comments in the Storage class header describe how its intended for the
class to behave, but it's not yet fully implemented. The DB class should be
fully implemented.

[michaeln, 2014-06-28 02:29:32]

thnx for the early preview

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_database.cc (left):

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_database.cc:629: // Delete
resource records associated with the registration.
We do want to delete the records from the 'resource' table at this time.

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_database.h:142: Status
DeleteRegistration(int64 registration_id, const GURL& origin);
The database class should not need to change. It doesn't delete resources it
only moves resourceids from one list to another. The newly_purgeable_resources
return value are those that have been moved to the purgeable list as a result of
this deletion. It's important that all the db changes made by this function
happen in a transaction otherwise there are things in the resource table for
versions that don't exist.

In the schema there are 3 resourceid related tables.

Uncommitted - key: "URES:" + <int64 'uncommitted_resource_id'>
Resource - "RES:" + <int64 'version_id'> + '\x00' + <int64 'resource_id'>
Purgeable - "PRES:" + <int64 'purgeable_resource_id'>


While a version is 'installing', the resourceids for scripts its downloading are
added to the uncommitted list (search for StoreUncommittedReponseId). As part of
committing the registration to store in StoreRegistration, the ids are moved
from the uncommitted list to the 'resource' list. As part of the
DeleteRegistration function, the ids are moved from the resource list to the
purgeable list.

Also as part of StoreRegistration, if the we're overrwriting a previous version
with a new version. The old versions resources are moved to the purgeable list
atomically with the rest of the db updates.

Its done that way so if the program crashes at any point, we have enough info to
cleanup upon restart.

Uncommitted --> dont need those, installations that never completed/committed
Resources --> need all of these
Purgeable --> dont need these either

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_database.h:144: Status
DeleteVersionResources(int64 version_id,
The seperate DeleteVersionResources method should not be needed.

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_storage.cc (left):

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:695:
StartPurgingResources(newly_purgeable_resources);
ditto defer this call...

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_storage.cc:722:
StartPurgingResources(newly_purgeable_resources);
Just the startpurging should be deferred till nomo controlleee time.

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/355163003/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_version.cc:401:
context_->storage()->DeleteVersionResources(
I this should StartPurging()

[falken, 2014-06-30 10:41:28]

Thanks for the detailed explanations! How does this look?

I was originally a bit reluctant to allow the worker's resources to move to the
"purgeable list" when they shouldn't be purged while the worker is controlling a
page, but now I see SWStorage drives when to actually do the purging, so that
looks fine to me now.

[michaeln, 2014-06-30 22:31:55]

this is lookin pretty good

> I was originally a bit reluctant to allow the worker's resources to move to
the
> "purgeable list" when they shouldn't be purged while the worker is controlling
a
> page, but now I see SWStorage drives when to actually do the purging, so that
> looks fine to me now.

yup, we'll have to be careful when it comes to automatically purging after a
restart, that mechanism can't accidentally purge these newly purgeable resources
that are still lingering

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_context_observer.h (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_context_observer.h:63: virtual
void OnNoControllees(int64 version_id) {}
this isn't used, delete it?

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:124:
deleted_version_resource_ids_.erase(it);
maybe vers->removelistener(this) so theres less of an opportunity for the DCHECK
on 122 to  happen

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:702:
StartPurgingResources(newly_purgeable_resources);
Do these need to be handled in the same fashion?

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.h:74: virtual void
OnNoControllees(ServiceWorkerVersion* version) OVERRIDE;
please put this method in the private section, its an impl detail that this
class implements this method and not part of the api consumers of this class
should be calling directly

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:392:
FOR_EACH_OBSERVER(Listener, listeners_, OnNoControllees(this));
if (!HasControllee()) {
  ScheduleStopWorker(
  NotifiyNoControllees(...);  // only if no controllees
}

[falken, 2014-07-01 06:17:40]

Thanks, PTAL

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_context_observer.h (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_context_observer.h:63: virtual
void OnNoControllees(int64 version_id) {}
On 2014/06/30 22:31:54, michaeln wrote:
> this isn't used, delete it?

Oops, yes, this is from another try.

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:124:
deleted_version_resource_ids_.erase(it);
On 2014/06/30 22:31:54, michaeln wrote:
> maybe vers->removelistener(this) so theres less of an opportunity for the
DCHECK
> on 122 to  happen

Done.

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:702:
StartPurgingResources(newly_purgeable_resources);
On 2014/06/30 22:31:54, michaeln wrote:
> Do these need to be handled in the same fashion?

Done, I planned to do that in a second patch but easy enough to do now.

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_storage.h (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.h:74: virtual void
OnNoControllees(ServiceWorkerVersion* version) OVERRIDE;
On 2014/06/30 22:31:54, michaeln wrote:
> please put this method in the private section, its an impl detail that this
> class implements this method and not part of the api consumers of this class
> should be calling directly

Done.

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
File content/browser/service_worker/service_worker_version.cc (right):

https://codereview.chromium.org/355163003/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_version.cc:392:
FOR_EACH_OBSERVER(Listener, listeners_, OnNoControllees(this));
On 2014/06/30 22:31:54, michaeln wrote:
> if (!HasControllee()) {
>   ScheduleStopWorker(
>   NotifiyNoControllees(...);  // only if no controllees
> }

oops! Done

[michaeln, 2014-07-01 20:54:07]

lgtm modulo some nits

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database.h:120: // Returns OK they
are successfully written. Otherwise, returns an error.
also describe the output params here

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database.h:141: // id of the
version that was stored and |newly_purgeable_resources| to its
'version that was deleted'

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_database_unittest.cc (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database_unittest.cc:480:
&newly_purgeable_resources));
add an expectation for version_id here

also i don't think theres an expectation for
DeleteRegistration(kNotRegiesteredId..) anywhere, i think the function returns
OK in that case, please add expectation version_id in that case too.

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database_unittest.cc:520: data,
resources1, &deleted_version_id, &newly_purgeable_resources));
add expectation for deleted_version_id here

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database_unittest.cc:542:
&newly_purgeable_resources));
and here too

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:848:
deleted_version_resource_ids_.end());
if newly_purgeable_resources.empty(), don't bother

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:983: int64 version_id =
kInvalidServiceWorkerVersionId;
nit: maybe call this deleted_version_id

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:541: virtual
void TearDown() OVERRIDE { ServiceWorkerStorageTest::TearDown(); }
not needed

[falken, 2014-07-02 04:04:25]

Thanks, addressed comments and will CQ. The updated patch contains a bit of new
test code so you might want to take a look (you can view diff against patchset 7
to see the changes).

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_database.h (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database.h:120: // Returns OK they
are successfully written. Otherwise, returns an error.
On 2014/07/01 20:54:06, michaeln wrote:
> also describe the output params here

Done.

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database.h:141: // id of the
version that was stored and |newly_purgeable_resources| to its
On 2014/07/01 20:54:06, michaeln wrote:
> 'version that was deleted'

Done.

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_database_unittest.cc (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database_unittest.cc:480:
&newly_purgeable_resources));
On 2014/07/01 20:54:06, michaeln wrote:
> add an expectation for version_id here
> 
> also i don't think theres an expectation for
> DeleteRegistration(kNotRegiesteredId..) anywhere, i think the function returns
> OK in that case, please add expectation version_id in that case too.

Done:
- Modified WriteRegistration and DeleteRegistration to set |version_id| to -1
when nothing is deleted.
- Added new test DeleteNonExistentRegistration

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database_unittest.cc:520: data,
resources1, &deleted_version_id, &newly_purgeable_resources));
On 2014/07/01 20:54:06, michaeln wrote:
> add expectation for deleted_version_id here

Done.

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_database_unittest.cc:542:
&newly_purgeable_resources));
On 2014/07/01 20:54:06, michaeln wrote:
> and here too

Done.

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_storage.cc (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:848:
deleted_version_resource_ids_.end());
On 2014/07/01 20:54:06, michaeln wrote:
> if newly_purgeable_resources.empty(), don't bother

good point, done

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:983: int64 version_id =
kInvalidServiceWorkerVersionId;
On 2014/07/01 20:54:07, michaeln wrote:
> nit: maybe call this deleted_version_id

Done.

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/355163003/diff/100001/content/browser/service...
content/browser/service_worker/service_worker_storage_unittest.cc:541: virtual
void TearDown() OVERRIDE { ServiceWorkerStorageTest::TearDown(); }
On 2014/07/01 20:54:07, michaeln wrote:
> not needed

oops, done

[falken, 2014-07-02 04:05:05]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-07-02 04:05:34]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/falken@chromium.org/355163003/140001

[commit-bot: I haz the power, 2014-07-02 05:46:07]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  win_chromium_rel on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/win_chromium_rel/buil...)

[commit-bot: I haz the power, 2014-07-02 07:47:25]

Change committed as 280976

[falken, 2014-07-03 04:40:34]

Actually I'm not sure what you mean by this:

> yup, we'll have to be careful when it comes to automatically purging after a
> restart, that mechanism can't accidentally purge these newly purgeable
resources
> that are still lingering

A browser restart means new document loads so they'll use the updated version,
no?

i.e., you can delete the purgeable resources when the browser starts up?

(was thinking of looking into that for my next patch)