Check the size of ResourceRecords in ServiceWorkerDatabase::ReadRegistration()

Check the size of ResourceRecords in ServiceWorkerDatabase::ReadRegistration()

The ResourceRecords must contain the ServiceWorker's main script.
So the size of it must be bigger than 1.

If the method returns STATUS_ERROR_CORRUPTED, DeleteAndStartOver() will be executed.

BUG=485900, 478732
Committed: https://crrev.com/9dd8083acc8e0cce593507bc5736adb6bd5a5a66
Cr-Commit-Position: refs/heads/master@{#329801}

[horo, 2015-05-08 08:05:37]

horo@chromium.org changed reviewers:
+ nhiroki@chromium.org

[horo, 2015-05-08 08:05:38]

nhiroki@
Could you please review this?

[nhiroki, 2015-05-08 08:14:42]

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_database.cc:533: if
(!resources->size())
empty()?

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_database.cc:579:
DCHECK(sequence_checker_.CalledOnValidSequencedThread());
How about adding DCHECK(!resources.empty()) here?

[nhiroki, 2015-05-08 08:19:00]

Sorry, one more comment

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_database.cc:533: if
(!resources->size())
On 2015/05/08 08:14:42, nhiroki wrote:
> empty()?

It would be better to add a comment about why this is treated as corruption like
CL description.

[horo, 2015-05-08 08:22:31]

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_database.cc:533: if
(!resources->size())
On 2015/05/08 08:19:00, nhiroki wrote:
> On 2015/05/08 08:14:42, nhiroki wrote:
> > empty()?
> 
> It would be better to add a comment about why this is treated as corruption
like
> CL description.

Done.

https://codereview.chromium.org/1135743002/diff/1/content/browser/service_wor...
content/browser/service_worker/service_worker_database.cc:579:
DCHECK(sequence_checker_.CalledOnValidSequencedThread());
On 2015/05/08 08:14:42, nhiroki wrote:
> How about adding DCHECK(!resources.empty()) here?

Done.

[nhiroki, 2015-05-08 08:24:34]

lgtm

[horo, 2015-05-08 08:33:07]

The CQ bit was checked by horo@chromium.org

[commit-bot: I haz the power, 2015-05-08 08:33:20]

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

[commit-bot: I haz the power, 2015-05-08 09:12:30]

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

[commit-bot: I haz the power, 2015-05-08 09:12:31]

Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[michaeln, 2015-05-08 20:41:07]

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

[michaeln, 2015-05-08 20:41:08]

https://codereview.chromium.org/1135743002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/1135743002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:582:
DCHECK(!resources.empty());
Should an attempt to write a reg with an empty resource list return an error? Do
we know w/o a doubt that we're not somehow writing registrations with an emtpy
resource list?

[horo, 2015-05-11 03:10:20]

https://codereview.chromium.org/1135743002/diff/40001/content/browser/service...
File content/browser/service_worker/service_worker_database.cc (right):

https://codereview.chromium.org/1135743002/diff/40001/content/browser/service...
content/browser/service_worker/service_worker_database.cc:582:
DCHECK(!resources.empty());
On 2015/05/08 20:41:08, michaeln wrote:
> Should an attempt to write a reg with an empty resource list return an error?
Do
> we know w/o a doubt that we're not somehow writing registrations with an emtpy
> resource list?

When WriteRegistration() is called, the registration must have at least one
resource.
It is because ServiceWorkerDatabase::WriteRegistration() is called from
ServiceWorkerStorage::StoreRegistration() which is called after the
ServiceWorker in installed.
While the ServiceWorker is being installed the resource for the main script must
be added to ResourceMap.

Is my understanding incorrect?

[horo, 2015-05-11 05:01:44]

I added check logic in ServiceWorkerStorage and updated tests.

nhiroki@
Could you please review this again?

[nhiroki, 2015-05-11 05:46:04]

Lgtm with nits. Please wait for michaeln@'s comments.

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:199: // avoid ID
corraption.
nit: dymmy -> dummy, corraption -> corruption (or collision?)

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:106: void
SimulateLoadWorkerMainScript(int embedded_worker_id);
nit: Can we make this function name consistent with others, for example,
SimulateWorkerScriptCached()?

[horo, 2015-05-11 07:41:30]

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.cc (right):

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.cc:199: // avoid ID
corraption.
On 2015/05/11 05:46:04, nhiroki wrote:
> nit: dymmy -> dummy, corraption -> corruption (or collision?)

Done.

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
File content/browser/service_worker/embedded_worker_test_helper.h (right):

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
content/browser/service_worker/embedded_worker_test_helper.h:106: void
SimulateLoadWorkerMainScript(int embedded_worker_id);
On 2015/05/11 05:46:04, nhiroki wrote:
> nit: Can we make this function name consistent with others, for example,
> SimulateWorkerScriptCached()?

Done.

[michaeln, 2015-05-11 19:05:10]

On 2015/05/11 03:10:20, horo wrote:
>
https://codereview.chromium.org/1135743002/diff/40001/content/browser/service...
> File content/browser/service_worker/service_worker_database.cc (right):
> 
>
https://codereview.chromium.org/1135743002/diff/40001/content/browser/service...
> content/browser/service_worker/service_worker_database.cc:582:
> DCHECK(!resources.empty());
> On 2015/05/08 20:41:08, michaeln wrote:
> > Should an attempt to write a reg with an empty resource list return an
error?
> Do
> > we know w/o a doubt that we're not somehow writing registrations with an
emtpy
> > resource list?
> 
> When WriteRegistration() is called, the registration must have at least one
> resource.
> It is because ServiceWorkerDatabase::WriteRegistration() is called from
> ServiceWorkerStorage::StoreRegistration() which is called after the
> ServiceWorker in installed.
> While the ServiceWorker is being installed the resource for the main script
must
> be added to ResourceMap.
> 
> Is my understanding incorrect?

That's what's supposed to be happening, but is it?

Simple things like main_script_status_ not being initialized can add up to
unintended consequences in cases. It's not clear that the problem lies in with
the database, there's a bunch of complicated code working up to the call to
WriteRegistration(). The CL assumes somethings is wrong with the DB. If that
assumption is off base, sw caches will be deleted unnecessarily.

[michaeln, 2015-05-11 19:22:24]

> > When WriteRegistration() is called, the registration must have at least one
> > resource.
> > It is because ServiceWorkerDatabase::WriteRegistration() is called from
> > ServiceWorkerStorage::StoreRegistration() which is called after the
> > ServiceWorker in installed.
> > While the ServiceWorker is being installed the resource for the main script
> must
> > be added to ResourceMap.
> > 
> > Is my understanding incorrect?
> 
> That's what's supposed to be happening, but is it?
> 
> Simple things like main_script_status_ not being initialized can add up to
> unintended consequences in cases. It's not clear that the problem lies in with
> the database, there's a bunch of complicated code working up to the call to
> WriteRegistration(). The CL assumes somethings is wrong with the DB. If that
> assumption is off base, sw caches will be deleted unnecessarily.

What about the ServiceWorkerRegistration::AbortPendingClear() callsite?

[horo, 2015-05-12 00:47:35]

On 2015/05/11 19:22:24, michaeln wrote:
> > > When WriteRegistration() is called, the registration must have at least
one
> > > resource.
> > > It is because ServiceWorkerDatabase::WriteRegistration() is called from
> > > ServiceWorkerStorage::StoreRegistration() which is called after the
> > > ServiceWorker in installed.
> > > While the ServiceWorker is being installed the resource for the main
script
> > must
> > > be added to ResourceMap.
> > > 
> > > Is my understanding incorrect?
> > 
> > That's what's supposed to be happening, but is it?
> > 
> > Simple things like main_script_status_ not being initialized can add up to
> > unintended consequences in cases. It's not clear that the problem lies in
with
> > the database, there's a bunch of complicated code working up to the call to
> > WriteRegistration(). The CL assumes somethings is wrong with the DB. If that
> > assumption is off base, sw caches will be deleted unnecessarily.

If there is a ServiceWorker registration database record which doesn't have any
resources.
the ServiceWorker can't start when the network is not connected.
I think that ServiceWorker database should not have such a broken registration
record, so we should delete it.

> What about the ServiceWorkerRegistration::AbortPendingClear() callsite?

In ServiceWorkerRegistration::AbortPendingClear(), StoreRegistration() is called
with waiting_version or active_version.
Both versions must have been installed and must have at least one resource.

[michaeln, 2015-05-12 01:25:18]

I just wanted to point out the the problem could be on the input rather than the
output side. I don't know why you're so convinced there can't possibly be a bug
that would provide an empty list as input to the Store method? If the db is
really corrupt seems like we'd be more likely to get leveldb errors out of it
than reads that complete with no results.

[horo, 2015-05-12 02:43:59]

On 2015/05/12 01:25:18, michaeln wrote:
> I just wanted to point out the the problem could be on the input rather than
the
> output side. I don't know why you're so convinced there can't possibly be a
bug
> that would provide an empty list as input to the Store method? If the db is
> really corrupt seems like we'd be more likely to get leveldb errors out of it
> than reads that complete with no results.

I think there could be a bug that StoreRegistration is called with zero
resource.
So I added a check logic which returns an error in
ServiceWorkerStorage::StoreRegistration() from Patch Set 4.

ServiceWorkerDatabase::WriteRegistration() is called only from
ServiceWorkerStorage::StoreRegistration().
So there is no problem with "DCHECK(!resources.empty());" in
ServiceWorkerDatabase::WriteRegistration().

[michaeln, 2015-05-14 03:27:59]

lgtm (but see question below)

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

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:542: if
(resources.empty()) {
thnx!

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:1102: if
(resources.empty()) {
Is this code reachable with your changes to service_worker_database.cc? I think
all ReturnFoundRegistration callsites check the status code prior to coming in
here. If thats right, maybe just put a DCHECK here and trust the callers will
handle delete and start over.

[horo, 2015-05-14 03:46:50]

Thank you!!

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

https://codereview.chromium.org/1135743002/diff/60001/content/browser/service...
content/browser/service_worker/service_worker_storage.cc:1102: if
(resources.empty()) {
On 2015/05/14 03:27:59, michaeln wrote:
> Is this code reachable with your changes to service_worker_database.cc? I
think
> all ReturnFoundRegistration callsites check the status code prior to coming in
> here. If thats right, maybe just put a DCHECK here and trust the callers will
> handle delete and start over.

Ah, yes.
It should be DCHECK(!resources.empty());

[horo, 2015-05-14 04:02:29]

The CQ bit was checked by horo@chromium.org

[horo, 2015-05-14 04:02:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
nhiroki@chromium.org, michaeln@chromium.org
Link to the patchset: https://codereview.chromium.org/1135743002/#ps100001
(title: "DCHECK in ServiceWorkerStorage::ReturnFoundRegistration()")

[commit-bot: I haz the power, 2015-05-14 04:02:38]

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

[commit-bot: I haz the power, 2015-05-14 04:43:23]

Committed patchset #6 (id:100001)

[commit-bot: I haz the power, 2015-05-14 04:44:25]

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