IndexedDB: Ignore open/delete requests from terminated renderers.

IndexedDB: Ignore open/delete requests from terminated renderers.

Tasks associated with transactions are terminated when a dispatcher
host's connection goes away, but queued connection open/delete
requests are not associated with transactions, meaning attempting to
send the results will fail. To avoid this, check to see if the
dispatcher is still connected when a request gets to the head of the
queue.

BUG=650635
R=cmumford@chromium.org

Committed: https://crrev.com/94384e5f7af8064d0b8812efe1bcdfa8709ff6cd
Cr-Commit-Position: refs/heads/master@{#422840}

[jsbell, 2016-09-30 23:41:02]

The CQ bit was checked by jsbell@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-30 23:41:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-01 00:38:10]

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

[commit-bot: I haz the power, 2016-10-01 00:38:11]

Dry run: This issue passed the CQ dry run.

[jsbell, 2016-10-03 17:22:19]

cmumford@ - please take a look?

I'm not incredibly thrilled with this patch and it will likely collide with
mojoification, but it seems correct for now?

[cmumford, 2016-10-03 18:09:41]

On 2016/10/03 17:22:19, jsbell wrote:
> cmumford@ - please take a look?
> 
> I'm not incredibly thrilled with this patch and it will likely collide with
> mojoification, but it seems correct for now?

Reviewing, but fix "check check" in description.

[jsbell, 2016-10-03 18:20:39]

Description was changed from

==========
IndexedDB: Ignore open/delete requests from terminated renderers.

Tasks associated with transactions are terminated when a dispatcher
host's connection goes away, but queued connection open/delete
requests are not associated with transactions, meaning attempting to
send the results will fail. To avoid this, check check to see if the
dispatcher is still connected when a request gets to the head of the
queue.

BUG=650635
R=cmumford@chromium.org
==========

to

==========
IndexedDB: Ignore open/delete requests from terminated renderers.

Tasks associated with transactions are terminated when a dispatcher
host's connection goes away, but queued connection open/delete
requests are not associated with transactions, meaning attempting to
send the results will fail. To avoid this, check to see if the
dispatcher is still connected when a request gets to the head of the
queue.

BUG=650635
R=cmumford@chromium.org
==========

[cmumford, 2016-10-03 18:39:27]

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_callbacks.cc:608: return
dispatcher_host_->is_open();
All the other methods have:

  DCHECK(dispatcher_host_.get());

