mandoline filesystem: Save cookie data to the mojo:filesystem.

mandoline filesystem: Save cookie data to the mojo:filesystem.

This makes the network service use the sql vfs to proxy writing the
cookies to the filesystem service. This means mojo:network_service does
not directly write its data to the OS filesystem, which will allow us to
sandbox it.

BUG=493311
Committed: https://crrev.com/bbb9e2f6486cccadde4dc2fa077af8f694105eaa
Cr-Commit-Position: refs/heads/master@{#337491}

[Elliot Glaysher, 2015-06-22 20:03:30]

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

[Elliot Glaysher, 2015-06-22 20:03:31]

msw: testing question.
jam: timing question.

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
File mojo/services/network/network_context.cc (right):

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
mojo/services/network/network_context.cc:132: // shell will corrupt the disk
cache.
This testing detail needs to be dealt with.

Now that we have mojo:network_service talking to mojo:filesystem, we need to
either

1) have a way to signal to the network_service to run in a testing mode where
our network_service asks for a temp dir instead of its origin dir. (see the
initialization in network_service_delegate.cc; all that needs to be changed is
"origin" to "temp" on a flag.)

2) Make the above comment not true; always store things on disk.

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
File mojo/services/network/network_service_delegate.cc (right):

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
mojo/services/network/network_service_delegate.cc:108:
io_worker_thread_.reset();
I worry about timing and unclean pipe shutdown.

Is Quit() the point where things are signaled to shutdown but haven't destroyed
pipes yet? (ie, we'll have our pipe to the filesystem still live here). If not,
is there a different place I should put this? What other premature pipe
destruction cases do I need to handle?

[msw, 2015-06-22 23:02:49]

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
File mojo/services/network/network_context.cc (right):

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
mojo/services/network/network_context.cc:132: // shell will corrupt the disk
cache.
On 2015/06/22 20:03:31, Elliot Glaysher wrote:
> This testing detail needs to be dealt with.
> 
> Now that we have mojo:network_service talking to mojo:filesystem, we need to
> either
> 
> 1) have a way to signal to the network_service to run in a testing mode where
> our network_service asks for a temp dir instead of its origin dir. (see the
> initialization in network_service_delegate.cc; all that needs to be changed is
> "origin" to "temp" on a flag.)
> 
> 2) Make the above comment not true; always store things on disk.

We currently don't test in parallel (we should, but there's no ETA on that).
So 2+ shells/runners using the network service in parallel is racy/broken?
Should NetworkServiceDelegate::Initialize always make a new temp dir (2)?
A test url mapping or flag could be ok, but we'd need that for many tests...
Sorry if I don't understand this well enough...

[Elliot Glaysher, 2015-06-22 23:36:39]

jam: requesting real review this time.

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
File mojo/services/network/network_context.cc (right):

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
mojo/services/network/network_context.cc:132: // shell will corrupt the disk
cache.
On 2015/06/22 23:02:49, msw wrote:
> On 2015/06/22 20:03:31, Elliot Glaysher wrote:
> > This testing detail needs to be dealt with.
> > 
> > Now that we have mojo:network_service talking to mojo:filesystem, we need to
> > either
> > 
> > 1) have a way to signal to the network_service to run in a testing mode
where
> > our network_service asks for a temp dir instead of its origin dir. (see the
> > initialization in network_service_delegate.cc; all that needs to be changed
is
> > "origin" to "temp" on a flag.)
> > 
> > 2) Make the above comment not true; always store things on disk.
> 
> We currently don't test in parallel (we should, but there's no ETA on that).
> So 2+ shells/runners using the network service in parallel is racy/broken?
> Should NetworkServiceDelegate::Initialize always make a new temp dir (2)?
> A test url mapping or flag could be ok, but we'd need that for many tests...
> Sorry if I don't understand this well enough...

NSD::Initialize can easily make a new temp dir every time, however that would
have to be controlled by a flag because the network service running for real
should use a persistent directory (so when you run mandoline two times, cookies
persist between invocations).

If we don't currently run tests in parallel, I can just do the easy thing and
not worry about this.

[msw, 2015-06-22 23:43:47]

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
File mojo/services/network/network_context.cc (right):

