Service Worker: Respect the "clear on exit" content setting

Service Worker: Respect the "clear on exit" content setting

Now Service Worker registrations and data get deleted as per
SpecialStoragePolicy.

BUG=419280
Committed: https://crrev.com/7253028818568d4233acc2031fe0a82f6d8d99b9
Cr-Commit-Position: refs/heads/master@{#299842}

[falken, 2014-10-07 09:24:13]

falken@chromium.org changed reviewers:
+ michaeln@chromium.org

[falken, 2014-10-07 09:24:14]

PTAL (unittest is in progress)

[michaeln, 2014-10-08 01:22:27]

lgtm modulo auto nits

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

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:701: for (auto origin
: origins) {
whooohoooo, hooray for c++ 11

per the notes at http://chromium-cpp.appspot.com/, please bind a reference
instead of making a copy, so either:

const GURL& origin
or
const auto& origin

In this case, i think the explicit GURL type is best practice since its both
more informative and fewer characters.

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:715:
registrations.begin(); itr != registrations.end(); ++itr) {
oh, care to for for range'ify this loop too?

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

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_database_unittest.cc:999:
origin_to_delete.insert(GURL(origin1));
origin1 is of type GURL so no need to construct a temporary for the arg (?)

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

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:1186: for (auto origin
: registered_origins_) {
ditto const GURL&

[michaeln, 2014-10-08 01:23:48]

oh, i'll take a look at the tests too if you like

[falken, 2014-10-08 02:36:44]

Thanks! Remediated to your review and added a test.

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

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:701: for (auto origin
: origins) {
On 2014/10/08 01:22:27, michaeln wrote:
> whooohoooo, hooray for c++ 11
> 
> per the notes at http://chromium-cpp.appspot.com/, please bind a reference
> instead of making a copy, so either:
> 
> const GURL& origin
> or
> const auto& origin
> 
> In this case, i think the explicit GURL type is best practice since its both
> more informative and fewer characters.

I see. Done.

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_database.cc:715:
registrations.begin(); itr != registrations.end(); ++itr) {
On 2014/10/08 01:22:27, michaeln wrote:
> oh, care to for for range'ify this loop too?

Done.

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

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_database_unittest.cc:999:
origin_to_delete.insert(GURL(origin1));
On 2014/10/08 01:22:27, michaeln wrote:
> origin1 is of type GURL so no need to construct a temporary for the arg (?)

Oops, that was leftover from trying to make the set. Done.

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

https://codereview.chromium.org/633873002/diff/20001/content/browser/service_...
content/browser/service_worker/service_worker_storage.cc:1186: for (auto origin
: registered_origins_) {
On 2014/10/08 01:22:27, michaeln wrote:
> ditto const GURL&

Done.

[falken, 2014-10-08 03:51:28]

falken@chromium.org changed reviewers:
+ jochen@chromium.org

[falken, 2014-10-08 03:51:29]

+jochen can you please review:
  content/browser/storage_partition_impl.cc

[jochen (gone - plz use gerrit), 2014-10-08 14:56:42]

jochen@chromium.org changed reviewers:
+ markusheintz@chromium.org

[jochen (gone - plz use gerrit), 2014-10-08 14:56:43]

basically the same as the cookie content settings CL

[michaeln1, 2014-10-08 19:28:56]

michaeln@google.com changed reviewers:
+ michaeln@google.com

[michaeln1, 2014-10-08 19:28:57]

test lgtm 2

https://codereview.chromium.org/633873002/diff/60001/content/browser/service_...
File content/browser/service_worker/service_worker_storage_unittest.cc (right):

https://codereview.chromium.org/633873002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:869:
cleared_registration->waiting_version()->SetStatus(ServiceWorkerVersion::NEW);
to more closely mimic actual usage, i think the waiting versions status should
be INSTALLED

https://codereview.chromium.org/633873002/diff/60001/content/browser/service_...
content/browser/service_worker/service_worker_storage_unittest.cc:894:
FindRegistrationForId(cleared_registration_id,
maybe test that it actually can be found prior to similating restart

[jochen (gone - plz use gerrit), 2014-10-09 11:42:05]

can somebody help me understand how we ensure that all origins are deleted
before shutdown proceeds?

https://codereview.chromium.org/633873002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_database_unittest.cc (right):

https://codereview.chromium.org/633873002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_database_unittest.cc:998:
std::set<GURL> origin_to_delete;
super nit. origin*s*_to_delete

[falken, 2014-10-09 13:34:26]

On 2014/10/09 11:42:05, jochen wrote:
> can somebody help me understand how we ensure that all origins are deleted
> before shutdown proceeds?

Hm, that's a good question.

StoragePartitionImpl::~StoragePartitionImpl() calls
ServiceWorkerContextWrapper::Shutdown on the IO thread which posts a task to
reset ServiceWorkerContextCore on the UI thread. Resetting context core destroys
ServiceWorkerStorage whose destructor calls ClearSessionOnlyOrigins() which
posts a task on the Service Worker db task runner to
DeleteAllDataForOriginsFromDB. The db task runner is created as SKIP_ON_SHUTDOWN
in ServiceWorkerContextWrapper::Init.

Does the task runner need to be BLOCK_SHUTDOWN?

[jochen (gone - plz use gerrit), 2014-10-09 13:44:37]

yeah, I think so.

Michael should know, he wrote that stuff IIRC :)

[michaeln, 2014-10-09 19:56:24]

> Does the task runner need to be BLOCK_SHUTDOWN?

Good question. Not all the tasks, but this one task scheduled in the storage
dtor should be.

[michaeln, 2014-10-09 19:58:37]

> Michael should know, he wrote that stuff IIRC :)

I helped add some features and fix some bugs is all (and was a big fan of the
SWP coming into being :)

[dominicc (has gone to gerrit), 2014-10-10 01:46:59]

I wonder if that will be sufficient to fix crbug.com/419290 as well, then.

[falken, 2014-10-10 09:33:44]

On 2014/10/09 19:56:24, michaeln wrote:
> > Does the task runner need to be BLOCK_SHUTDOWN?
> 
> Good question. Not all the tasks, but this one task scheduled in the storage
> dtor should be.

Is there a recommended way to make just one task BLOCK_SHUTDOWN? The ways I'm
seeing need special care for the unit tests. Possibilities:

1) Give database_task_runner_ a known sequence token name. In the dtor use
PostSequencedWorkerTaskWithShutdownBehavior directly with the token, instead of
using the runner object. Then the unit tests must properly construct a task
runner from the blocking pool with that sequence token (instead of just
base::ThreadTaskRunnerHandle::Get), and will get more complex from
multi-threading.

2) Introduce a ServiceWorkerDatabaseTaskRunner class, with methods PostTask and
PostShutdownBlockingTask, which uses a fixed
sequence token like 1). But for unit tests use a
MockSeviceWorkerDatabaseTaskRunner which just posts to
base::ThreadTaskRunnerHandle::Get().

Any thoughts, is there a simpler way I'm missing?

[jochen (gone - plz use gerrit), 2014-10-10 11:14:59]

other systems do (2), so I'd go with that

[michaeln1, 2014-10-10 19:32:57]

> I wonder if that will be sufficient to fix crbug.com/419290 as well, then.

No, i really don't think so. The only tasks posted to the 'taskrunner' are those
that read/write the database deep inside the storage class. Everyting put on
that background thread is well isolated from everything else. The crash is that
bug has to do with profile/browsingcontext/storagepartitionimpl shutdown
sequence which is all executing on the UI thread. Looks like that sequence
relies on there being no running renderers (at least where it interacts with the
ExtensionSystem) , but the ServiceWorkerProcessManager has kept a process going
that violates the expectation.

[michaeln1, 2014-10-10 19:45:21]

> > > Does the task runner need to be BLOCK_SHUTDOWN?
> > 
> > Good question. Not all the tasks, but this one task scheduled in the storage
> > dtor should be.
> Is there a recommended way to make just one task BLOCK_SHUTDOWN? The ways I'm
> seeing need special care for the unit tests. Possibilities:
> 
> 1) Give database_task_runner_ a known sequence token name. In the dtor use
> PostSequencedWorkerTaskWithShutdownBehavior directly with the token, instead
of
> using the runner object. Then the unit tests must properly construct a task
> runner from the blocking pool with that sequence token (instead of just
> base::ThreadTaskRunnerHandle::Get), and will get more complex from
> multi-threading.
> 
> 2) Introduce a ServiceWorkerDatabaseTaskRunner class, with methods PostTask
and
> PostShutdownBlockingTask, which uses a fixed
> sequence token like 1). But for unit tests use a
> MockSeviceWorkerDatabaseTaskRunner which just posts to
> base::ThreadTaskRunnerHandle::Get().
> 
> Any thoughts, is there a simpler way I'm missing?

