[IndexedDB] Closing mojo connections when renderer quits

[IndexedDB] Closing mojo connections when renderer exits.

Changes IndexedDBDispatcherHost to be owned by a unique_ptr instead of
ref counted, and makes the mojo connections for the database and the
cursor interfaces strongly stored (owned) by the dispatcher host.

The dispatcher host now supports weak pointers, which are used by the
IndexedDB.*Callbacks (still refcounted, and probably by necessity) to
ignore responses when the host is gone. The host also observes renderer
exit - which can happen well before the render process host is
destroyed - to destruct the bindings and clear the weak pointers.

Finally, minor cleanup includes minimizing dependencies, adding the
connection error listener on IndexedDBDatabaseCallbacks to cut that off
as early as possible, and formally create the IDBHelper on the
dispatcher host to make the idb thread separation cleaner.

BUG=696055
Review-Url: https://codereview.chromium.org/2727733004
Cr-Commit-Position: refs/heads/master@{#463786}
Committed: https://chromium.googlesource.com/chromium/src/+/1f18ca937688b4e0ee570b4d506f5ac21ead5b93

[dmurph, 2017-03-03 01:34:13]

dmurph@chromium.org changed reviewers:
+ cmumford@chromium.org, jsbell@chromium.org, pwnall@chromium.org,
reillyg@chromium.org

[dmurph, 2017-03-03 01:34:14]

Hello all,

This patch should fix the one major change from the mojo switch, and closes the
mojo connections as soon as the renderer quits. This should be totally fine (and
more efficient), as all of those pending transactions would have been aborted
anyways, as they need confirmation w/ the renderer.

Daniel

[pwnall, 2017-03-03 01:41:12]

Is the dependency on the other CL necessary? Also, can you explain how you test
to make sure the code is acting intended? A TEST= line might be helpful if this
can't be covered by automated tests.

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:105:
RecordMetrics();
Is this a leftover from a different CL?

[Reilly Grant (use Gerrit), 2017-03-03 01:51:47]

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:23: #include
"mojo/public/cpp/bindings/strong_associated_binding_set.h"
This header is missing from the patch.

[Reilly Grant (use Gerrit), 2017-03-03 01:59:59]

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/cursor_impl.h:50: IndexedDBDispatcherHost*
dispatcher_host_;
Add comment:

// This raw pointer is safe because all CursorImpl instances are owned by an
IndexedDBDispatcherHost.

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/database_impl.h:121: IndexedDBDispatcherHost*
dispatcher_host_;
Add comment:

// This raw pointer is safe because all DatabaseImpl instances are owned by an
IndexedDBDispatcherHost.

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:58: void
SetDatabaseImpl(std::unique_ptr<DatabaseImpl> database) {
This function is unused and doesn't seem right to me because there could be
multiple database connections.

[dmurph, 2017-03-04 00:39:26]

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

[dmurph, 2017-03-04 00:39:41]

I'm still trying to figure out how to test this in the best way.

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/cursor_impl.h:50: IndexedDBDispatcherHost*
dispatcher_host_;
On 2017/03/03 01:59:59, Reilly Grant wrote:
> Add comment:
> 
> // This raw pointer is safe because all CursorImpl instances are owned by an
> IndexedDBDispatcherHost.

Done.

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/database_impl.h:121: IndexedDBDispatcherHost*
dispatcher_host_;
On 2017/03/03 01:59:59, Reilly Grant wrote:
> Add comment:
> 
> // This raw pointer is safe because all DatabaseImpl instances are owned by an
> IndexedDBDispatcherHost.

Done.

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:23: #include
"mojo/public/cpp/bindings/strong_associated_binding_set.h"
On 2017/03/03 01:51:47, Reilly Grant wrote:
> This header is missing from the patch.

Done.

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_dispatcher_host.h:58: void
SetDatabaseImpl(std::unique_ptr<DatabaseImpl> database) {
On 2017/03/03 01:59:59, Reilly Grant wrote:
> This function is unused and doesn't seem right to me because there could be
> multiple database connections.

Done.

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

https://codereview.chromium.org/2727733004/diff/1/content/browser/indexed_db/...
content/browser/indexed_db/indexed_db_transaction_coordinator.cc:105:
RecordMetrics();
On 2017/03/03 01:41:12, pwnall wrote:
> Is this a leftover from a different CL?

removed - I had the trace url dependency in here.

[commit-bot: I haz the power, 2017-03-04 00:39:42]

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

[dmurph, 2017-03-04 00:40:09]

dmurph@chromium.org changed reviewers:
+ rockot@chromium.org

[dmurph, 2017-03-04 00:40:10]

+rockot for mojo changes.

[Reilly Grant (use Gerrit), 2017-03-04 01:26:14]

lgtm with nits

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:3299:
IDB_TRACE("IndexedDBBackingStore::Cursor::ContinueNext");
The changes in this file look unrelated.

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.h:19: #include
"content/browser/indexed_db/database_impl.h"
This include is unnecessary.

[commit-bot: I haz the power, 2017-03-04 01:34:33]

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

[commit-bot: I haz the power, 2017-03-04 01:34:34]

Dry run: This issue passed the CQ dry run.

[cmumford, 2017-03-07 20:24:23]

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/associated_binding.h (right):

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/associated_binding.h:99: // |Bind| method. Does not
take ownership of |impl|, which must outlive this
Doesn't moving |impl| mean that we're taking ownership of |impl|?

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/strong_associated_binding_set.h (right):

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/strong_associated_binding_set.h:19: Interface,
AssociatedBinding<Interface, UniquePtrImplRefTraits<Interface>>,
Nit: clang-format wants param #2 on own line.

[dmurph, 2017-03-07 20:26:06]

Note: This patch needs a bit more lifetime changes. Will take a little more time
- basically removing refptrs as much as possible.

[Ken Rockot(use gerrit already), 2017-03-07 20:27:45]

Sorry I seem to have missed this review. LGTM

[Ken Rockot(use gerrit already), 2017-03-07 20:30:50]

On 2017/03/07 at 20:27:45, Ken Rockot wrote:
> Sorry I seem to have missed this review. LGTM

(for addition of StrongAssociatedBindingSet, anyway)

Also I partially agree with cmumford's comment. I would just remove the text
about taking ownership from the comment on AssociatedBinding, since the
ownership behavior is a property of the ImplRefTraits specialization now. i.e.
while moving a unique_ptr<Foo> transfers ownership, moving a Foo* does nothing
at all (and doesn't imply ownership transfer)

[dmurph, 2017-03-09 20:40:39]

Description was changed from

==========
[IndexedDB] Closing mojo connections when renderer quits

BUG=696055
==========

to

==========
[IndexedDB] Closing mojo connections when renderer exits.

Changes IndexedDBDispatcherHost to be owned by a unique_ptr instead of ref
counted, and makes the mojo connections for the database and the cursor
interfaces strongly stored (owned) by the dispatcher host.

The dispatcher host now supports weak pointers, which are used by the
IndexedDB.*Callbacks (still refcounted, and probably by necessity) to ignore
responses when the host is gone. The host also observes renderer exit - which
can happen well before the render process host is destroyed - to destruct the
bindings and clear the weak pointers.

Finally, minor cleanup includes minimizing dependencies, adding the connection
error listener on IndexedDBDatabaseCallbacks to cut that off as early as
possible, and formally create the IDBHelper on the dispatcher host to make the
idb thread separation cleaner.

BUG=696055
==========

[dmurph, 2017-03-09 20:41:13]

Description was changed from

==========
[IndexedDB] Closing mojo connections when renderer exits.

Changes IndexedDBDispatcherHost to be owned by a unique_ptr instead of ref
counted, and makes the mojo connections for the database and the cursor
interfaces strongly stored (owned) by the dispatcher host.

The dispatcher host now supports weak pointers, which are used by the
IndexedDB.*Callbacks (still refcounted, and probably by necessity) to ignore
responses when the host is gone. The host also observes renderer exit - which
can happen well before the render process host is destroyed - to destruct the
bindings and clear the weak pointers.

Finally, minor cleanup includes minimizing dependencies, adding the connection
error listener on IndexedDBDatabaseCallbacks to cut that off as early as
possible, and formally create the IDBHelper on the dispatcher host to make the
idb thread separation cleaner.

BUG=696055
==========

to

==========
[IndexedDB] Closing mojo connections when renderer exits.

Changes IndexedDBDispatcherHost to be owned by a unique_ptr instead of
ref counted, and makes the mojo connections for the database and the
cursor interfaces strongly stored (owned) by the dispatcher host.

The dispatcher host now supports weak pointers, which are used by the
IndexedDB.*Callbacks (still refcounted, and probably by necessity) to
ignore responses when the host is gone. The host also observes renderer
exit - which can happen well before the render process host is
destroyed - to destruct the bindings and clear the weak pointers.

Finally, minor cleanup includes minimizing dependencies, adding the
connection error listener on IndexedDBDatabaseCallbacks to cut that off
as early as possible, and formally create the IDBHelper on the
dispatcher host to make the idb thread separation cleaner.

BUG=696055
==========

[dmurph, 2017-03-09 20:44:15]

Hello all, can you PTAL at this patch?

I updated the patch description to try to encompass all of the changes here.
There are three unresolved things:
1. Instead of adding the host as an observer, I can just have the
RenderProcessHostImpl manually call a method, so we we don't have ordering
issues. Not sure if this would be a thing.
2. I need to add a test that destroys / exits the renderer BEFORE the mojo
connections are closed. This actually seems impossible right now - it looks like
the legacy ipc system associated binding close before I'm notified of renderer
exit.
3. I need to obviously update all the test code so it compiles.

Daniel

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_backing_store.cc (right):

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_backing_store.cc:3299:
IDB_TRACE("IndexedDBBackingStore::Cursor::ContinueNext");
On 2017/03/04 01:26:14, Reilly Grant wrote:
> The changes in this file look unrelated.

Done.

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_dispatcher_host.h (right):

https://codereview.chromium.org/2727733004/diff/20001/content/browser/indexed...
content/browser/indexed_db/indexed_db_dispatcher_host.h:19: #include
"content/browser/indexed_db/database_impl.h"
On 2017/03/04 01:26:14, Reilly Grant wrote:
> This include is unnecessary.

Done.

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/associated_binding.h (right):

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/associated_binding.h:99: // |Bind| method. Does not
take ownership of |impl|, which must outlive this
On 2017/03/07 20:24:23, cmumford wrote:
> Doesn't moving |impl| mean that we're taking ownership of |impl|?

Done.

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
File mojo/public/cpp/bindings/strong_associated_binding_set.h (right):

https://codereview.chromium.org/2727733004/diff/20001/mojo/public/cpp/binding...
mojo/public/cpp/bindings/strong_associated_binding_set.h:19: Interface,
AssociatedBinding<Interface, UniquePtrImplRefTraits<Interface>>,
On 2017/03/07 20:24:23, cmumford wrote:
> Nit: clang-format wants param #2 on own line.

Done.

[Reilly Grant (use Gerrit), 2017-03-10 00:11:22]

https://codereview.chromium.org/2727733004/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2727733004/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.h:110: IndexedDBDispatcherHost*
dispatcher_host_;
I'm less confident that this is safe unless we can be sure that destruction of
an IndexedDBConnection guarantees that this will be freed.

[dmurph, 2017-03-10 23:40:26]

Still pre-tests update patch.

https://codereview.chromium.org/2727733004/diff/80001/content/browser/indexed...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2727733004/diff/80001/content/browser/indexed...
content/browser/indexed_db/indexed_db_callbacks.h:110: IndexedDBDispatcherHost*
dispatcher_host_;
On 2017/03/10 00:11:22, Reilly Grant wrote:
> I'm less confident that this is safe unless we can be sure that destruction of
> an IndexedDBConnection guarantees that this will be freed.

Removed. All raw pointers are removed except for ones we can prove lifetime
ownership.

[Reilly Grant (use Gerrit), 2017-03-11 00:31:40]

Looking good.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/database_impl.cc:463: LOG(ERROR) << "about to call
closed";
Leftover logging.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:266: }
nit: No braces around single-line ifs.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:529: if (database) {
|database| will always be non-null.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:67: };
DISALLOW_COPY_AND_ASSIGN(IDBThreadHelper);

[dmurph, 2017-03-21 23:29:05]

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

[commit-bot: I haz the power, 2017-03-21 23:29:18]

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, 2017-03-21 23:45:00]

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

[commit-bot: I haz the power, 2017-03-21 23:45:01]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[pwnall, 2017-03-27 21:55:12]

The code LGTM w/ nits, with the caveat that I haven't taken a close look at
mojo's API yet.

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/cursor_impl.cc (right):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/cursor_impl.cc:36: IndexedDBDispatcherHost*
dispatcher_host,
Nice job getting rid of ref counting here!

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (left):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:995:
IDB_TRACE("IndexedDBBackingStore::IndexCursorOptions");
Is this intentional?

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:3262:
IDB_TRACE("IndexedDBBackingStore::Cursor::Continue");
Is this also intentional?

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:261: // Only create a new
Database if the connection was not previously sent in
Is "Database" still accurate here?

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/mock_indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/mock_indexed_db_callbacks.cc:9: #include
"base/threading/thread_task_runner_handle.h"
why?

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/mock_indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/mock_indexed_db_database_callbacks.cc:7: #include
"content/browser/indexed_db/indexed_db_context_impl.h"
why?

[dmurph, 2017-03-29 18:38:46]

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

[commit-bot: I haz the power, 2017-03-29 18:39:48]

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

[dmurph, 2017-03-29 18:40:25]

I removed the testing in favor of doing the lifetime testing (which currently
never happens - only would occur if we mojoify totally) into a different patch
here: 
https://codereview.chromium.org/2784003002

as it's rather complex and I need more help from the mojo group.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/database_impl.cc:463: LOG(ERROR) << "about to call
closed";
On 2017/03/11 00:31:40, Reilly Grant wrote:
> Leftover logging.

Done.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:266: }
On 2017/03/11 00:31:40, Reilly Grant wrote:
> nit: No braces around single-line ifs.

Done.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:529: if (database) {
On 2017/03/11 00:31:40, Reilly Grant wrote:
> |database| will always be non-null.

Done.

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2727733004/diff/120001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:67: };
On 2017/03/11 00:31:40, Reilly Grant wrote:
> DISALLOW_COPY_AND_ASSIGN(IDBThreadHelper);

Done.

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_backing_store.cc (left):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:995:
IDB_TRACE("IndexedDBBackingStore::IndexCursorOptions");
On 2017/03/27 21:55:12, pwnall wrote:
> Is this intentional?

Done.

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/indexed_db_backing_store.cc:3262:
IDB_TRACE("IndexedDBBackingStore::Cursor::Continue");
On 2017/03/27 21:55:12, pwnall wrote:
> Is this also intentional?

Done.

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:261: // Only create a new
Database if the connection was not previously sent in
On 2017/03/27 21:55:12, pwnall wrote:
> Is "Database" still accurate here?

Done.

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/mock_indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/mock_indexed_db_callbacks.cc:9: #include
"base/threading/thread_task_runner_handle.h"
On 2017/03/27 21:55:12, pwnall wrote:
> why?

Done.

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
File content/browser/indexed_db/mock_indexed_db_database_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/140001/content/browser/indexe...
content/browser/indexed_db/mock_indexed_db_database_callbacks.cc:7: #include
"content/browser/indexed_db/indexed_db_context_impl.h"
On 2017/03/27 21:55:12, pwnall wrote:
> why?

Done.

[commit-bot: I haz the power, 2017-03-29 19:11:12]

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

[commit-bot: I haz the power, 2017-03-29 19:11:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmurph, 2017-03-29 22:55:30]

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

[dmurph, 2017-03-29 22:55:41]

This patch is so good that it exposed a previously rare race condition. Fixed
that as well.

[commit-bot: I haz the power, 2017-03-29 22:56:18]

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, 2017-03-29 22:59:46]

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

