ServiceWorker: Fix DCHECK failure during DB recovery.

ServiceWorker: Fix DCHECK failures during DB recovery.

1. Register client to quota manager after it is initialized.
As the ServiceWorker cache storage is not affected during DB
recovery we don't need to create a new ServiceWorkerCacheStorageManager.

2. Ongoing register jobs could be aborted by DB recovery.

BUG=none

Committed: https://crrev.com/3ccbc1aae4906c7bf46c91d9090db051c3dc7f44
Cr-Commit-Position: refs/heads/master@{#319044}

[xiang, 2015-02-25 08:18:44]

xiang.long@intel.com changed reviewers:
+ falken@chromium.org, michaeln@chromium.org

[xiang, 2015-02-25 08:19:23]

PTAL, thanks.

[michaeln, 2015-02-25 22:28:38]

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

https://codereview.chromium.org/943333007/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_cache_storage_manager.cc:115:
manager->quota_manager_proxy_ = old_manager->quota_manager_proxy_;
Good bug, i think the fix still needs some work.

This doesn't fully reconnect the new sw world to the quota system. There are two
parts to that interface.

1) SW calls QuotaProxy to notify it of usage as it occurs usage.

2) QuotaMgr calls SW via ServiceWorkerCacheQuotaClient to query it about who is
storing how much and to tell it to delete data per user actions and evicition
logic.

This reestablishes #1 but leaves #2 busted. The ServiceWorkerCacheQuotaClient's
weakptr to the old cachemgr is null after deleteandstartup.

The other quota client used by the sw (ServiceWorkerQuotaClient) is resilient to
deleteandstartover since it doesn't directly reference any objects that get
deleted when deleteandstartover happens. I'd consider having
ServiceWorkerCacheQuotaClient use indirection like that too.

I'm not sure how we tell the quota system NotifyStorageWiped(clientid)?

[xiang, 2015-02-27 09:20:48]

Thanks for the review, some comments/questions before change the CL.

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

https://codereview.chromium.org/943333007/diff/1/content/browser/service_work...
content/browser/service_worker/service_worker_cache_storage_manager.cc:115:
manager->quota_manager_proxy_ = old_manager->quota_manager_proxy_;
On 2015/02/25 22:28:38, michaeln wrote:
> Good bug, i think the fix still needs some work.
> 
> This doesn't fully reconnect the new sw world to the quota system. There are
two
> parts to that interface.
> 
> 1) SW calls QuotaProxy to notify it of usage as it occurs usage.
> 
> 2) QuotaMgr calls SW via ServiceWorkerCacheQuotaClient to query it about who
is
> storing how much and to tell it to delete data per user actions and evicition
> logic.
> 
> This reestablishes #1 but leaves #2 busted. The
ServiceWorkerCacheQuotaClient's
> weakptr to the old cachemgr is null after deleteandstartup.
> 

Thanks for pointing this out, I overlooked it.
How about add a "Restart" method which just clear the cache_storage_map_, and
calls it when context core want to startover again?

> The other quota client used by the sw (ServiceWorkerQuotaClient) is resilient
to
> deleteandstartover since it doesn't directly reference any objects that get
> deleted when deleteandstartover happens. I'd consider having
> ServiceWorkerCacheQuotaClient use indirection like that too.
> 

IMO ServiceWorkerContextWrapper seems a general SW interface which is not
suitable for handling SW Caches.

> I'm not sure how we tell the quota system NotifyStorageWiped(clientid)?

How about add one to quota manager proxy? From the code seems both
ServiceWorkerCacheQuotaClient & ServiceWorkerQuotaClient won't notify quota
change for recovery cleanup.

[michaeln, 2015-02-27 22:58:55]

What a minute, CacheStorage is not wiped by delete and start over, everything
remains intact! (which is fine, lets keep it that way)

Given that, a different solution might make more sense. Instead of deleting the
existing CacheStorageMgr and creating a new object, we could transfer ownership
of the existing mgr to the new ServiceWorkerContextCore object. The linkage with
the quotasystem would not be busted. Wdyt?

ServiceWorkerContextCore::ServiceWorkerContextCore(
    ServiceWorkerContextCore* old_context,
    ServiceWorkerContextWrapper* wrapper)
    : wrapper_(wrapper),
      providers_(old_context->providers_.release()),
      cache_manager_(old_context->cache_manager_.release()), <----- similar to
how providers are handled


> > ServiceWorkerCacheQuotaClient use indirection like that too.
> 
> IMO ServiceWorkerContextWrapper seems a general SW interface which is not
> suitable for handling SW Caches.

nit: Having the quota client hold a scoped_refptr<Wrapper> (and indirectly
getting the ptr to the cachmgr as needed), instead of holding a weakptr directly
to the cachemgr, doesn't mean the wrapper class would be handling sw caches. The
wrapper class is there to help integrate with other parts of the content lib and
the quota integration does fall into that category.