https://codereview.chromium.org/1179413010/diff/160001/mojo/services/network/...
mojo/services/network/network_context.cc:132: // shell will corrupt the disk
cache.
On 2015/06/22 23:36:39, Elliot Glaysher wrote:
> On 2015/06/22 23:02:49, msw wrote:
> > On 2015/06/22 20:03:31, Elliot Glaysher wrote:
> > > This testing detail needs to be dealt with.
> > > 
> > > Now that we have mojo:network_service talking to mojo:filesystem, we need
to
> > > either
> > > 
> > > 1) have a way to signal to the network_service to run in a testing mode
> where
> > > our network_service asks for a temp dir instead of its origin dir. (see
the
> > > initialization in network_service_delegate.cc; all that needs to be
changed
> is
> > > "origin" to "temp" on a flag.)
> > > 
> > > 2) Make the above comment not true; always store things on disk.
> > 
> > We currently don't test in parallel (we should, but there's no ETA on that).
> > So 2+ shells/runners using the network service in parallel is racy/broken?
> > Should NetworkServiceDelegate::Initialize always make a new temp dir (2)?
> > A test url mapping or flag could be ok, but we'd need that for many tests...
> > Sorry if I don't understand this well enough...
> 
> NSD::Initialize can easily make a new temp dir every time, however that would
> have to be controlled by a flag because the network service running for real
> should use a persistent directory (so when you run mandoline two times,
cookies
> persist between invocations).
> 
> If we don't currently run tests in parallel, I can just do the easy thing and
> not worry about this.

sgtm, I'm sure this wouldn't be the only obstacle to concurrent test execution.

[Elliot Glaysher, 2015-06-23 23:50:26]

So this creates temporary directories for each test invocation...on windows and
linux. I'm not sure how to pass arguments to android. (I assume that it should
be done in gtest.py too...)

[msw, 2015-06-24 00:14:41]

https://codereview.chromium.org/1179413010/diff/180001/mojo/tools/mopy/gtest.py
File mojo/tools/mopy/gtest.py (right):

https://codereview.chromium.org/1179413010/diff/180001/mojo/tools/mopy/gtest....
mojo/tools/mopy/gtest.py:159: temp_dir = tempfile.mkdtemp()
Avoid making the temp dir and appending the argument when listing tests; you can
add a check like in _build_command_line: |if not "--gtest_list_tests" in args:|.
I'd like a cleaner fix for per-test temp-dirs (given this problem and the error
output comment below), but nothing brilliant comes to mind.

https://codereview.chromium.org/1179413010/diff/180001/mojo/tools/mopy/gtest....
mojo/tools/mopy/gtest.py:161: command += ['--user-data-dir=%s' % temp_dir]
Hmm, this argument won't appear in the logs when _print_error is called with
other _build_command_line invocations... Maybe add some other logging so it's at
least printed somewhere? You might not be able to use
|logging.getLogger().debug("Using temporary user-data-dir: " + temp_dir)| since
this function is run in a separate process, but you should be able to append a
string like that to |result|.

https://codereview.chromium.org/1179413010/diff/180001/mojo/tools/mopy/gtest....
mojo/tools/mopy/gtest.py:170: shell.StartActivity('MojoShellActivity', args +
[apptest], wf, wf.close)
I guess we'll want a shell.mkdtemp and shell.rmtree implemented in
mojo/tools/mopy/android.py using pylib's adb commands (as much as possible)

[Elliot Glaysher, 2015-06-24 22:22:41]

https://codereview.chromium.org/1179413010/diff/180001/mojo/tools/mopy/gtest.py
File mojo/tools/mopy/gtest.py (right):

https://codereview.chromium.org/1179413010/diff/180001/mojo/tools/mopy/gtest....
mojo/tools/mopy/gtest.py:159: temp_dir = tempfile.mkdtemp()
On 2015/06/24 00:14:40, msw wrote:
> Avoid making the temp dir and appending the argument when listing tests; you
can
> add a check like in _build_command_line: |if not "--gtest_list_tests" in
args:|.
> I'd like a cleaner fix for per-test temp-dirs (given this problem and the
error
> output comment below), but nothing brilliant comes to mind.

I've changed the entire approach here. I set a --use-temporary-user-data-dir
flag, which the mojo_runner sees, creates a temporary data directory very early
in the lifecycle and then sets the --user-data-dir flag to that in the C++ code.
That should works cross platform with android.

[msw, 2015-06-24 23:00:35]

lgtm with nits, please try apptests locally before landing.

https://codereview.chromium.org/1179413010/diff/200001/mojo/runner/scoped_use...
File mojo/runner/scoped_user_data_dir.cc (right):

