Fix skydb --gdb to always have symbols

Fix skydb --gdb to always have symbols

This replaces my previous --gdb work:
https://codereview.chromium.org/848013004/

And obsoletes my build-id based attempt:
https://codereview.chromium.org/788903011

Context:
mojo_shell downloads arbitrary binaries from
urls copying them to temp files before calling
dlopen.  Because the names it used were random
this broke gdb, pprof, etc. tools which wanted
to make address -> symbol translations based
on the library load path.

The major thing this change does is move away
from the previous method of watching the logs
of mojo_shell for 'Caching %url as %file...'
messages or the /tmp/mojo_shell.%pid.map
file to having mojo_shell use a priori knowable
names for all of its library loads.  Thus
we can similarly build a directory of correctly
named symboled binaries corresponding to the
expected load names of libraries.

This change does this in 3 pieces:

1.  Introduces the concept of 'app ids' (which
are currently just the md5 of the distributed app
binary) and teaches dynamic_application_loader to
rename all apps to APP_ID.mojo before loading them.
This has the nice side-effect of always loading
an app with a dlopen/library name which is both
unique to the application as well as predictable.

2.  Re-writes the mojo_cache_linker script to
no longer watch stdin/adb logcat for 'caching...'
messages and instead build a links directory
based on pre-determined app_ids (md5s) linking
back to the symboled binaries.

3.  Remove a bunch of the former mojo_cache_linker
calling code which is no longer needed now that
the library_names of loaded application .so's are
predictable before launching mojo_shell

I'm happy to make app_ids fancier, originally I
was going to use ELF's build-id's directly:
https://codereview.chromium.org/788903011
but unfortunately gdbserver does not know
how to do a build-id lookup on the serverside:
https://groups.google.com/a/google.com/forum/#!topic/gdb-discuss/Fd0R-gFaqXk

R=aa@chromium.org, abarth@chromium.org

Committed: https://chromium.googlesource.com/external/mojo/+/fddf1753e008e13e990663b53e1864d4c027464a

[Aaron Boodman, 2015-01-23 21:56:41]

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:7: #include <fstream>
We don't typically use stl's stream support in Chromium. Not sure if it's a
style thing tho...

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:287: static void
RecordCacheToURLMapping(const base::FilePath& path,
We do not need this anymore, right?

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:309: // Using MD5 because base/md5.h has an
incremental API, any crypto would work.
Yeah it would be better to use sha256 I think, if only to prevent developers
from having to think about what collision would mean. I'm surprised we don't
have other incremental hashes lying around, but if we don't, I think this can be
a TODO.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:339: // The copy has completed move to a
predictable location $TMP/$APP_ID.mojo.
I don't think this comment is true .. we haven't moved to APP_ID yet, right?
That is below.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:340: if (copy_success) {
If the copy failed, we're SOL. Should not do anymore work and send false to
other-callback.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:345: std::string map_name =
base::StringPrintf("%s.mojo", app_id.data());
app_id.c_str()

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:345: std::string map_name =
base::StringPrintf("%s.mojo", app_id.data());
I think 'mapped_name' might be a better name for this variable (current name
makes me think it is referring to the map file itself).

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:367: // We don't use the created file, just
want the directory and random name.
I think this comment is incorrect ... we do use the temp file now.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:371: base::Unretained(this), callback));
Add a comment explaining why unretained is ok?

[eseidel, 2015-01-23 22:12:21]

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:7: #include <fstream>
On 2015/01/23 21:56:40, Aaron Boodman wrote:
> We don't typically use stl's stream support in Chromium. Not sure if it's a
> style thing tho...

Yeah, ComputeAppId is lifted directly from android/md5sum.cc which does use
fstream.  I'm happy to change it to something else?

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:309: // Using MD5 because base/md5.h has an
incremental API, any crypto would work.
On 2015/01/23 21:56:41, Aaron Boodman wrote:
> Yeah it would be better to use sha256 I think, if only to prevent developers
> from having to think about what collision would mean. I'm surprised we don't
> have other incremental hashes lying around, but if we don't, I think this can
be
> a TODO.

Agreed.  I'll change it to say TODO, sadly md5 is our only incremental api.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:339: // The copy has completed move to a
predictable location $TMP/$APP_ID.mojo.
On 2015/01/23 21:56:40, Aaron Boodman wrote:
> I don't think this comment is true .. we haven't moved to APP_ID yet, right?
> That is below.

Yeah, sorry I was trying to explain what the method was supposed to do.  There
should be a comma before 'move'.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:340: if (copy_success) {
On 2015/01/23 21:56:41, Aaron Boodman wrote:
> If the copy failed, we're SOL. Should not do anymore work and send false to
> other-callback.

We already send false if the copy failed (by skipping the if block), I also
fixed it to send false if the move fails.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:345: std::string map_name =
base::StringPrintf("%s.mojo", app_id.data());
On 2015/01/23 21:56:40, Aaron Boodman wrote:
> app_id.c_str()

Done.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:367: // We don't use the created file, just
want the directory and random name.
On 2015/01/23 21:56:41, Aaron Boodman wrote:
> I think this comment is incorrect ... we do use the temp file now.

Well, CreateTempFile creates and empty file and then immediately closes it.  We
overwrite the file by moving a new file over it, but I agree the comment isn't
very useful now.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:371: base::Unretained(this), callback));
On 2015/01/23 21:56:40, Aaron Boodman wrote:
> Add a comment explaining why unretained is ok?