Here too?

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:298: if
(!callbacks_->IsValid()) {
I can see completing the open if disconnecting, but does this mean that if a
render process immediately closes after sending a delete request then that
request won't be implemented? This feels like something we'd still want to do.

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:98: is_open_ = false;
Do you need |is_open|, or can you just use database_dispatcher_host_?

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:120: bool is_open()
const { return is_open_; }
What about moving this into the *.cc file and adding:

  DCHECK(indexed_db_context_->TaskRunner()->RunsTasksOnCurrentThread());

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/mock_indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/mock_indexed_db_callbacks.cc:36: bool
MockIndexedDBCallbacks::IsValid() const { return true; }
clang-format wants to change this (and a few other things) in this CL.

[jsbell, 2016-10-03 19:28:37]

Thanks for the initial pass. Does a lack of comments on the overall approach
and/or test indicate that this seems reasonable, or are you still chewing on it?

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_callbacks.cc:608: return
dispatcher_host_->is_open();
On 2016/10/03 18:39:26, cmumford wrote:
> All the other methods have:
> 
>   DCHECK(dispatcher_host_.get());
> 
> Here too?

Done.

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:298: if
(!callbacks_->IsValid()) {
On 2016/10/03 18:39:26, cmumford wrote:
> I can see completing the open if disconnecting, but does this mean that if a
> render process immediately closes after sending a delete request then that
> request won't be implemented? This feels like something we'd still want to do.

Could go either way, yeah. I decided this way for symmetry since we can't report
the delete as being successful and/or reporting data loss (although the renderer
could just crash later in the process, too - no guarantee the message ever gets
there).

How strongly do you feel about letting the delete proceed?

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:98: is_open_ = false;
On 2016/10/03 18:39:26, cmumford wrote:
> Do you need |is_open|, or can you just use database_dispatcher_host_?

It proceeds the CloseAll() which can trigger side-effects like unblocking
the connection queue. I'll add a comment.

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:120: bool is_open()
const { return is_open_; }
On 2016/10/03 18:39:27, cmumford wrote:
> What about moving this into the *.cc file and adding:
> 
>   DCHECK(indexed_db_context_->TaskRunner()->RunsTasksOnCurrentThread());

Done.

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/mock_indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/mock_indexed_db_callbacks.cc:36: bool
MockIndexedDBCallbacks::IsValid() const { return true; }
On 2016/10/03 18:39:27, cmumford wrote:
> clang-format wants to change this (and a few other things) in this CL.

Done.

[cmumford, 2016-10-03 20:03:34]

lgtm

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:298: if
(!callbacks_->IsValid()) {
My main worry is that a normal (i.e. not-crash) shutdown of a render process
could initiate the delete request and then shutdown before the IDB thread could
process the request. That has a privacy aspect to it, so my preference is to
honor the request even though the page is gone.

In all honesty I don't feel strongly about this, but wanted to point it out.

[jsbell, 2016-10-03 20:57:58]

Thanks cmumford@ - one more look?

ISTM we need similar logic if the open/delete request gets delayed, i.e. if
Perform() doesn't immediately proceed into the main logic
(StartUpgrade/DoDelete). I'd like to tackle that in a follow-up, since it'll
require additional tests.

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
File content/browser/indexed_db/indexed_db_database.cc (right):

https://codereview.chromium.org/2386683003/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_database.cc:298: if
(!callbacks_->IsValid()) {
On 2016/10/03 20:03:34, cmumford wrote:
> My main worry is that a normal (i.e. not-crash) shutdown of a render process
> could initiate the delete request and then shutdown before the IDB thread
could
> process the request. That has a privacy aspect to it, so my preference is to
> honor the request even though the page is gone.

That's a very good point, I hadn't considered that scenario. I agree we should
let deletes proceed.

[cmumford, 2016-10-04 16:19:49]

lgtm

[jsbell, 2016-10-04 16:29:18]

The CQ bit was checked by jsbell@chromium.org

[commit-bot: I haz the power, 2016-10-04 16:29:45]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-10-04 17:20:16]

Description was changed from

==========
IndexedDB: Ignore open/delete requests from terminated renderers.

Tasks associated with transactions are terminated when a dispatcher
host's connection goes away, but queued connection open/delete
requests are not associated with transactions, meaning attempting to
send the results will fail. To avoid this, check to see if the
dispatcher is still connected when a request gets to the head of the
queue.

BUG=650635
R=cmumford@chromium.org
==========

to

==========
IndexedDB: Ignore open/delete requests from terminated renderers.

Tasks associated with transactions are terminated when a dispatcher
host's connection goes away, but queued connection open/delete
requests are not associated with transactions, meaning attempting to
send the results will fail. To avoid this, check to see if the
dispatcher is still connected when a request gets to the head of the
queue.

BUG=650635
R=cmumford@chromium.org
==========

[commit-bot: I haz the power, 2016-10-04 17:20:19]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-10-04 17:25:44]

Description was changed from

==========
IndexedDB: Ignore open/delete requests from terminated renderers.

Tasks associated with transactions are terminated when a dispatcher
host's connection goes away, but queued connection open/delete
requests are not associated with transactions, meaning attempting to
send the results will fail. To avoid this, check to see if the
dispatcher is still connected when a request gets to the head of the
queue.

BUG=650635
R=cmumford@chromium.org
==========

to

==========
IndexedDB: Ignore open/delete requests from terminated renderers.

Tasks associated with transactions are terminated when a dispatcher
host's connection goes away, but queued connection open/delete
requests are not associated with transactions, meaning attempting to
send the results will fail. To avoid this, check to see if the
dispatcher is still connected when a request gets to the head of the
queue.

BUG=650635
R=cmumford@chromium.org

Committed: https://crrev.com/94384e5f7af8064d0b8812efe1bcdfa8709ff6cd
Cr-Commit-Position: refs/heads/master@{#422840}
==========

[commit-bot: I haz the power, 2016-10-04 17:25:46]

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