> > I'm not sure how we tell the quota system NotifyStorageWiped(clientid)?
> 
> How about add one to quota manager proxy? From the code seems both
> ServiceWorkerCacheQuotaClient & ServiceWorkerQuotaClient won't notify quota
> change for recovery cleanup.

The lack of a notifywiped api is probably out of scope for this cl (and also not
needed for this cl).

[xiang, 2015-03-03 08:53:24]

Thanks for the review. CL updated.

On 2015/02/27 22:58:55, michaeln wrote:
> What a minute, CacheStorage is not wiped by delete and start over, everything
> remains intact! (which is fine, lets keep it that way)
> 
> Given that, a different solution might make more sense. Instead of deleting
the
> existing CacheStorageMgr and creating a new object, we could transfer
ownership
> of the existing mgr to the new ServiceWorkerContextCore object. The linkage
with
> the quotasystem would not be busted. Wdyt?
> 
> ServiceWorkerContextCore::ServiceWorkerContextCore(
>     ServiceWorkerContextCore* old_context,
>     ServiceWorkerContextWrapper* wrapper)
>     : wrapper_(wrapper),
>       providers_(old_context->providers_.release()),
>       cache_manager_(old_context->cache_manager_.release()), <----- similar to
> how providers are handled
> 

Yes, this way is better. I should always read code more carefully...

> 
> > > ServiceWorkerCacheQuotaClient use indirection like that too.
> > 
> > IMO ServiceWorkerContextWrapper seems a general SW interface which is not
> > suitable for handling SW Caches.
> 
> nit: Having the quota client hold a scoped_refptr<Wrapper> (and indirectly
> getting the ptr to the cachmgr as needed), instead of holding a weakptr
directly
> to the cachemgr, doesn't mean the wrapper class would be handling sw caches.
The
> wrapper class is there to help integrate with other parts of the content lib
and
> the quota integration does fall into that category.
> 

Got it, Thanks! I was misunderstand with using Wrapper to proxy sw storage
operations like sw quota client.

[michaeln, 2015-03-03 20:14:11]

lgtm, thnx!

[falken, 2015-03-04 05:58:09]

Why are the changes to service_worker_registration_status.cc and
service_worker_script_cache_map.cc needed?

They look right, just wondering if they're related to the quota manager change.

[xiang, 2015-03-04 06:34:36]

On 2015/03/04 05:58:09, falken wrote:
> Why are the changes to service_worker_registration_status.cc and
> service_worker_script_cache_map.cc needed?
> 
> They look right, just wondering if they're related to the quota manager
change.

They are intended to fix CHECK/DCEHCK failure of aborting register job caused by
context DeleteAndStartOver DB.
Would it better to split them?

[falken, 2015-03-04 06:51:54]

On 2015/03/04 06:34:36, xiang wrote:
> On 2015/03/04 05:58:09, falken wrote:
> > Why are the changes to service_worker_registration_status.cc and
> > service_worker_script_cache_map.cc needed?
> > 
> > They look right, just wondering if they're related to the quota manager
> change.
> 
> They are intended to fix CHECK/DCEHCK failure of aborting register job caused
by
> context DeleteAndStartOver DB.
> Would it better to split them?

No, that's ok, but could you mention it in the CL description so future code
archaeologists can see why the change was made?

lgtm 2

[xiang, 2015-03-04 06:55:09]

On 2015/03/04 06:51:54, falken wrote:
> On 2015/03/04 06:34:36, xiang wrote:
> > On 2015/03/04 05:58:09, falken wrote:
> > > Why are the changes to service_worker_registration_status.cc and
> > > service_worker_script_cache_map.cc needed?
> > > 
> > > They look right, just wondering if they're related to the quota manager
> > change.
> > 
> > They are intended to fix CHECK/DCEHCK failure of aborting register job
caused
> by
> > context DeleteAndStartOver DB.
> > Would it better to split them?
> 
> No, that's ok, but could you mention it in the CL description so future code
> archaeologists can see why the change was made?
> 
> lgtm 2

I think so :), done.

[xiang, 2015-03-04 06:56:44]

The CQ bit was checked by xiang.long@intel.com

[xiang, 2015-03-04 06:57:17]

The CQ bit was unchecked by xiang.long@intel.com

[xiang, 2015-03-04 07:17:42]

The CQ bit was checked by xiang.long@intel.com

[commit-bot: I haz the power, 2015-03-04 07:18:48]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/943333007/20001

[commit-bot: I haz the power, 2015-03-04 09:48:56]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2015-03-04 09:49:40]

Patchset 2 (id:??) landed as
https://crrev.com/3ccbc1aae4906c7bf46c91d9090db051c3dc7f44
Cr-Commit-Position: refs/heads/master@{#319044}