Always use mojo_shell in over-http mode

Always use mojo_shell in over-http mode

--origin tells mojo_shell to map all mojo: urls
to to a new base-url instead of the build directory.

This makes skydb's mojo_shell *always* use the network
loading path, which is what Android mojo_shell does
and is more like how mojo_shell will eventually be
used.

I also fixed the disk-space leak in the
dynamic_application_loader's NetworkLoader path
by having it immediately unlink the /tmp copy
of the library after dlopen.

In order to keep pprof working I had to teach the
dynamic_application_loader to write out a map of
/tmp/file -> url mappings so that we can fix
the pprof file afterwords.

This will "break" skydb --gdb on linux in exactly
as much as it is already broken on Android, but
I'm working on a build-id based solution for both
so that gdb knows how to find symbols for
non-existant randomly named /tmp libraries.

R=abarth@chromium.org, viettrungluu@chromium.org
BUG=450696
Committed: https://chromium.googlesource.com/external/mojo/+/05d7c0dc584f9bde9ea4135fcb1b17e7088ee671

[abarth-chromium, 2015-01-20 23:20:52]

lgtm

[eseidel, 2015-01-20 23:46:29]

This is going to break adam's pprofing, so I need to find a way to get
mojo_shell to dump it's http -> tmp file mappings (other than its current stdout
logging) before I land this.

[eseidel, 2015-01-21 22:11:42]

Updated to fix pprof and the disk-space leak.  PTAL.

[abarth-chromium, 2015-01-21 22:18:49]

Looks fine, but you need someone else to review the shell parts.

https://codereview.chromium.org/829183005/diff/20001/sky/tools/debugger/promp...
File sky/tools/debugger/prompt/prompt.cc (right):

https://codereview.chromium.org/829183005/diff/20001/sky/tools/debugger/promp...
sky/tools/debugger/prompt/prompt.cc:186: "Profiling requires debug=false and
enable_profiling=true");
debug=false -> is_debug=false

https://codereview.chromium.org/829183005/diff/20001/sky/tools/skydb
File sky/tools/skydb (right):

https://codereview.chromium.org/829183005/diff/20001/sky/tools/skydb#newcode358
sky/tools/skydb:358: as_string = profile_file.read().decode('iso-8859-1')
You should comment on why this encoding

[jamesr, 2015-01-21 22:23:04]

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

[jamesr, 2015-01-21 22:23:04]

The file saving part seems fine, the way the unlink is hooked up seems fishy to
me.  Could you break that out into a separate part and loop in Trung or someone
to figure out the right way to do the loader integration?

https://codereview.chromium.org/829183005/diff/20001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/829183005/diff/20001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:366: LOG(INFO) << "ReportComplete";
too spammy

https://codereview.chromium.org/829183005/diff/20001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:367: Loader::ReportComplete();
relying on implementation inheritance here seems pretty fishy...

[eseidel, 2015-01-21 22:44:43]

https://codereview.chromium.org/829183005/diff/20001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/829183005/diff/20001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:366: LOG(INFO) << "ReportComplete";
On 2015/01/21 22:23:04, jamesr wrote:
> too spammy

Oops!  Was a debug log.  Thx!

https://codereview.chromium.org/829183005/diff/20001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:367: Loader::ReportComplete();
On 2015/01/21 22:23:04, jamesr wrote:
> relying on implementation inheritance here seems pretty fishy...

Oh?  I just want to call super().

https://codereview.chromium.org/829183005/diff/20001/sky/tools/debugger/promp...
File sky/tools/debugger/prompt/prompt.cc (right):

https://codereview.chromium.org/829183005/diff/20001/sky/tools/debugger/promp...
sky/tools/debugger/prompt/prompt.cc:186: "Profiling requires debug=false and
enable_profiling=true");
On 2015/01/21 22:18:49, abarth wrote:
> debug=false -> is_debug=false

Done.

https://codereview.chromium.org/829183005/diff/20001/sky/tools/skydb
File sky/tools/skydb (right):