[commit-bot: I haz the power, 2017-03-29 22:59:47]

Dry run: Try jobs failed on following builders:
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
  mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmurph, 2017-03-29 23:40:44]

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

[commit-bot: I haz the power, 2017-03-29 23:41:38]

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, 2017-03-30 00:13:06]

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

[commit-bot: I haz the power, 2017-03-30 00:13:07]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[dmurph, 2017-04-10 19:21:18]

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

[commit-bot: I haz the power, 2017-04-10 19:22:05]

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, 2017-04-10 20:48:03]

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

[commit-bot: I haz the power, 2017-04-10 20:48:04]

Dry run: This issue passed the CQ dry run.

[dmurph, 2017-04-11 00:28:47]

+jam for render_process_host_impl.cc/h
Chris/Josh, can you PTAL?

[dmurph, 2017-04-11 00:29:07]

dmurph@chromium.org changed reviewers:
+ jam@chromium.org

[dmurph, 2017-04-11 00:29:08]

+jam for render_process_host_impl.cc/h

[jam, 2017-04-11 01:13:54]

lgtm

[jsbell, 2017-04-11 16:58:04]

Can we turn more of the comments about task runner affinity into assertions?

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/database_impl.cc:32: // Expect to be created on IO
thread, and called/destroyed on IDB thread.
Can you use DCHECK_CURRENTLY_ON and
DCHECK(idb_runner_->RunsTasksOnCurrentThread()) to enforce this?

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:260:
DCHECK(!database_created_);
This should really refer to 'connection' not 'database'; it ends up being an
IDBDatabase instance on the blink side but it's really a connection. 