#1 doesn't play well with testing. #2 does and we did something like that in the
domstorage system. The interface could vend a seperate SequencedTaskRunners for
the blocking behavior:
normalRunnerDerivative->GetShutdownBlockingTaskRunner()->post().

[falken, 2014-10-14 09:14:29]

Thanks for the advice. Now it blocks on shutdown. PTAL.

https://codereview.chromium.org/633873002/diff/80001/content/browser/service_...
File content/browser/service_worker/service_worker_database_unittest.cc (right):

https://codereview.chromium.org/633873002/diff/80001/content/browser/service_...
content/browser/service_worker/service_worker_database_unittest.cc:998:
std::set<GURL> origin_to_delete;
On 2014/10/09 11:42:05, jochen wrote:
> super nit. origin*s*_to_delete

Done.

[jochen (gone - plz use gerrit), 2014-10-14 13:56:56]

i defer to Michael for service_worker/*

rest lgtm

[michaeln, 2014-10-16 01:15:05]

lgtm

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_database_task_manager.cc
(right):

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_database_task_manager.cc:22:
pool->GetNamedSequenceToken(kServiceWorkerDatabaseTaskSequenceTokenName);
In the existing code, each swcontext uses a unique token. Is there a reason to
alter that existing behavior by using the same named token for all swcontexts?
If not, probably should continue to use a unique token per-context. So tasks
from different swcontexts don't impede one another.
 token = pool->GetSequenceToken();

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_database_task_manager.h
(right):

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_database_task_manager.h:43:
scoped_refptr<base::SequencedWorkerPool> pool_;
May not need the pool_ data member, the Clone method can construct a new
instance with new refs to old's task runners.

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

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:251:
old_storage->database_task_manager_->Clone(),
Ah, I was wondering what clone() was for.

[falken, 2014-10-16 02:04:51]

Thanks for the reviews.

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_database_task_manager.cc
(right):

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_database_task_manager.cc:22:
pool->GetNamedSequenceToken(kServiceWorkerDatabaseTaskSequenceTokenName);
On 2014/10/16 01:15:04, michaeln wrote:
> In the existing code, each swcontext uses a unique token. Is there a reason to
> alter that existing behavior by using the same named token for all swcontexts?
> If not, probably should continue to use a unique token per-context. So tasks
> from different swcontexts don't impede one another.
>  token = pool->GetSequenceToken();

Good point! Done.

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
File content/browser/service_worker/service_worker_database_task_manager.h
(right):

https://codereview.chromium.org/633873002/diff/190001/content/browser/service...
content/browser/service_worker/service_worker_database_task_manager.h:43:
scoped_refptr<base::SequencedWorkerPool> pool_;
On 2014/10/16 01:15:04, michaeln wrote:
> May not need the pool_ data member, the Clone method can construct a new
> instance with new refs to old's task runners.

Good point also, done.

[falken, 2014-10-16 02:06:01]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-10-16 02:08:53]

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

[commit-bot: I haz the power, 2014-10-16 02:16:18]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-10-16 02:16:20]

Try jobs failed on following builders:
  linux_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/linux_gpu/builds/...)
  mac_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/mac_gpu/builds/69237)
  win_gpu on tryserver.chromium.gpu
(http://build.chromium.org/p/tryserver.chromium.gpu/builders/win_gpu/builds/73907)
  android_aosp on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_aosp/bu...)
  android_arm64_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_arm64_d...)
  android_chromium_gn_compile_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_chromium_gn_compile_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_chromiu...)
  android_clang_dbg_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)
  chromium_presubmit on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_chromeos_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32 on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_dbg on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_rel on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_swarming on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_compile_dbg on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_swarming on tryserver.chromium.mac
(http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_rel on tryserver.chromium.win
(http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...)

[falken, 2014-10-16 03:05:56]

The CQ bit was checked by falken@chromium.org

[commit-bot: I haz the power, 2014-10-16 03:07:43]

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

[commit-bot: I haz the power, 2014-10-16 04:46:57]

Committed patchset #9 (id:230001)

[commit-bot: I haz the power, 2014-10-16 04:47:55]

Patchset 9 (id:??) landed as
https://crrev.com/7253028818568d4233acc2031fe0a82f6d8d99b9
Cr-Commit-Position: refs/heads/master@{#299842}