https://codereview.chromium.org/829183005/diff/20001/sky/tools/skydb#newcode358
sky/tools/skydb:358: as_string = profile_file.read().decode('iso-8859-1')
On 2015/01/21 22:18:49, abarth wrote:
> You should comment on why this encoding

Done.

[eseidel, 2015-01-21 22:46:00]

eseidel@chromium.org changed reviewers:
+ viettrungluu@chromium.org

[eseidel, 2015-01-21 22:46:01]

Trung, can you review the dynamic_application_loader bits?

[vtl, 2015-01-21 23:16:20]

vtl@google.com changed reviewers:
+ vtl@google.com

[vtl, 2015-01-21 23:16:21]

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:295: base::DeleteFile(path_, false);
This makes me sad, but oh well.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:296: path_ = path_.AddExtension(".mojo"); //
Make libraries easy to spot.
nit: Usually we put two spaces before the "//" for same-line comments.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:304: // Record the URL -> temp-file mapping
for debugging tools.
Could you factor this out into a separate function?

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:306: GetTempDir(&temp_dir);
You should probably say base::GetTempDir, rather than relying on ADL.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:311: std::string map_entry =
base::StringPrintf("%s %s\n",
Probably we should consider quoting/escaping the path/URL at some point, but we
can worry about that later.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:367: // As soon as we've loaded the library
we can delete the tmp file.
"tmp" -> "temp"?

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:372: 
nit: One fewer blank line.

[eseidel, 2015-01-21 23:23:31]

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:295: base::DeleteFile(path_, false);
On 2015/01/21 23:16:21, vtl wrote:
> This makes me sad, but oh well.

Acknowledged.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:296: path_ = path_.AddExtension(".mojo"); //
Make libraries easy to spot.
On 2015/01/21 23:16:21, vtl wrote:
> nit: Usually we put two spaces before the "//" for same-line comments.

Done.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:304: // Record the URL -> temp-file mapping
for debugging tools.
On 2015/01/21 23:16:21, vtl wrote:
> Could you factor this out into a separate function?

Done.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:306: GetTempDir(&temp_dir);
On 2015/01/21 23:16:21, vtl wrote:
> You should probably say base::GetTempDir, rather than relying on ADL.

Done.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:311: std::string map_entry =
base::StringPrintf("%s %s\n",
On 2015/01/21 23:16:21, vtl wrote:
> Probably we should consider quoting/escaping the path/URL at some point, but
we
> can worry about that later.

Acknowledged.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:367: // As soon as we've loaded the library
we can delete the tmp file.
On 2015/01/21 23:16:21, vtl wrote:
> "tmp" -> "temp"?

Done.

https://codereview.chromium.org/829183005/diff/40001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:372: 
On 2015/01/21 23:16:21, vtl wrote:
> nit: One fewer blank line.

Done.

[eseidel, 2015-01-21 23:23:54]

PTAL

[viettrungluu, 2015-01-21 23:32:50]

LGTM w/nit

https://codereview.chromium.org/829183005/diff/60001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/829183005/diff/60001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:285: static
nit: Probably the "canonical" way to format this would be:

static void RecordCacheToURLMapping(const base::FilePath& path,
                                    const GURL& url) {

(you can use git cl format if you want, though I don't believe that shell/ has
been mass-clang-formatted yet)

[eseidel, 2015-01-21 23:53:21]

Committed patchset #4 (id:60001) manually as
05d7c0dc584f9bde9ea4135fcb1b17e7088ee671 (presubmit successful).

[qsr, 2015-01-22 16:29:07]

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

[qsr, 2015-01-22 16:29:07]

https://codereview.chromium.org/829183005/diff/60001/shell/dynamic_applicatio...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/829183005/diff/60001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:375: DeleteFile(path_, false);
I don't think this fix the issue with file leaks. This is called when the
application is finished. Most of our apps never finish.

See https://codereview.chromium.org/865253002 for a fix.