(I suppose we call the mojo interface for it 'database', though...)

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:292: // Only create a new
connection if the database was not previously sent in
s/database/one/

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:303: closed_ = true;
Seeing this usage, maybe rename 'complete_' or 'done_' ?

"closed" sounds too much like "connection closed"

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:115: bool closed_ = false;
Add comments about what these mean.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database_callbacks.h (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.h:47: bool closed =
false;
Shouldn't this be: closed_

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:80: 
nit: remove extra blank line

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:82: idb_helper_ =
idb_runner_
When will idb_runner_ be null? Some unit tests?

[dmurph, 2017-04-11 19:32:23]

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

[dmurph, 2017-04-11 19:32:50]

Thanks!

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/database_impl.cc (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/database_impl.cc:32: // Expect to be created on IO
thread, and called/destroyed on IDB thread.
On 2017/04/11 16:58:03, jsbell wrote:
> Can you use DCHECK_CURRENTLY_ON and
> DCHECK(idb_runner_->RunsTasksOnCurrentThread()) to enforce this?

Done.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.cc (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:260:
DCHECK(!database_created_);
On 2017/04/11 16:58:04, jsbell wrote:
> This should really refer to 'connection' not 'database'; it ends up being an
> IDBDatabase instance on the blink side but it's really a connection. 
> 
> (I suppose we call the mojo interface for it 'database', though...)

Done.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:292: // Only create a new
connection if the database was not previously sent in
On 2017/04/11 16:58:03, jsbell wrote:
> s/database/one/

Done.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.cc:303: closed_ = true;
On 2017/04/11 16:58:04, jsbell wrote:
> Seeing this usage, maybe rename 'complete_' or 'done_' ?
> 
> "closed" sounds too much like "connection closed"

Done.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_callbacks.h (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_callbacks.h:115: bool closed_ = false;
On 2017/04/11 16:58:04, jsbell wrote:
> Add comments about what these mean.

Done.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_database_callbacks.h (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_database_callbacks.h:47: bool closed =
false;
On 2017/04/11 16:58:04, jsbell wrote:
> Shouldn't this be: closed_

Done.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
File content/browser/indexed_db/indexed_db_dispatcher_host.cc (right):

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:80: 
On 2017/04/11 16:58:04, jsbell wrote:
> nit: remove extra blank line

Done.

https://codereview.chromium.org/2727733004/diff/220001/content/browser/indexe...
content/browser/indexed_db/indexed_db_dispatcher_host.cc:82: idb_helper_ =
idb_runner_
On 2017/04/11 16:58:04, jsbell wrote:
> When will idb_runner_ be null? Some unit tests?

Yes, unit tests.

[commit-bot: I haz the power, 2017-04-11 19:33:39]

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

[jsbell, 2017-04-11 20:35:48]

lgtm!

[dmurph, 2017-04-11 20:47:21]

The CQ bit was unchecked by dmurph@chromium.org

[dmurph, 2017-04-11 20:47:29]

The CQ bit was checked by dmurph@chromium.org

[dmurph, 2017-04-11 20:47:29]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
reillyg@chromium.org, pwnall@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2727733004/#ps240001
(title: "comments")

[commit-bot: I haz the power, 2017-04-11 20:48:16]

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, 2017-04-11 21:51:12]

CQ is committing da patch.

Bot data: {"patchset_id": 240001, "attempt_start_ts": 1491943649204330,
"parent_rev": "54c4473401a83a2e700d39cf3ee0fee91eb198fc", "commit_rev":
"1f18ca937688b4e0ee570b4d506f5ac21ead5b93"}

[commit-bot: I haz the power, 2017-04-11 21:52:06]

Description was changed from

==========
[IndexedDB] Closing mojo connections when renderer exits.

Changes IndexedDBDispatcherHost to be owned by a unique_ptr instead of
ref counted, and makes the mojo connections for the database and the
cursor interfaces strongly stored (owned) by the dispatcher host.

The dispatcher host now supports weak pointers, which are used by the
IndexedDB.*Callbacks (still refcounted, and probably by necessity) to
ignore responses when the host is gone. The host also observes renderer
exit - which can happen well before the render process host is
destroyed - to destruct the bindings and clear the weak pointers.

Finally, minor cleanup includes minimizing dependencies, adding the
connection error listener on IndexedDBDatabaseCallbacks to cut that off
as early as possible, and formally create the IDBHelper on the
dispatcher host to make the idb thread separation cleaner.

BUG=696055
==========

to

==========
[IndexedDB] Closing mojo connections when renderer exits.

Changes IndexedDBDispatcherHost to be owned by a unique_ptr instead of
ref counted, and makes the mojo connections for the database and the
cursor interfaces strongly stored (owned) by the dispatcher host.

The dispatcher host now supports weak pointers, which are used by the
IndexedDB.*Callbacks (still refcounted, and probably by necessity) to
ignore responses when the host is gone. The host also observes renderer
exit - which can happen well before the render process host is
destroyed - to destruct the bindings and clear the weak pointers.

Finally, minor cleanup includes minimizing dependencies, adding the
connection error listener on IndexedDBDatabaseCallbacks to cut that off
as early as possible, and formally create the IDBHelper on the
dispatcher host to make the idb thread separation cleaner.

BUG=696055

Review-Url: https://codereview.chromium.org/2727733004
Cr-Commit-Position: refs/heads/master@{#463786}
Committed:
https://chromium.googlesource.com/chromium/src/+/1f18ca937688b4e0ee570b4d506f...
==========

[commit-bot: I haz the power, 2017-04-11 21:52:08]

Committed patchset #13 (id:240001) as
https://chromium.googlesource.com/chromium/src/+/1f18ca937688b4e0ee570b4d506f...