mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c112794307cc4858616f5aeaa05dbfe0d89b671e
Cr-Commit-Position: refs/heads/master@{#382343}

[Elliot Glaysher, 2016-02-25 23:16:11]

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

[Elliot Glaysher, 2016-02-25 23:16:12]

jam: question in an unfinished patch

https://codereview.chromium.org/1737933002/diff/1/content/browser/mojo/mojo_s...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1737933002/diff/1/content/browser/mojo/mojo_s...
content/browser/mojo/mojo_shell_context.cc:259:
base::FilePath::FromUTF8Unsafe("/work/chrome/tmp_profile");
So my current big question is how do I safely get the path to the current user
data dir from here?

MojoShellContext is initialized during BrowserMainLoop::BrowserThreadsStarted();
I don't believe we yet have a profile. Now I could try to recreate the logic
that we do in chrome, but that's actually really complicated and I was wondering
if there was any shortcut here. chrome's GetDefaultUserDataDirectory() is
actually has platform specific implementations, and also references "chrome"
outright, which I assume we don't want at this layer.

(Really, we should have a ProfileService here which encapsulates access to the
filesystem here, but Ben's user identity changes haven't landed yet, so this is
delayed on that.)

[jam, 2016-02-26 19:06:58]

Description was changed from

==========
mojo leveldb: Get filesystem and leveldb connected to DOMStorageContext.

[xxx: not commitable as is; we'll need to deal with the connection
identity issue (by making a profile service) and the filesystem root
directory detection before this can safely land.]

BUG=
==========

to

==========
mojo leveldb: Get filesystem and leveldb connected to DOMStorageContext.

[xxx: not commitable as is; we'll need to deal with the connection
identity issue (by making a profile service) and the filesystem root
directory detection before this can safely land.]

BUG=586194
==========

[michaeln, 2016-02-26 21:59:23]

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

[michaeln, 2016-02-26 21:59:25]

mostly questions

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
File components/filesystem/file_system_app.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
components/filesystem/file_system_app.cc:20: lock_table_(new LockTable),
This only works if there's only one filesystem vending stuff within a given
directory or something else is ensuring there's no collisions between what
multiple instance are vending/locking within the same dir. Is that right?

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
File components/filesystem/file_system_app.h (right):

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
components/filesystem/file_system_app.h:29: FileSystemApp(const base::FilePath&
persistent_dir);
root_directory or plain old directory are less loaded terms

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
File components/filesystem/file_system_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
components/filesystem/file_system_impl.cc:27: :
remote_application_url_(connection->GetRemoteApplicationURL()),
what is this?

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
components/filesystem/file_system_impl.cc:44: std::move(directory), path,
std::move(temp_dir), lock_table_);
is the directory deleted in the event of a program crash? 

does anything use this TempDir feature yet, is there anything in particular
planning to use it yet?

https://codereview.chromium.org/1737933002/diff/20001/components/leveldb/leve...
File components/leveldb/leveldb_app.h (right):