https://codereview.chromium.org/1179413010/diff/200001/mojo/runner/scoped_use...
mojo/runner/scoped_user_data_dir.cc:28:
command_line->AppendSwitchPath(switches::kUserDataDir, temp_dir_.path());
Heh, the irony here is that you're appending --user-data-dir after
--use-temporary-user-data-dir is detected, which violates the statement above.
(it's fine, but you may want to revise the error string or add a comment here)

https://codereview.chromium.org/1179413010/diff/200001/mojo/runner/switches.cc
File mojo/runner/switches.cc (right):

https://codereview.chromium.org/1179413010/diff/200001/mojo/runner/switches.c...
mojo/runner/switches.cc:45: const char kUseTemporaryUserDataDir[] =
"use-temporary-user-data-dir";
nit: abc ordering

https://codereview.chromium.org/1179413010/diff/200001/mojo/runner/switches.h
File mojo/runner/switches.h (right):

https://codereview.chromium.org/1179413010/diff/200001/mojo/runner/switches.h...
mojo/runner/switches.h:23: extern const char kUseTemporaryUserDataDir[];
nit: abc ordering

https://codereview.chromium.org/1179413010/diff/200001/mojo/tools/mopy/gtest.py
File mojo/tools/mopy/gtest.py (right):

https://codereview.chromium.org/1179413010/diff/200001/mojo/tools/mopy/gtest....
mojo/tools/mopy/gtest.py:9: import shutil
nit: remove

https://codereview.chromium.org/1179413010/diff/200001/mojo/tools/mopy/gtest....
mojo/tools/mopy/gtest.py:12: import tempfile
nit: remove

[Elliot Glaysher, 2015-06-24 23:10:30]

ping jam for mojo review

[jam, 2015-06-25 15:28:24]

https://codereview.chromium.org/1179413010/diff/220001/mojo/runner/desktop/ma...
File mojo/runner/desktop/main.cc (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/runner/desktop/ma...
mojo/runner/desktop/main.cc:19: mojo::runner::ScopedUserDataDir
scoped_user_data_dir;
a few issues of putting it here: it's running for child process (which could be
fixed by moving it down), but also it's not running for mandoline since that has
a different main in mandoline/app/desktop/main.cc. also it would be good to put
this somewhere shared with android (then we don't really need a class around
these few lines). what about in mojo::runner::Context?

https://codereview.chromium.org/1179413010/diff/220001/mojo/runner/scoped_use...
File mojo/runner/scoped_user_data_dir.cc (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/runner/scoped_use...
mojo/runner/scoped_user_data_dir.cc:16: if
(command_line->HasSwitch(switches::kUseTemporaryUserDataDir)) {
nit: early return here would remove indentation

https://codereview.chromium.org/1179413010/diff/220001/mojo/runner/scoped_use...
mojo/runner/scoped_user_data_dir.cc:17: if
(command_line->HasSwitch(switches::kUserDataDir)) {
nit: just CHECK(!command_line->HasSwitch(switches::kUserDataDir));

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
File mojo/services/network/DEPS (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
mojo/services/network/DEPS:3: "+components/filesystem",
better to make this components/filesystem/public/interfaces/ to ensure no one
starts using non-public files

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
File mojo/services/network/network_context.cc (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
mojo/services/network/network_context.cc:30: }  // namespace
nit: i hadn't added this since this seems unnecessary when the block is short

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
File mojo/services/network/network_context.h (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
mojo/services/network/network_context.h:31: explicit NetworkContext(
nit: no explicit

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
File mojo/services/network/network_service_delegate.cc (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
mojo/services/network/network_service_delegate.cc:91: if
(command_line->HasSwitch("user-data-dir")) {
seems this should use a constant

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
mojo/services/network/network_service_delegate.cc:94:
CHECK(PathService::Get(base::DIR_TEMP, &base_path));
why have all the services that need to write to disk (there may be more than
this one) look for user-data-dir and if it's not there create a temp, as opposed
to do this once in the shell process and then always have it pass user-data-dir
(which may be temp)?

[Elliot Glaysher, 2015-06-25 22:46:46]

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

[Elliot Glaysher, 2015-06-25 22:46:48]

Redirecting jam->sky since jam is gone for a while now.

I'm basically spinning my wheels on the android stuff now. Given that we're
going to be shut down for at least a week, I've just #ifdef-ed out the
initialization code on android, and have verified that apptest pass on an
android device with these blocks ifdefed out.

(To reiterate, all this works in mandoline on android when we let the filesystem
initialize itself; this only timeout/deadlocks in the tests, and I still haven't
been able to get anything working there.)

Is there a platform expert on how mojo android initialization works that I can
talk to in a week when I come back from vacation?

https://codereview.chromium.org/1179413010/diff/220001/mojo/runner/scoped_use...
File mojo/runner/scoped_user_data_dir.cc (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/runner/scoped_use...
mojo/runner/scoped_user_data_dir.cc:17: if
(command_line->HasSwitch(switches::kUserDataDir)) {
On 2015/06/25 15:28:23, jam wrote:
> nit: just CHECK(!command_line->HasSwitch(switches::kUserDataDir));

Disagree. Errors that users can trigger should have proper error messages. Did
clean up message a bit more though.

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
File mojo/services/network/network_service_delegate.cc (right):

https://codereview.chromium.org/1179413010/diff/220001/mojo/services/network/...
mojo/services/network/network_service_delegate.cc:94:
CHECK(PathService::Get(base::DIR_TEMP, &base_path));
On 2015/06/25 15:28:24, jam wrote:
> why have all the services that need to write to disk (there may be more than
> this one) look for user-data-dir and if it's not there create a temp, as
opposed
> to do this once in the shell process and then always have it pass
user-data-dir
> (which may be temp)?

We don't want these services to actually have any dependency on paths or to do
any calculation. We want services (other than the filesystem process) to have
any interaction with the disk at all. This entire block here should be deleted
two or three patch sets in the future, but is scaffolding for now.

(This code here has nothing to do with where we put cookies. It exists for just
the network disk cache at this point.)

[sky, 2015-06-25 23:28:51]

Rubber stamp LGTM. John can re-review when he gets back.

[Elliot Glaysher, 2015-06-26 00:33:46]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-06-26 00:33:47]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org
Link to the patchset: https://codereview.chromium.org/1179413010/#ps260001
(title: "Punt on the android issue.")

[commit-bot: I haz the power, 2015-06-26 00:33:59]

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

[commit-bot: I haz the power, 2015-06-26 00:42:21]

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

[commit-bot: I haz the power, 2015-06-26 00:42:23]

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

[Elliot Glaysher, 2015-07-06 20:35:22]

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

[Elliot Glaysher, 2015-07-06 20:35:24]

+shess for owners stamp on //sql/

[Scott Hess - ex-Googler, 2015-07-06 20:48:00]

LGTM, minor suggestion.

https://codereview.chromium.org/1179413010/diff/280001/sql/mojo/mojo_vfs.cc
File sql/mojo/mojo_vfs.cc (right):

https://codereview.chromium.org/1179413010/diff/280001/sql/mojo/mojo_vfs.cc#n...
sql/mojo/mojo_vfs.cc:255: mojo_name = base::StringPrintf("Temp_%d.db",
temp_number++);
AFAICT this is in the same namespace as other databases?  Might be worthwhile to
have the named case CHECK that the name is not of this form.

[Elliot Glaysher, 2015-07-06 21:03:36]

The CQ bit was checked by erg@chromium.org

[Elliot Glaysher, 2015-07-06 21:03:37]

The patchset sent to the CQ was uploaded after l-g-t-m from msw@chromium.org,
sky@chromium.org, shess@chromium.org
Link to the patchset: https://codereview.chromium.org/1179413010/#ps300001
(title: "Add CHECK.")

[commit-bot: I haz the power, 2015-07-06 21:05:39]

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

[commit-bot: I haz the power, 2015-07-06 22:15:32]

Committed patchset #16 (id:300001)

[commit-bot: I haz the power, 2015-07-06 22:16:24]

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

[Fady Samuel, 2015-07-07 14:49:34]

A revert of this CL (patchset #16 id:300001) has been created in
https://codereview.chromium.org/1225673006/ by fsamuel@chromium.org.

The reason for reverting is: I'm seeing the following crash when closing the
Mandoline window on Linux:

[0707/104753:FATAL:sqlite_persistent_cookie_store.cc(1135)] Check failed: false.
Could not add a cookie to the DB.
#0 0x00000046aef1 __interceptor_backtrace
#1 0x7fec4b87954e base::debug::StackTrace::StackTrace()
#2 0x7fec4b479a1d logging::LogMessage::~LogMessage()
#3 0x7fec53f03c23 net::SQLitePersistentCookieStore::Backend::Commit()
#4 0x7fec53f04f38
net::SQLitePersistentCookieStore::Backend::InternalBackgroundClose()
#5 0x7fec53f1902b base::internal::RunnableAdapter<>::Run()

Reverting this patch resolves the issue..