I'm not sure unretained is OK.  If the copy for some reason takes longer than
DynamicApplicationLoader lives then I could imagine us crashing.  That would
only happen during an unexpected termination, but I could see it happening.  We
probably want a weak ptr to DynamicApplicationLoader instead.

[Aaron Boodman, 2015-01-23 22:29:00]

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
File shell/dynamic_application_loader.cc (right):

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:7: #include <fstream>
On 2015/01/23 22:12:21, eseidel wrote:
> On 2015/01/23 21:56:40, Aaron Boodman wrote:
> > We don't typically use stl's stream support in Chromium. Not sure if it's a
> > style thing tho...
> 
> Yeah, ComputeAppId is lifted directly from android/md5sum.cc which does use
> fstream.  I'm happy to change it to something else?

How about base::File::Read() and friends?

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:340: if (copy_success) {
On 2015/01/23 22:12:21, eseidel wrote:
> On 2015/01/23 21:56:41, Aaron Boodman wrote:
> > If the copy failed, we're SOL. Should not do anymore work and send false to
> > other-callback.
> 
> We already send false if the copy failed (by skipping the if block), I also
> fixed it to send false if the move fails.

I mean we should not call RecordCacheToURLMapping() or the line after that. If
|copy_success| is false, we should early return and call the next callback
immediately with false.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:367: // We don't use the created file, just
want the directory and random name.
On 2015/01/23 22:12:21, eseidel wrote:
> On 2015/01/23 21:56:41, Aaron Boodman wrote:
> > I think this comment is incorrect ... we do use the temp file now.
> 
> Well, CreateTempFile creates and empty file and then immediately closes it. 
We
> overwrite the file by moving a new file over it, but I agree the comment isn't
> very useful now.

I see. Can you just delete it then.

https://codereview.chromium.org/866383004/diff/1/shell/dynamic_application_lo...
shell/dynamic_application_loader.cc:371: base::Unretained(this), callback));
On 2015/01/23 22:12:21, eseidel wrote:
> On 2015/01/23 21:56:40, Aaron Boodman wrote:
> > Add a comment explaining why unretained is ok?
> 
> I'm not sure unretained is OK.  If the copy for some reason takes longer than
> DynamicApplicationLoader lives then I could imagine us crashing.  That would
> only happen during an unexpected termination, but I could see it happening. 
We
> probably want a weak ptr to DynamicApplicationLoader instead.

It actually looks to me like NetworkLoader cannot be deleted while this
operation is outstanding (modulo the entire shell shutting down), but
StartNetworkRequest is using a weakptr back to NetworkLoader, so I think you
should do the same here for consistency.

[eseidel, 2015-01-23 22:53:05]

Making RenameToAppId (now CopyCompleted) fail-fast to the callback without using
goto: was kinda awkward, but I did it.  Probably could do it cleaner.

I ended up adding back the /tmp/mojo_shell.pid.maps file writing since that's
used by pprof at the moment and fixing pprof to not depend on that will be a bit
tricky.  I think we may actually want mojo_shell.pid.maps anyway since that's
our only source of URL -> /tmp/app_id.mojo mapping, even if gdb can get by find
with just having the app_id.mojo name, I could imagine things want to know where
this app_id.mojo came from on the interblags.

[eseidel, 2015-01-23 22:53:18]

PTAL, I believe I've addressed all your comments.

[Aaron Boodman, 2015-01-23 22:56:01]

lgtm

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

https://codereview.chromium.org/866383004/diff/60001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:309: // AppIds should be be both predictable
and unique, but any crypto would work.
nit: s/crypto/hash.

[eseidel, 2015-01-23 23:18:25]

Committed patchset #5 (id:80001) manually as
fddf1753e008e13e990663b53e1864d4c027464a (presubmit successful).

[qsr, 2015-01-26 09:47:51]

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

[qsr, 2015-01-26 09:47:52]

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

https://codereview.chromium.org/866383004/diff/80001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:312: static bool ComputeAppId(const
base::FilePath& path,
Do you need to read the full file for this? You should be able to only use a
prefix. Also, when we read this from the network, we already need to read the
full file from the datapipe to a real file, can we compute the hash at the same
time?

https://codereview.chromium.org/866383004/diff/80001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:340: base::FilePath* new_path) {
I think you can have a race here when we go to multiprocess. Multiple process
will have to run the same application (at least for content handler, but you can
also imagine someone saving the same bytes in 2 different URLs). Also already
today if someone save the same bytes in 2 different URLs, we will have the exact
same bytes run twice. With this change, there is a race.

[qsr, 2015-01-26 09:49:10]

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

https://codereview.chromium.org/866383004/diff/80001/shell/dynamic_applicatio...
shell/dynamic_application_loader.cc:297: base::FilePath map_path =
temp_dir.Append(map_name);
Now that you use md5, do you still need this? This file is currently leaking.