https://codereview.chromium.org/1737933002/diff/20001/components/leveldb/leve...
components/leveldb/leveldb_app.h:16: scoped_ptr<mojo::ShellClient>
CreateLevelDBApp();
The terminology is going to take some getting used to. Everytihng is an "app".
Ok, if you say so :)

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:182:
GURL("mojo:filesystem"), GURL(content::kBrowserMojoAppUrl));
is there a way to provide the root_directory path to the factory?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:212: void
DOMStorageContextWrapper::OnDirectoryOpened(filesystem::FileError err) {
what about err checking

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:215:
leveldb_connection_ = content::MojoAppConnection::Create(
This ends up invoking the CreateLevelDbApp() function is that right and all this
app is is a factory for leveldb_s

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:224: void
DOMStorageContextWrapper::OnDatabaseOpened(leveldb::DatabaseError status) {
what about status checking?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_wrapper.h (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.h:83: void
LevelDBWrapperImplHasNoBindings(const std::string& origin);
+1 the OnFooHappened naming, i'd vote to rename this to match and to put it at
the end of the method list, at least after all the open related things since
closing happens after opening. OnAllBindingsClosed()?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.h:111:
filesystem::DirectoryPtr directory_;
Do we need to retain all of these things for the life of the object? How are
these related and what implications does deleting one that spawned another have?

I guess these FooPtr types are 'smart' scopers of some kind?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.h:115:
leveldb::LevelDBDatabasePtr database_;
Do mojo things have thread affinity with regard to destruction? I don't think
this class makes any promises about which thread its dtor'd on.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
File content/browser/level_db_wrapper_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
content/browser/level_db_wrapper_impl.cc:29: no_bindings_callback_.Run();
this call doesn't look right, looks like the callback can get called twice (and
get called during cntxwppr destrcution), maybe that's why you hit the dcheck?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:261:
base::Bind(&filesystem::CreateFileSystemApp, path);
Is there one MojoShellContext and application_manager_ per-process or is this a
per-profile kind of object?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:269:
application_manager_->SetLoaderForURL(
voodoo :)

[michaeln, 2016-02-27 00:59:03]

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
File content/browser/level_db_wrapper_impl.h (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
content/browser/level_db_wrapper_impl.h:23: class LevelDBWrapperImpl : public
LevelDBWrapper {
Hey, why isn't this class part in component/leveldb?

[Elliot Glaysher, 2016-03-02 23:01:06]

Description was changed from

==========
mojo leveldb: Get filesystem and leveldb connected to DOMStorageContext.

[xxx: not commitable as is; we'll need to deal with the connection
identity issue (by making a profile service) and the filesystem root
directory detection before this can safely land.]

BUG=586194
==========

to

==========
mojo leveldb: Get filesystem and leveldb connected to DOMStorageContext.

[xxx: *still* not committable as is; while we now deal with the conncetion
identity issue with the ProfileApp, there's a weird shutdown bug where we
are leaving LevelDBWrapperImpl instances alive.]

BUG=586194
==========

[Elliot Glaysher, 2016-03-02 23:06:20]

OK, so this is close now. There's just the shutdown crash to work on. Michael,
you said that DOMStorageContextWrapper didn't have thread affinity. But we
create LevelDBWrapperImpls which do have thread affinity here. Do you have any
ideas about bridging this?

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
File components/filesystem/file_system_app.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
components/filesystem/file_system_app.cc:20: lock_table_(new LockTable),
On 2016/02/26 21:59:24, michaeln wrote:
> This only works if there's only one filesystem vending stuff within a given
> directory or something else is ensuring there's no collisions between what
> multiple instance are vending/locking within the same dir. Is that right?

Two points:

- The filesystem app was meant to be a singleton to all file access in mojo
code. It was replaced with the code I landed yesterday. (And in the
ProfileService which replaces this, it is a singleton for each profile
directory.)

- The lock table is a backup. We still perform OS file locking, however, on
posix, locking is advisory and we want to have the same Windows-y semantics
everywhere.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:182:
GURL("mojo:filesystem"), GURL(content::kBrowserMojoAppUrl));
On 2016/02/26 21:59:25, michaeln wrote:
> is there a way to provide the root_directory path to the factory?

This was the big reason I went off and did the ProfileApp/Service
refactor--there wasn't a way to perform this sort of binding.

The new patchset performs these associations by binding a BrowserContext to a
mojo user_id, and then having the ProfileApplication aware of user_ids.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:215:
leveldb_connection_ = content::MojoAppConnection::Create(
On 2016/02/26 21:59:24, michaeln wrote:
> This ends up invoking the CreateLevelDbApp() function is that right and all
this
> app is is a factory for leveldb_s

I believe so. We connect to leveldb here, and then pass this leveldb instance to
each LevelDBWrapperImpl.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_wrapper.h (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.h:111:
filesystem::DirectoryPtr directory_;
On 2016/02/26 21:59:25, michaeln wrote:
> Do we need to retain all of these things for the life of the object? How are
> these related and what implications does deleting one that spawned another
have?

So we want to keep |file_system_| (now profile_system_) and |directory_| alive
while we're asking for a directory, and then |directory_| gets nulled when we
pass ownership to leveldb. I just added code to tear these down once we have
|database_|.

We do need to keep the |leveldb_connection_| alive for the lifetime of this
object though.

> I guess these FooPtr types are 'smart' scopers of some kind?

Yes.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.h:115:
leveldb::LevelDBDatabasePtr database_;
On 2016/02/26 21:59:25, michaeln wrote:
> Do mojo things have thread affinity with regard to destruction? I don't think
> this class makes any promises about which thread its dtor'd on.

They have strong affinity for the thread they were created on.

(Could this explain the current crashing on trunk? crbug.com/590345. That stack
trace mutates quite a bit with this patch so I might be introducing a different
shutdown bug here, but...)

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
File content/browser/level_db_wrapper_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
content/browser/level_db_wrapper_impl.cc:29: no_bindings_callback_.Run();
On 2016/02/26 21:59:25, michaeln wrote:
> this call doesn't look right, looks like the callback can get called twice
(and
> get called during cntxwppr destrcution), maybe that's why you hit the dcheck?

Could you elaborate on this?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:261:
base::Bind(&filesystem::CreateFileSystemApp, path);
On 2016/02/26 21:59:25, michaeln wrote:
> Is there one MojoShellContext and application_manager_ per-process or is this
a
> per-profile kind of object?

There's a single MojoShellContext per browser process currently. This is
declaring a thing that creates a new application instance when (in the future)
something tries to connect to the url passed in.

https://codereview.chromium.org/1737933002/diff/100001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/100001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:255: LOG(ERROR) <<
"Writing some crud to dom storage: " << status;
[xxx: Obviously not commitable as is, this is just to show that we can write to
a profile directory scoped database. (~/.config/chromium/Default/dom_storage/
has some leveldb files in it now!)]

[Elliot Glaysher, 2016-03-02 23:06:20]

OK, so this is close now. There's just the shutdown crash to work on. Michael,
you said that DOMStorageContextWrapper didn't have thread affinity. But we
create LevelDBWrapperImpls which do have thread affinity here. Do you have any
ideas about bridging this?

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
File components/filesystem/file_system_app.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/components/filesystem/f...
components/filesystem/file_system_app.cc:20: lock_table_(new LockTable),
On 2016/02/26 21:59:24, michaeln wrote:
> This only works if there's only one filesystem vending stuff within a given
> directory or something else is ensuring there's no collisions between what
> multiple instance are vending/locking within the same dir. Is that right?

Two points:

- The filesystem app was meant to be a singleton to all file access in mojo
code. It was replaced with the code I landed yesterday. (And in the
ProfileService which replaces this, it is a singleton for each profile
directory.)

- The lock table is a backup. We still perform OS file locking, however, on
posix, locking is advisory and we want to have the same Windows-y semantics
everywhere.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:182:
GURL("mojo:filesystem"), GURL(content::kBrowserMojoAppUrl));
On 2016/02/26 21:59:25, michaeln wrote:
> is there a way to provide the root_directory path to the factory?

This was the big reason I went off and did the ProfileApp/Service
refactor--there wasn't a way to perform this sort of binding.

The new patchset performs these associations by binding a BrowserContext to a
mojo user_id, and then having the ProfileApplication aware of user_ids.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.cc:215:
leveldb_connection_ = content::MojoAppConnection::Create(
On 2016/02/26 21:59:24, michaeln wrote:
> This ends up invoking the CreateLevelDbApp() function is that right and all
this
> app is is a factory for leveldb_s

I believe so. We connect to leveldb here, and then pass this leveldb instance to
each LevelDBWrapperImpl.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
File content/browser/dom_storage/dom_storage_context_wrapper.h (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.h:111:
filesystem::DirectoryPtr directory_;
On 2016/02/26 21:59:25, michaeln wrote:
> Do we need to retain all of these things for the life of the object? How are
> these related and what implications does deleting one that spawned another
have?

So we want to keep |file_system_| (now profile_system_) and |directory_| alive
while we're asking for a directory, and then |directory_| gets nulled when we
pass ownership to leveldb. I just added code to tear these down once we have
|database_|.

We do need to keep the |leveldb_connection_| alive for the lifetime of this
object though.

> I guess these FooPtr types are 'smart' scopers of some kind?

Yes.

https://codereview.chromium.org/1737933002/diff/20001/content/browser/dom_sto...
content/browser/dom_storage/dom_storage_context_wrapper.h:115:
leveldb::LevelDBDatabasePtr database_;
On 2016/02/26 21:59:25, michaeln wrote:
> Do mojo things have thread affinity with regard to destruction? I don't think
> this class makes any promises about which thread its dtor'd on.

They have strong affinity for the thread they were created on.

(Could this explain the current crashing on trunk? crbug.com/590345. That stack
trace mutates quite a bit with this patch so I might be introducing a different
shutdown bug here, but...)

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
File content/browser/level_db_wrapper_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/level_d...
content/browser/level_db_wrapper_impl.cc:29: no_bindings_callback_.Run();
On 2016/02/26 21:59:25, michaeln wrote:
> this call doesn't look right, looks like the callback can get called twice
(and
> get called during cntxwppr destrcution), maybe that's why you hit the dcheck?

Could you elaborate on this?

https://codereview.chromium.org/1737933002/diff/20001/content/browser/mojo/mo...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1737933002/diff/20001/content/browser/mojo/mo...
content/browser/mojo/mojo_shell_context.cc:261:
base::Bind(&filesystem::CreateFileSystemApp, path);
On 2016/02/26 21:59:25, michaeln wrote:
> Is there one MojoShellContext and application_manager_ per-process or is this
a
> per-profile kind of object?

There's a single MojoShellContext per browser process currently. This is
declaring a thing that creates a new application instance when (in the future)
something tries to connect to the url passed in.

https://codereview.chromium.org/1737933002/diff/100001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/100001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:255: LOG(ERROR) <<
"Writing some crud to dom storage: " << status;
[xxx: Obviously not commitable as is, this is just to show that we can write to
a profile directory scoped database. (~/.config/chromium/Default/dom_storage/
has some leveldb files in it now!)]

[jam, 2016-03-03 16:32:11]

https://codereview.chromium.org/1737933002/diff/120001/components/profile_ser...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1737933002/diff/120001/components/profile_ser...
components/profile_service/profile_app.h:28: // threads; this class will have to
be made into something like core services.
nit: the "core services" comment is gonna be indecipherable to almost anyone
reading this :)

https://codereview.chromium.org/1737933002/diff/120001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/120001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:96: LOG(ERROR) <<
"Size of level_db_wrappers_: " << level_db_wrappers_.size();
nit: change from LOG(ERROR) to something else that doesn't show up for everyone,
or only LOG(ERROR) if the container isn't empty?

https://codereview.chromium.org/1737933002/diff/120001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:184:
profile_connection_ = content::MojoAppConnection::Create(
nit: here and in other places in the file, no need for for "content::"

https://codereview.chromium.org/1737933002/diff/120001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:210: // The DCHECK
here broke on chrome-extensions.
can you explain this some more? I don't understand how we would get this
callback and the origin isn't in the map

https://codereview.chromium.org/1737933002/diff/120001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:226:
leveldb_->Open(std::move(directory_), "dom_storage", GetProxy(&database_),
s/dom_storage/local_storage


"dom_storage" in the code refers to both local storage and session storage, and
we'll want to store the latter separately. for now, this work is only addressing
local storage

https://codereview.chromium.org/1737933002/diff/120001/content/browser/mojo/m...
File content/browser/mojo/mojo_app_connection_impl.h (right):

https://codereview.chromium.org/1737933002/diff/120001/content/browser/mojo/m...
content/browser/mojo/mojo_app_connection_impl.h:17:
MojoAppConnectionImpl(BrowserContext* context,
document

https://codereview.chromium.org/1737933002/diff/120001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.h (right):

https://codereview.chromium.org/1737933002/diff/120001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.h:47: static void
ConnectToApplicationWithContext(
add comment

https://codereview.chromium.org/1737933002/diff/120001/content/browser/storag...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/120001/content/browser/storag...
content/browser/storage_partition_impl.cc:447:
context->GetSpecialStoragePolicy());
no need to pass in context and a parameter that could be derived from it
(specialstoragepolicy)

https://codereview.chromium.org/1737933002/diff/120001/content/public/browser...
File content/public/browser/mojo_app_connection.h (right):

https://codereview.chromium.org/1737933002/diff/120001/content/public/browser...
content/public/browser/mojo_app_connection.h:36: BrowserContext* context,
the new parameter should be documented

https://codereview.chromium.org/1737933002/diff/120001/content/public/test/co...
File content/public/test/content_browser_test.h (right):

https://codereview.chromium.org/1737933002/diff/120001/content/public/test/co...
content/public/test/content_browser_test.h:33: BrowserContext*
GetBrowserContext() const;
no need to add, tests do
shell()->web_contents()->GetBrowserContext()

[Elliot Glaysher, 2016-03-04 18:40:19]

Description was changed from

==========
mojo leveldb: Get filesystem and leveldb connected to DOMStorageContext.

[xxx: *still* not committable as is; while we now deal with the conncetion
identity issue with the ProfileApp, there's a weird shutdown bug where we
are leaving LevelDBWrapperImpl instances alive.]

BUG=586194
==========

to

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService form the profile
application to the dedicated leveldb application to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
==========

[Elliot Glaysher, 2016-03-04 18:42:42]

erg@chromium.org changed reviewers:
+ ben@chromium.org

[Elliot Glaysher, 2016-03-04 18:42:43]

This is now ready for actual review. We now shut start up and shut down cleanly.
I believe we're managing the lifetimes of the applications correctly.

Since jam@ is now out for a week, subbing ben@.

https://codereview.chromium.org/1737933002/diff/120001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/120001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:210: // The DCHECK
here broke on chrome-extensions.
On 2016/03/03 16:32:10, jam wrote:
> can you explain this some more? I don't understand how we would get this
> callback and the origin isn't in the map

Fixing shutdown elsewhere fixed this. Changed back.

[Ben Goodger (Google), 2016-03-04 20:50:49]

https://codereview.chromium.org/1737933002/diff/180001/components/leveldb/lev...
File components/leveldb/leveldb_app.h (right):

https://codereview.chromium.org/1737933002/diff/180001/components/leveldb/lev...
components/leveldb/leveldb_app.h:18: class LevelDBApp : public
mojo::ShellClient,
Can you explain why the LevelDB functionality is a discrete application with its
own identity (as far as the shell is concerned), as differentiated from your
comment in the profile service file that talks about different interfaces (e.g.
preferences) vended by the profile application?

reasons to have a separate application:
- this code has a different capability requirements than other code exposed by
profile service
- this code wants to be run in a different sandbox
- this code runs as a specific user where the profile service runs as all users
(I don't think this is the case?)
- you expect (at some point in the future) this code to be packaged separately
from the profile service.
- maybe others?

I can see two views of the profile service. One is that it is a very simple
broker of access to directories and offers no other interfaces. Everything else
is implemented as separate applications layered on top, ultimately with the mind
that they're even packaged separately, run in different processes.
Another is that it's an aggregator of all user-related state, brokering
directory access & providing access to other things like leveldb, profile, etc.
Which do you have in mind?

https://codereview.chromium.org/1737933002/diff/180001/components/leveldb/lev...
File components/leveldb/leveldb_service_impl.h (right):

https://codereview.chromium.org/1737933002/diff/180001/components/leveldb/lev...
components/leveldb/leveldb_service_impl.h:11: #include
"mojo/public/cpp/bindings/strong_binding.h"
???

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
components/profile_service/profile_app.cc:41:
base::MessageLoop::current()->QuitNow();
you can also use mojo/shell/public/cpp/message_loop_ref.h for this.

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
components/profile_service/profile_app.h:30: // sub-applications.
what's the reason for needing to provide interfaces on different threads?

https://codereview.chromium.org/1737933002/diff/180001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1737933002/diff/180001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:289:
mojo::shell::mojom::Connector::kUserRoot,
this should be kUserInherit. Then the shell will figure out the right thing to
do.

https://codereview.chromium.org/1737933002/diff/180001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:296: BrowserContext* context,
rather than introduce knowledge of BrowserContext here, just make this method
take a uint32_t and push the burden of looking up the uid to the caller.

https://codereview.chromium.org/1737933002/diff/180001/content/public/browser...
File content/public/browser/mojo_app_connection.h (right):

https://codereview.chromium.org/1737933002/diff/180001/content/public/browser...
content/public/browser/mojo_app_connection.h:37: BrowserContext* context,
same here... just take a uint32_t

[michaeln, 2016-03-04 21:26:00]

hi elliot, i'm taking at look this afternoon, please give me some time to put
together some feedback

>
https://codereview.chromium.org/1737933002/diff/180001/content/browser/mojo/m...
> content/browser/mojo/mojo_shell_context.cc:296: BrowserContext* context,
> rather than introduce knowledge of BrowserContext here, just make this method
> take a uint32_t and push the burden of looking up the uid to the caller.

i have a similar comment in the dom_storage code

[Ben Goodger (Google), 2016-03-04 22:13:17]

https://codereview.chromium.org/1737933002/diff/180001/components/leveldb/lev...
File components/leveldb/leveldb_app.h (right):

https://codereview.chromium.org/1737933002/diff/180001/components/leveldb/lev...
components/leveldb/leveldb_app.h:18: class LevelDBApp : public
mojo::ShellClient,
For the curious, Elliot & I discussed this offline.

In this CL the application concept is being used to get thread isolation for
leveldb vs. filesystem impls, which interact poorly when they live on the same
thread.

My suggestion is instead of using an application for LevelDB, make the profile
application vend a LevelDB interface via its service provider, and when
requested bind that interface request on a different thread:

profile_app.cc:

bool ProfileApp::AcceptConnection(Connection* conn) {
  ..
  conn->AddInterface<LevelDB>(this);
}

..

void ProfileApp::Create(Connection* conn, LevelDBRequest req) {
  new LevelDBThread(std::move(req));
}

class LevelDBThread : public base::SimpleThread {
..
};

[michaeln, 2016-03-05 01:38:54]

https://codereview.chromium.org/1737933002/diff/160001/components/leveldb/lev...
File components/leveldb/leveldb_service_impl.h (right):

https://codereview.chromium.org/1737933002/diff/160001/components/leveldb/lev...
components/leveldb/leveldb_service_impl.h:23: const mojo::String& dbname,
We might want this to accept relative filepaths to open databases in nested
directories. Either that or we'll need to produce sub_directories from the
profile_dir at a higher level.

https://codereview.chromium.org/1737933002/diff/160001/components/profile_ser...
File components/profile_service/profile_service_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/160001/components/profile_ser...
components/profile_service/profile_service_impl.cc:37: const
GetDirectoryCallback& callback) {
Would it make sense to have a variant that accepts a FilePath? If a full
filepath is provided, it must a child of this->path_. If a relative filepath is
provided, it means path_.AppendRelativePath.

I suppose callers can call profile_directory_.OpenDirectory() but notice that in
the local storage case, current code doesn't actually know the relative path
from {profiledir} to leveldb file, however we do know the fullpath.

I'm noticing that To open a subdirectory thats nested down a couple levels
requires mulitple async calls to dir.OpenDirectory(), one for each nesting level
down.

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:185:
profile_service_->GetDirectory(
What do we do about incognito profiles? How is that represented by the
profile_service?

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:213: NOTREACHED();
What makes this not reachable? If the native file system gives us errors, will
we not reach this?

We'll probably want to mimic behavior of the current system in the event of
errors that prevent reading/writing to disk. I'll take a look at what that that
behavior is and let you know.

If the old system is unable to read/write to its underlying database, it still
functions and will retain values for the duration of the browsing session, it
might drop things if all tabs for an origin are closed. The idea is the show
must go on despite database corruption or a readonly filesystem.

edit: Just looked at where the callback is invoked. If the GetDirectory() method
always returns OK, we should remove the |err| parameter.

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:219:
leveldb_connection_ = MojoAppConnection::Create(
naming nit: I understand this was chosen from a mojo'ish point of view, but from
a db'ish point of view, |connection| is an odd choice. It's definitely not a
connection to a database. What's current called database_ is much closer to a
connection in the db'ish sense.

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:223:
leveldb_->Open(std::move(directory_), "local_storage", GetProxy(&database_),
I think directory_ corresponds to a particular profile dir, is that right?
Something like...

C:\Users\fred\AppData\Local\Google\Chrome\User Data\Default
  or
C:\Users\fred\AppData\Local\Google\Chrome\User Data\Profile 2

We need to be more specific than that, there's a separate Storage directory for
each StorageParition, a Profile may contain multiple partitions.

The |data_path| given the the constructor is the full path to the storage
partitions directory, unless it's an incognito profile in which case its an
empty string.

For the main storage partition |data_path| == profile dir. For sub-partitions
created by the apps/extensions system it looks like...

{profiledir}/Storage/ext/<extensionid>/def
or
{profiledir}/Storage/ext/<extensionid>/D77D9D3CCB10

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:225:
weak_ptr_factory_.GetWeakPtr()));
We currently keep individual local storage sqlite databases in
"{paritiondir}/Local Storage". I'd suggest we keep this new database here...

{partitiondir}/Local Storage/leveldb/

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:230: NOTREACHED();
This is certainly reachable, per the show must go on rule, we'll have to
continue with a null database_ ptr.

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
File components/profile_service/profile_service_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
components/profile_service/profile_service_impl.cc:22: base::FilePath
base_profile_dir,
const ref for the FilePath&

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
components/profile_service/profile_service_impl.cc:41:
callback.Run(filesystem::FileError::OK);
If this method can never fail, we don't need a error code and what is the
purpose of the completion callback?

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
File components/profile_service/public/interfaces/profile.mojom (right):

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
components/profile_service/public/interfaces/profile.mojom:14: // it has access
to is the User's profile.
How is incognito represented?

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:189:
connection_state_ = CONNECITON_IN_PROGRESS;
Stupid mojo question?

The GetDirectory callback always return OK so it's not obvious why your waiting
for the callback to continue. Is the problem that we can't pass directory_ to
the leveldb_->Open() method until voodoo has transpired?

Would invoking OnDirectoryOpened(OK) right here work (along with ignoring
GetDirectory's completeion callback)?

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.h (right):

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.h:103: uint32_t
mojo_user_id_;
I'd prefer to factor the new mojo based impl out into a separate class (an inner
class would be fine or a class declared in an anon namespace in the .cc) rather
than co-mingle old/new code in the same class. This class can own an instance of
the new object and when Shutdown() is called, delete it. Shutdown() is called on
the ui thread so that would help with our thread affinity.

Eventually the new code will subsume the old code, but we have a ways to go
before can do that.

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.h:118:
leveldb::LevelDBDatabasePtr database_;
Since we only need leveldb_connection_ and database_ for the lifetime of the
object and the others are only transiently needed during the connection
sequence, it might be nice to not store the transients as direct datamembers.

[Elliot Glaysher, 2016-03-10 21:00:35]

PTAL

ben: I've made the profile application serve the leveldb service, too.

michaeln: I've fixed paths and error handling (so we continue even if we don't
have a leveldb object.)

https://codereview.chromium.org/1737933002/diff/160001/components/profile_ser...
File components/profile_service/profile_service_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/160001/components/profile_ser...
components/profile_service/profile_service_impl.cc:37: const
GetDirectoryCallback& callback) {
On 2016/03/05 01:38:54, michaeln wrote:
> Would it make sense to have a variant that accepts a FilePath? If a full
> filepath is provided, it must a child of this->path_. If a relative filepath
is
> provided, it means path_.AppendRelativePath.

Yes. Previously that's how this worked, but in a previous patch, I pulled it out
due to reviewer comments.

(Given your other comments about which directory under the profile dir we need
to write to, I went and added it back.)

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:185:
profile_service_->GetDirectory(
On 2016/03/05 01:38:54, michaeln wrote:
> What do we do about incognito profiles? How is that represented by the
> profile_service?

I was under the impression that incognito stuff never went to disk; that
incognito local storage was stuffed into an in-memory std::map<>.

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:213: NOTREACHED();
On 2016/03/05 01:38:54, michaeln wrote:
> What makes this not reachable? If the native file system gives us errors, will
> we not reach this?
> 
> We'll probably want to mimic behavior of the current system in the event of
> errors that prevent reading/writing to disk. I'll take a look at what that
that
> behavior is and let you know.
> 
> If the old system is unable to read/write to its underlying database, it still
> functions and will retain values for the duration of the browsing session, it
> might drop things if all tabs for an origin are closed. The idea is the show
> must go on despite database corruption or a readonly filesystem.
> 
> edit: Just looked at where the callback is invoked. If the GetDirectory()
method
> always returns OK, we should remove the |err| parameter.

Changed all of this so that it should work even when we've been given an error
from subdirectory creation. (work == pass a null leveldb object to
LevelDBWrapperImpl.)

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:219:
leveldb_connection_ = MojoAppConnection::Create(
On 2016/03/05 01:38:54, michaeln wrote:
> naming nit: I understand this was chosen from a mojo'ish point of view, but
from
> a db'ish point of view, |connection| is an odd choice. It's definitely not a
> connection to a database. What's current called database_ is much closer to a
> connection in the db'ish sense.

To match the mojo convention, I renamed |leveldb_| to |leveldb_service_|.

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:223:
leveldb_->Open(std::move(directory_), "local_storage", GetProxy(&database_),
On 2016/03/05 01:38:54, michaeln wrote:
> I think directory_ corresponds to a particular profile dir, is that right?
> Something like...
> 
> C:\Users\fred\AppData\Local\Google\Chrome\User Data\Default
>   or
> C:\Users\fred\AppData\Local\Google\Chrome\User Data\Profile 2
> 
> We need to be more specific than that, there's a separate Storage directory
for
> each StorageParition, a Profile may contain multiple partitions.
> 
> The |data_path| given the the constructor is the full path to the storage
> partitions directory, unless it's an incognito profile in which case its an
> empty string.
> 
> For the main storage partition |data_path| == profile dir. For sub-partitions
> created by the apps/extensions system it looks like...
> 
> {profiledir}/Storage/ext/<extensionid>/def
> or
> {profiledir}/Storage/ext/<extensionid>/D77D9D3CCB10

Done, but not like this. |directory_| now points to the per partition subdir,
and then under that goes the leveldb dir.

(I believe we talked about this in person.)

https://codereview.chromium.org/1737933002/diff/160001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:225:
weak_ptr_factory_.GetWeakPtr()));
On 2016/03/05 01:38:54, michaeln wrote:
> We currently keep individual local storage sqlite databases in
> "{paritiondir}/Local Storage". I'd suggest we keep this new database here...
> 
> {partitiondir}/Local Storage/leveldb/

Done.

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
components/profile_service/profile_app.cc:41:
base::MessageLoop::current()->QuitNow();
On 2016/03/04 20:50:49, Ben Goodger (Google) wrote:
> you can also use mojo/shell/public/cpp/message_loop_ref.h for this.

Done. (Here and in leveldb.)

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1737933002/diff/180001/components/profile_ser...
components/profile_service/profile_app.h:30: // sub-applications.
On 2016/03/04 20:50:49, Ben Goodger (Google) wrote:
> what's the reason for needing to provide interfaces on different threads?

[Discussed in person; leaving the comment here for everyone else.]

They block on each other.

leveldb has a synchronous low level interface for filesystem access. The leveldb
service implements that for mojo filesystem access. This means that for things
like opening a file, we send a mojo message and then WaitForIncomingResponse().
(After this, we have a file descriptor and don't have to do any waiting so
reading and writing don't block on mojo; just on the os level primitives.)

ProfileService is an alternative service that has most of the guts of what was
mojo:filesystem, but is aware of user profiles. If we serve the leveldb service
and the profile service from the same thread, we then get into a situation where
leveldb will post a task to profile on the current thread and then will block on
the current thread waiting for a result.

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
File components/profile_service/profile_service_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
components/profile_service/profile_service_impl.cc:41:
callback.Run(filesystem::FileError::OK);
On 2016/03/05 01:38:54, michaeln wrote:
> If this method can never fail, we don't need a error code and what is the
> purpose of the completion callback?

Problem: This method can never fail. Resolution: replaced it with a method which
can fail. :p

(aka, the create a subdirectory variant mentioned earlier.)

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
File components/profile_service/public/interfaces/profile.mojom (right):

https://codereview.chromium.org/1737933002/diff/200001/components/profile_ser...
components/profile_service/public/interfaces/profile.mojom:14: // it has access
to is the User's profile.
On 2016/03/05 01:38:54, michaeln wrote:
> How is incognito represented?

Right now, an incognito window is a different mojo user to make sure that we
don't reuse contexts between the main and OTR profiles. That might be the wrong
decision, and maybe there should be a
content::BrowserContext::InitializeChildProfile() to match c::B::Initialize().

(I'm not sure what you're asking.)

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:189:
connection_state_ = CONNECITON_IN_PROGRESS;
On 2016/03/05 01:38:54, michaeln wrote:
> Stupid mojo question?
> 
> The GetDirectory callback always return OK so it's not obvious why your
waiting
> for the callback to continue. Is the problem that we can't pass directory_ to
> the leveldb_->Open() method until voodoo has transpired?

I believe so. I tried that first, but had issues so I went with an asynchronous
callback to signal that |directory_| was ready to be used.

(That said, this point is now moot since GetSubDirectory can fail if it fails to
create the subdirectory.)

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.h (right):

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.h:103: uint32_t
mojo_user_id_;
On 2016/03/05 01:38:54, michaeln wrote:
> I'd prefer to factor the new mojo based impl out into a separate class (an
inner
> class would be fine or a class declared in an anon namespace in the .cc)
rather
> than co-mingle old/new code in the same class. This class can own an instance
of
> the new object and when Shutdown() is called, delete it. Shutdown() is called
on
> the ui thread so that would help with our thread affinity.
> 
> Eventually the new code will subsume the old code, but we have a ways to go
> before can do that.

Moved all implementation details into their own inner class.

https://codereview.chromium.org/1737933002/diff/200001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.h:118:
leveldb::LevelDBDatabasePtr database_;
On 2016/03/05 01:38:54, michaeln wrote:
> Since we only need leveldb_connection_ and database_ for the lifetime of the
> object and the others are only transiently needed during the connection
> sequence, it might be nice to not store the transients as direct datamembers.

I previously tried to do this, and had problems. Tried again now and it looks
like I conflated this with the other threading problems. Done.

[Elliot Glaysher, 2016-03-10 21:34:45]

Description was changed from

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService form the profile
application to the dedicated leveldb application to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
==========

to

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
==========

[Ben Goodger (Google), 2016-03-11 23:21:38]

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/leveldb_thread.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:22:
base::Bind(&mojo::common::MessagePumpMojo::Create);
I don't believe you have to do this anymore.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:34: leveldb_bindings_.reset();
rather than use a scoped_ptr<mojo::BindingSet<>>, you could use mojo::BindingSet
& call CloseAllBindings() here.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:47:
mojo::InterfaceRequest<leveldb::LevelDBService> request) {
nit: here, and elsewhere, the new hotness is to use the generated:

leveldb::LevelDBServiceRequest

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/leveldb_thread.h (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.h:52: scoped_ptr<mojo::MessageLoopRef>
parent_thread_message_loop_ref_;
profile_service_message_loop_ref_?

// Hold a reference to the profile service's message loop to keep it alive as
long as there are LevelDB service bindings. Set on the parent thread...

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.h:58: mojo::MessageLoopRefFactory
thread_message_loop_ref_;
Shorter:

// The lifetime of the leveldb thread's message loop is tied to the lifetime of
the level db service it vends.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/profile_app.cc:66: mojo::Connection* connection,
up to prev line

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/profile_app.cc:67: leveldb::LevelDBServiceRequest
request) {
indent over

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/profile_app.h:31: // need, such as preferences, but
these will need to be served off of separate
Why make the generic statement that all interfaces will have to be served off of
different threads? I don't think that's true. (Unless you can tell me why it
is.)

https://codereview.chromium.org/1737933002/diff/420001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.h (right):

https://codereview.chromium.org/1737933002/diff/420001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.h:48: static void
ConnectToApplicationWithUserId(
I think so few people use this class now that you can just add the user_id
parameter to the canonical ConnectToApplication() method.

https://codereview.chromium.org/1737933002/diff/420001/mojo/mojo_shell.gyp
File mojo/mojo_shell.gyp (right):

https://codereview.chromium.org/1737933002/diff/420001/mojo/mojo_shell.gyp#ne...
mojo/mojo_shell.gyp:43: '<(DEPTH)/net/net.gyp:net',
who and the what now?

[Elliot Glaysher, 2016-03-12 00:21:01]

Description was changed from

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
==========

to

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[Elliot Glaysher, 2016-03-14 20:33:20]

When making MojoShellContext::ConnectToApplication() take a user id, I had to
make some modifications to FrameMojoShell so that it got the user id of the
current BrowserContext. (This fixes some cast_shell_linux failures above.)

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/leveldb_thread.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:22:
base::Bind(&mojo::common::MessagePumpMojo::Create);
On 2016/03/11 23:21:37, Ben Goodger (Google) wrote:
> I don't believe you have to do this anymore.

If I don't do this, I get:

   Check failed: GetThreadWasQuitProperly().

on shutdown. Also, we're still manually calling this in mojo:font_service,
mojo:leveldb, etc.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:34: leveldb_bindings_.reset();
On 2016/03/11 23:21:37, Ben Goodger (Google) wrote:
> rather than use a scoped_ptr<mojo::BindingSet<>>, you could use
mojo::BindingSet
> & call CloseAllBindings() here.

I can not do that without getting CalledOnValidThread() errors.

(The problem is that even empty mojo::Callbacks have thread affinity to the
thread that created them. And BindingSet creates a non empty closure to call
this method on connection errors, and sets that up from the launched thread.)

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/profile_app.h:31: // need, such as preferences, but
these will need to be served off of separate
On 2016/03/11 23:21:38, Ben Goodger (Google) wrote:
> Why make the generic statement that all interfaces will have to be served off
of
> different threads? I don't think that's true. (Unless you can tell me why it
> is.)

Removed comment about threading.

https://codereview.chromium.org/1737933002/diff/520001/content/browser/frame_...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1737933002/diff/520001/content/browser/frame_...
content/browser/frame_host/frame_mojo_shell.cc:65: mojo_user_id_, target->name,
We can't pass kInheritUser here since we're calling Shell::Connect(), which
dchecks on kInheritUser. Thankfully, our browser context has a user id.

[Ben Goodger (Google), 2016-03-14 22:25:57]

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/leveldb_thread.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:22:
base::Bind(&mojo::common::MessagePumpMojo::Create);
I think you should use another MessagePump type then. According to Yuzhu
MessagePumpMojo shouldn't be required anymore.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:34: leveldb_bindings_.reset();
What thread is this called on and what thread are the bindings on?

https://codereview.chromium.org/1737933002/diff/520001/content/browser/frame_...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1737933002/diff/520001/content/browser/frame_...
content/browser/frame_host/frame_mojo_shell.cc:40:
mojo_user_id_(BrowserContext::GetMojoUserIdFor(
do you need to do this here or can you just wait until you need it for
ConnectToApplication?

[Elliot Glaysher, 2016-03-14 23:14:53]

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/leveldb_thread.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:22:
base::Bind(&mojo::common::MessagePumpMojo::Create);
On 2016/03/14 22:25:57, Ben Goodger (Google) wrote:
> I think you should use another MessagePump type then. According to Yuzhu
> MessagePumpMojo shouldn't be required anymore.

Done. (There's a weird interaction where Threads using non-custom message loops
need to have SetThreadWasQuitProperly() before they're destroyed, but custom
message loops don't need that.)

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:34: leveldb_bindings_.reset();
On 2016/03/14 22:25:57, Ben Goodger (Google) wrote:
> What thread is this called on and what thread are the bindings on?

This base::Thread is started up, and in LevelDBThread::Init(), we create the
BindingSet so that it's bound to this base::Thread. We also set the connection
error there, so it will run on this base::Thread too. However, this class's
destructor gets run on our spawner's thread, which is why we need to be able to
reset the bindings here on error.

https://codereview.chromium.org/1737933002/diff/520001/content/browser/frame_...
File content/browser/frame_host/frame_mojo_shell.cc (right):

https://codereview.chromium.org/1737933002/diff/520001/content/browser/frame_...
content/browser/frame_host/frame_mojo_shell.cc:40:
mojo_user_id_(BrowserContext::GetMojoUserIdFor(
On 2016/03/14 22:25:57, Ben Goodger (Google) wrote:
> do you need to do this here or can you just wait until you need it for
> ConnectToApplication?

Done.

[michaeln, 2016-03-16 03:11:13]

As coded, we'll be adding two new dedicated threads for each Profile, one thread
exclusively handles *(LevelDBServiceImpl and LevelDBDatabaseImpl) method calls
and the other handles LevelDBFileThread'ing.

Most of the time these threads will be idle.

Jam mentioned work underway to bind an interface to a task_runner. At some point
it would be nice to decouple these classes from their dedicated threads.

In a world like that...

Each LevelDBServiceImpl and LevelDBDatabaseImpl instance could be bound to its
own sequence. And the file thread tasks, it looks like those don't have to run
in any particular order, or more accurately, the callers are are providing all
the ordering needed for the ops they initiate.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/leveldb_thread.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/leveldb_thread.cc:64:
parent_thread_message_loop_ref_.reset();
Maybe cleanup long lived objects with thread affinity here, a balancing delete
of the objects created in init.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/profile_app.cc:66: mojo::Connection* connection,
what is |connection| used for in this method?

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/profile_app.cc:68: if (!leveldb_thread_) {
if OnServiceError() with empty bindings had already occured... what happens
Create(conn, serviceReq) is called

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
File components/profile_service/leveldb_thread.cc (right):

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
components/profile_service/leveldb_thread.cc:32: leveldb_service_.reset();
Does this cause the threadmsg loop ref to drop to zero, what happens to the
thread at this point?

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
components/profile_service/leveldb_thread.cc:46: DCHECK(leveldb_service_);
Is this racey?

What guarantees levelddb_service and bindings are not null here? If we ever get
to zero bindings with a BindNewRequestImpl task in the queue, what happens?

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
File components/profile_service/leveldb_thread.h (right):

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
components/profile_service/leveldb_thread.h:65: leveldb::LevelDBServiceRequest
initial_leveldb_request_;
Is the second path to establish a binding needed? I think we might not need the
extra data member.

thrd = new ldbThread(looprf);
thrd->BindNewRequest(req);

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
components/profile_service/profile_app.cc:67: if (!leveldb_thread_) {
If the leveldb_thread_ self terminates, how is a new one established?

I'm not sure we want/need a self terminating ldbthread. It can terminate when
this app deletes deletes it.

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
File components/profile_service/profile_service_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/540001/components/profile_ser...
components/profile_service/profile_service_impl.cc:46: const mojo::String&
dir_name,
since this can actually be a nested directory path, consider renaming it?

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:87: const
mojo::String& origin,
needs rebaselining to const url::Origin& origin

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:106: //
--mojo-local-storage for now). Maps between an origin and its prefixed
nit: hoist the --mojo-locals-storage comment to the MojoState class level

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:108:
std::map<std::string, scoped_ptr<LevelDBWrapperImpl>> level_db_wrappers_;
ditto url::Origin rebaseline, and then the doc comment is not really needed

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:110: // The
underlying mojo user id.
nit: comment not really needed

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:113: // The
subdirectory of this dom storage context.
This directory is specifically for local storage, session storage goes
elsewhere. Either update the comment or just delete the comment, not sure it's
needed.

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:114: std::string
subdirectory_;
base::FilePath?

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:144:
mojo::String::From(subdirectory_),
when subdirectory_ is empty, as in the incognito case, what happens?

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:228: #if
defined(OS_WIN)
Resorting to platform specific path handling feels like a step backwards. Is it
possible to teach mojo about the base::FilePath datatype to avoid this? If we
must OS_ISFY it, maybe push this close as possible to the mojom api call and
storing/passing the value as a FilePath up till that point.

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.h (right):

https://codereview.chromium.org/1737933002/diff/540001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.h:79: // through the
public interface.
thnx

https://codereview.chromium.org/1737933002/diff/540001/content/browser/storag...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/1737933002/diff/540001/content/browser/storag...
content/browser/storage_partition_impl.h:146: const base::FilePath&
local_partition_path);
These two paths are confusing and i think potentially error prone. As coded...

  profile_path = {somerootdir}/rel_path
  local_partition_path = rel_path

That looks error prone because theirs nothing keeping a caller from passing in
values such that profile_path does not end in rel_path, and its difficult to
explain what the caller should pass in. Wdyt about something like...

  profile_path = {somerootdir}
  partition_relative_path = rel_path
 
Where the directory the partition should use is formed as:
{somerootdir}/rel_path.

[Elliot Glaysher, 2016-03-17 19:29:44]

Ownership has been radically changed.

LevelDBThread is gone; ProfileApplication takes two task runners (the file and
db threads) and binds the two services to them.

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/420001/components/profile_ser...
components/profile_service/profile_app.cc:66: mojo::Connection* connection,
On 2016/03/16 03:11:12, michaeln wrote:
> what is |connection| used for in this method?

Nothing, but it's part of the virtual interface since some things do do
connection specific stuff on Create().

https://codereview.chromium.org/1737933002/diff/540001/content/browser/storag...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/1737933002/diff/540001/content/browser/storag...
content/browser/storage_partition_impl.h:146: const base::FilePath&
local_partition_path);
On 2016/03/16 03:11:13, michaeln wrote:
> These two paths are confusing and i think potentially error prone. As coded...
> 
>   profile_path = {somerootdir}/rel_path
>   local_partition_path = rel_path
> 
> That looks error prone because theirs nothing keeping a caller from passing in
> values such that profile_path does not end in rel_path, and its difficult to
> explain what the caller should pass in. Wdyt about something like...
> 
>   profile_path = {somerootdir}
>   partition_relative_path = rel_path
>  
> Where the directory the partition should use is formed as:
> {somerootdir}/rel_path.

Done.

https://codereview.chromium.org/1737933002/diff/660001/content/common/mojo/cu...
File content/common/mojo/current_thread_loader.cc (right):

https://codereview.chromium.org/1737933002/diff/660001/content/common/mojo/cu...
content/common/mojo/current_thread_loader.cc:22: new
mojo::ShellConnection(shell_client_.get(), std::move(request))));
This is weird, but appears to be how things are done. The AndroidAppLoader, for
instance just keeps one shell connection forever. Can this be called multiple
times? (aka, should I remove the vector and switch it to a scoped_ptr?)

[michaeln, 2016-03-17 22:35:42]

this is looking pretty good to me, i did see a few nits, a number of my comments
are really questions about mojo

the dom_storage parts specifically lgtm

https://codereview.chromium.org/1737933002/diff/680001/components/filesystem/...
File components/filesystem/directory_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/components/filesystem/...
components/filesystem/directory_impl.cc:29: const scoped_refptr<LockTable>&
lock_table)
i don't like the lenghty threads about this, but i've read them, the smartptr
passing mavens would have you pass the refptr by value and ideally std::move()
it into the datamember below

https://codereview.chromium.org/1737933002/diff/680001/components/filesystem/...
File components/filesystem/file_impl.h (right):

https://codereview.chromium.org/1737933002/diff/680001/components/filesystem/...
components/filesystem/file_impl.h:34: const scoped_refptr<LockTable>&
lock_table);
ditto smartptr passing mavens by scoped_refptr<> value

https://codereview.chromium.org/1737933002/diff/680001/components/filesystem/...
File components/filesystem/file_system_impl.h (right):

https://codereview.chromium.org/1737933002/diff/680001/components/filesystem/...
components/filesystem/file_system_impl.h:35: const scoped_refptr<LockTable>&
lock_table);
and again

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
File components/leveldb/leveldb_app.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
components/leveldb/leveldb_app.cc:30: _exit(1);
do we ever expect to get here in chrome? is this even linked into chrome or is
this just for testing? just curious

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
File components/leveldb/leveldb_service_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
components/leveldb/leveldb_service_impl.cc:18:
LevelDBServiceImpl::LevelDBServiceImpl() : thread_(new LevelDBFileThread) {}
ok, so we get one new thread per profile

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
components/profile_service/profile_app.cc:46: class
ProfileApp::LevelDBServiceObjects
nice arrangement for thread safety!

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
components/profile_service/profile_app.cc:59: leveldb_service_.reset(new
leveldb::LevelDBServiceImpl);
does this direct instantiation mean we're not using the LevelDBApp? just
curious?

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
File components/profile_service/profile_app.h (right):

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
components/profile_service/profile_app.h:21: const
scoped_refptr<base::SingleThreadTaskRunner>& leveldb_service_runner);
ditto smartptr passing mavens

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
File components/profile_service/public/interfaces/profile.mojom (right):

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
components/profile_service/public/interfaces/profile.mojom:21:
GetSubDirectory(string dir_name, filesystem.Directory& dir)
naming nit: dir_path to match impl

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
File components/profile_service/user_id_map.h (right):

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
components/profile_service/user_id_map.h:13: // Currently, ProfileApp is run
from within the chrome process. This means it
"means it that" nixit

https://codereview.chromium.org/1737933002/diff/680001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/browse...
content/browser/browser_context.cc:342: while
(g_used_user_ids.Get().find(new_id) != g_used_user_ids.Get().end())
just curious, is this really needed or would a CHECK() be sufficient?

https://codereview.chromium.org/1737933002/diff/680001/content/browser/browse...
content/browser/browser_context.cc:348:
profile::AssociateMojoUserIDWithProfileDir(new_id, path);
can come later but... i think path is not-empty for incognito and reflects the
path of its main profile? if thats right, i'd vote to associate "" instead.

https://codereview.chromium.org/1737933002/diff/680001/content/browser/dom_st...
File content/browser/dom_storage/dom_storage_context_wrapper.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:92:
level_db_wrappers_.erase(origin);
given the current strategy of no leveldb backing for incognito, to not lose data
we'll need to keep the level_db_wrappers alive for the lifetime of the incognito
session, so only erase is database is not null

let's do that for now, but like we talked about, i'd vote to eventually change
strategies such that the LevelDbService can vend in-memory-only databases, idb
will definitely need that and there are other benifts too.

* makes incog vs non-incog code paths identifical from the callers pov, so less
special casing, less code, less bug

* tests can run MASSIVELY faster against an in memory databases, any consumer of
an instance should setup to run unittests tests that way

https://codereview.chromium.org/1737933002/diff/680001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:146:
mojo::String::From(subdirectory_.AsUTF8Unsafe()),
yup, utf8 is really safe since this is just the relpath from {profieldir} to the
"Local Storage" dircectory

https://codereview.chromium.org/1737933002/diff/680001/content/browser/dom_st...
content/browser/dom_storage/dom_storage_context_wrapper.cc:197: // |leveldb_|
should be know to either be valid or invalid by now. Run our
typo: known

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:267: shell_->SetLoaderForName(
SetLoaderForName() used to happen unconditionally (albeit slightly differently).
It's only needed for the localstorage stuff? Just checking.

Why change to do this conditionally? Just curious?

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo_s...
File content/browser/mojo_shell_browsertest.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo_s...
content/browser/mojo_shell_browsertest.cc:58: // With no loader registered at
this URL, the shell should spawn a utility
oh, if you don't set the loader, the system automatically creates a new process?
So in the absence switches::kMojoLocalStorage a new process will get created at
the callsite i asked about earlier (just above).

https://codereview.chromium.org/1737933002/diff/680001/content/browser/storag...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/storag...
content/browser/storage_partition_impl.h:134: // The |partition_path| is the
absolute path to the root of this
comment is confusing and the param names don't match the method sig?

The |profile_path| is the absolute path to the root of the profile's directory.
The |relative_partition_path| is the path from that root to the directory
specifically for this StorageParititon. The |relative_partition_path| may be
empty empty, this is the case for the default partition whose data resides
directly in |profile_path|.

?

https://codereview.chromium.org/1737933002/diff/680001/content/common/mojo/cu...
File content/common/mojo/current_thread_loader.h (right):

https://codereview.chromium.org/1737933002/diff/680001/content/common/mojo/cu...
content/common/mojo/current_thread_loader.h:33: // is called exactly once since
all
since all what?

[Elliot Glaysher, 2016-03-18 19:41:13]

jam: could you please do an OWNERS review?

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
File components/leveldb/leveldb_app.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
components/leveldb/leveldb_app.cc:30: _exit(1);
On 2016/03/17 22:35:41, michaeln wrote:
> do we ever expect to get here in chrome? is this even linked into chrome or is
> this just for testing? just curious

This is not linked into chrome.

(I don't actually know why this is here. Ken, who added this, mentioned in the
commit message to https://codereview.chromium.org/1801963002 that this is the
wrong thing and is temporary.)

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
File components/leveldb/leveldb_service_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/components/leveldb/lev...
components/leveldb/leveldb_service_impl.cc:18:
LevelDBServiceImpl::LevelDBServiceImpl() : thread_(new LevelDBFileThread) {}
On 2016/03/17 22:35:41, michaeln wrote:
> ok, so we get one new thread per profile

For now at least.

It might be possible to depinject the file thread here.

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
File components/profile_service/profile_app.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/components/profile_ser...
components/profile_service/profile_app.cc:59: leveldb_service_.reset(new
leveldb::LevelDBServiceImpl);
On 2016/03/17 22:35:41, michaeln wrote:
> does this direct instantiation mean we're not using the LevelDBApp? just
> curious?

Correct. Apps don't contain other apps. Apps contain services. And the services
should be arranged in a way that they're reusable across apps.

(If you look at components/leveldb/BUILD.gn, you'll see we don't even compile
the LevelDBApp into the general library.)

https://codereview.chromium.org/1737933002/diff/680001/content/browser/browse...
File content/browser/browser_context.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/browse...
content/browser/browser_context.cc:342: while
(g_used_user_ids.Get().find(new_id) != g_used_user_ids.Get().end())
On 2016/03/17 22:35:41, michaeln wrote:
> just curious, is this really needed or would a CHECK() be sufficient?

It is extraordinarily improbable, but in case GenerateGUID() returns two same
128 bit numbers we shouldn't crash...

(Admittedly, this was much more probable when user ids were 31 bit numbers.)

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo/m...
File content/browser/mojo/mojo_shell_context.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo/m...
content/browser/mojo/mojo_shell_context.cc:267: shell_->SetLoaderForName(
On 2016/03/17 22:35:42, michaeln wrote:
> SetLoaderForName() used to happen unconditionally (albeit slightly
differently).
> It's only needed for the localstorage stuff? Just checking.
> 
> Why change to do this conditionally? Just curious?

Right now, the only caller is local storage and I'd like to minimize impact on
normal chrome execution while developing this.

(I should have had it behind a flag from the beginning.)

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo_s...
File content/browser/mojo_shell_browsertest.cc (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/mojo_s...
content/browser/mojo_shell_browsertest.cc:58: // With no loader registered at
this URL, the shell should spawn a utility
On 2016/03/17 22:35:42, michaeln wrote:
> oh, if you don't set the loader, the system automatically creates a new
process?
> So in the absence switches::kMojoLocalStorage a new process will get created
at
> the callsite i asked about earlier (just above).

IIUC, depends on the runner.

In the default mojo runner, this is in fact what happens. However, in
MojoShellContext, the default loader is set to a special loader that ignores all
loads. (see mojo_shell_context.cc:73).

https://codereview.chromium.org/1737933002/diff/680001/content/browser/storag...
File content/browser/storage_partition_impl.h (right):

https://codereview.chromium.org/1737933002/diff/680001/content/browser/storag...
content/browser/storage_partition_impl.h:134: // The |partition_path| is the
absolute path to the root of this
On 2016/03/17 22:35:42, michaeln wrote:
> comment is confusing and the param names don't match the method sig?
> 
> The |profile_path| is the absolute path to the root of the profile's
directory.
> The |relative_partition_path| is the path from that root to the directory
> specifically for this StorageParititon. The |relative_partition_path| may be
> empty empty, this is the case for the default partition whose data resides
> directly in |profile_path|.
> 
> ?

Fixed comment.

[jam, 2016-03-18 22:31:27]

lgtm

[Elliot Glaysher, 2016-03-18 22:45:08]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-03-18 22:45:09]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1737933002/#ps720001
(title: "Do the profile path thing jam wanted.")

[commit-bot: I haz the power, 2016-03-18 22:45:34]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737933002/720001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737933002/720001

[commit-bot: I haz the power, 2016-03-19 00:58:13]

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

[commit-bot: I haz the power, 2016-03-19 00:58:18]

Failed to apply patch for content/browser/leveldb_wrapper_impl.cc:
While running git apply --index -3 -p1;
  error: patch failed: content/browser/leveldb_wrapper_impl.cc:9
  Falling back to three-way merge...
  Applied patch to 'content/browser/leveldb_wrapper_impl.cc' with conflicts.
  U content/browser/leveldb_wrapper_impl.cc

Patch:       content/browser/leveldb_wrapper_impl.cc
Index: content/browser/leveldb_wrapper_impl.cc
diff --git a/content/browser/leveldb_wrapper_impl.cc
b/content/browser/leveldb_wrapper_impl.cc
index
137e5c42fe6a5f43d2a370a0a30b877caaa1b962..623b728e19ee4db7d4e0e0ddfabc869489736c26
100644
--- a/content/browser/leveldb_wrapper_impl.cc
+++ b/content/browser/leveldb_wrapper_impl.cc
@@ -9,43 +9,57 @@
 namespace content {
 
 LevelDBWrapperImpl::LevelDBWrapperImpl(
-    const std::string& prefix, const base::Closure& no_bindings_callback)
-    : prefix_(prefix), no_bindings_callback_(no_bindings_callback) {
+    leveldb::LevelDBDatabase* database,
+    const std::string& prefix,
+    const base::Closure& no_bindings_callback)
+    : prefix_(prefix),
+      no_bindings_callback_(no_bindings_callback),
+      database_(database) {
   bindings_.set_connection_error_handler(base::Bind(
       &LevelDBWrapperImpl::OnConnectionError, base::Unretained(this)));
 }
 
-void LevelDBWrapperImpl::Bind(mojo::InterfaceRequest<LevelDBWrapper> request) {
+void LevelDBWrapperImpl::Bind(LevelDBWrapperRequest request) {
   bindings_.AddBinding(this, std::move(request));
 }
 
-LevelDBWrapperImpl::~LevelDBWrapperImpl() {
-}
+LevelDBWrapperImpl::~LevelDBWrapperImpl() {}
 
 void LevelDBWrapperImpl::Put(mojo::Array<uint8_t> key,
                              mojo::Array<uint8_t> value,
                              const mojo::String& source,
                              const PutCallback& callback) {
+  NOTIMPLEMENTED();
+  callback.Run(leveldb::DatabaseError::NOT_SUPPORTED);
 }
 
 void LevelDBWrapperImpl::Delete(mojo::Array<uint8_t> key,
                                 const mojo::String& source,
                                 const DeleteCallback& callback) {
+  NOTIMPLEMENTED();
+  callback.Run(leveldb::DatabaseError::NOT_SUPPORTED);
 }
 
 void LevelDBWrapperImpl::DeleteAll(LevelDBObserverPtr observer,
                                    const mojo::String& source,
                                    const DeleteAllCallback& callback) {
   // TODO(jam): store observer and call it when changes occur.
+  NOTIMPLEMENTED();
+  callback.Run(leveldb::DatabaseError::NOT_SUPPORTED);
 }
 
 void LevelDBWrapperImpl::Get(mojo::Array<uint8_t> key,
                              const GetCallback& callback) {
+  NOTIMPLEMENTED();
+  callback.Run(leveldb::DatabaseError::NOT_SUPPORTED, mojo::Array<uint8_t>());
 }
 
 void LevelDBWrapperImpl::GetAll(LevelDBObserverPtr observer,
                                 const GetAllCallback& callback) {
   // TODO(jam): store observer and call it when changes occur.
+  NOTIMPLEMENTED();
+  callback.Run(leveldb::DatabaseError::NOT_SUPPORTED,
+               mojo::Array<KeyValuePtr>());
 }
 
 void LevelDBWrapperImpl::OnConnectionError() {

[Elliot Glaysher, 2016-03-21 17:27:17]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2016-03-21 17:27:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
michaeln@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/1737933002/#ps740001
(title: "merge tot")

[commit-bot: I haz the power, 2016-03-21 17:27:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1737933002/740001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1737933002/740001

[commit-bot: I haz the power, 2016-03-21 18:55:28]

Committed patchset #38 (id:740001)

[commit-bot: I haz the power, 2016-03-21 18:56:44]

Description was changed from

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c112794307cc4858616f5aeaa05dbfe0d89b671e
Cr-Commit-Position: refs/heads/master@{#382343}
==========

[commit-bot: I haz the power, 2016-03-21 18:56:46]

Patchset 38 (id:??) landed as
https://crrev.com/c112794307cc4858616f5aeaa05dbfe0d89b671e
Cr-Commit-Position: refs/heads/master@{#382343}

[horo, 2016-03-31 04:23:17]

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

[horo, 2016-03-31 04:23:19]

https://codereview.chromium.org/1737933002/diff/740001/content/browser/storag...
File content/browser/storage_partition_impl.cc (right):

https://codereview.chromium.org/1737933002/diff/740001/content/browser/storag...
content/browser/storage_partition_impl.cc:446: base::FilePath path = in_memory ?
base::FilePath() : context->GetPath();
|path| is also used by IndexedDBContextImpl, CacheStorageContextImpl,
ServiceWorkerContextWrapper, WebRTCIdentityStore and
PlatformNotificationContextImpl.

I think this CL has broken those storage.

https://crbug.com/598314

[nhiroki, 2016-03-31 07:39:56]

Description was changed from

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c112794307cc4858616f5aeaa05dbfe0d89b671e
Cr-Commit-Position: refs/heads/master@{#382343}
==========

to

==========
mojo leveldb: Get profile and leveldb connected to DOMStorageContext.

Building on the last patch which built mojo:profile and built one
for each BrowserContext, we move the LevelDBService in the profile
application to a dedicated leveldb thread to avoid deadlocks.
(Since applications all run on one thread by default, leveldb would
deadlock while waiting for profile to return data.)

This patch connects to the correct applications in
DOMStorageContextWrapper and then vends a LevelDBService instance to
each LevelDBWrapperImpl, for future use.

BUG=586194
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/c112794307cc4858616f5aeaa05dbfe0d89b671e
Cr-Commit-Position: refs/heads/master@{#382343}
==========