603592 - Sort out interaction of Android custom library loader and Breakpad

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

The custom library loader mwu wrote in bug 588607 is going to screw up a critical piece of Breakpad. Currently, the Breakpad code reads /proc/<pid>/maps to get a list of modules loaded in the process. For each module, it also mmaps the file and calculates a File ID by hashing the first page of the text section, which is what we use to map modules back to debug symbols on the server side. Since mwu's loader maps modules directly out of the apk into anonymous maps, this is going to fall over. We'll need to figure out a workaround here.

Since all we really need is the filename + file ID, maybe we can just store those somewhere when the loader does its thing. The File ID should be cheap to calculate, it's just a cheesy XOR hash of the first page of the text section:
http://code.google.com/p/google-breakpad/source/browse/trunk/src/common/linux/file_id.cc#140

I don't know how we'll retrieve that data from Breakpad. We clone() a new process to do the actual dumping, and I don't think we share memory with the dying process. The Linux dumping code gets all of its data by reading things from /proc/<pid>, and using ptrace to poke the process.

Comment 1

[Michael Wu [:mwu]]

Attachment: Report mapping to crash reporter

try this.

Comment 2

[Michael Wu [:mwu]]

Attachment: Report mapping to crash reporter, v2

now with more APKOpen.h

Comment 3

[(not currently active) Ted Mielczarek]

Ok, I had to track down one tiny bug this morning but I got it working:
http://crash-stats.mozilla.com/report/index/300fc519-4fa8-46c8-aa6b-e25202101020

That's from a build with mwu's dlopen changes + the patches on this bug + some other patches I will attach in a minute.

Comment 4

[(not currently active) Ted Mielczarek]

Attachment: make a file_id host binary that can spit out a Breakpad file id

Comment 5

[(not currently active) Ted Mielczarek]

Attachment: add a breakpad api to add info about known modules

Comment 6

[(not currently active) Ted Mielczarek]

Attachment: Add an API for the Android embedding to use to provide info about shared libraries mapped into anonymous mappings

Comment 7

[(not currently active) Ted Mielczarek]

Comment on attachment 484409 [details] [diff] [review]
Report mapping to crash reporter, v2

You're probably right that we should just pass this whole array of info to the CrashReporter API at once, but I don't think it's really bad as it is, just redundant.

>+extern "C" void
>+report_mapping(char *name, void *base, uint32_t len, uint32_t offset)
>+{
>+  if (!file_ids)
>+    return;
>+
>+  struct mapping_info *info = &lib_mapping[mapping_count++];
>+  info->name = strdup(name);
>+  info->base = (uintptr_t)base;
>+  info->len = len;
>+  info->offset = offset;
>+
>+  char * entry = strstr(file_ids, name);
>+  if (entry)
>+    info->file_id = strndup(entry + strlen(name) + 1, 32);

You're just leaking the strdup'ed strings, aren't you? Probably doesn't matter since it's a small fixed set.

>+#ifdef MOZ_CRASHREPORTER
>+  lib_mapping = (struct mapping_info *)calloc(32, sizeof(*lib_mapping));

This is a hardcoded limit, eh? Can you at least make it a named constant, and sanity-check mapping_count against it in report_mapping?

>diff --git a/toolkit/mozapps/installer/packager.mk b/toolkit/mozapps/installer/packager.mk
>--- a/toolkit/mozapps/installer/packager.mk
>+++ b/toolkit/mozapps/installer/packager.mk
>@@ -170,6 +170,7 @@ DIST_FILES = \
>   modules \
>   res \
>   lib \
>+  lib.id \
>   extensions \
>   application.ini \
>   platform.ini \
>@@ -200,6 +201,11 @@ INNER_MAKE_PACKAGE	= \
>     mkdir -p lib/armeabi && \
>     cp lib*.so lib && \
>     mv lib/libmozutils.so lib/armeabi && \
>+    rm -f lib.id && \
>+    for SOMELIB in lib/*.so ; \
>+    do \
>+      printf "`basename $$SOMELIB`:`$(_ABS_DIST)/host/bin/file_id $$SOMELIB`\n" >> lib.id ; \
>+    done && \

God, I still hate all of this. I could suggest some slight cleanup here, but it's lipstick on a pig.

Looks ok in general.

Comment 8

[(not currently active) Ted Mielczarek]

Comment on attachment 484679 [details] [diff] [review]
make a file_id host binary that can spit out a Breakpad file id

The Breakpad changes in this patch are just upstream changes that we hadn't taken locally yet.

Comment 9

[(not currently active) Ted Mielczarek]

Comment on attachment 484680 [details] [diff] [review]
add a breakpad api to add info about known modules

I'll get upstream review on this as well, but if you would like to give this a once-over we can get it landed in m-c to get all this Android crash reporting working. There are unit tests for both MinidumpWriter and ExceptionHandler (although the patch hunks may not all have made it into this patch) and they all pass. You can see the full patch against upstream here:
http://hg.mozilla.org/users/tmielczarek_mozilla.com/breakpad-mq/file/68f964cf9f0b/module-info-api

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 484681 [details] [diff] [review]
Add an API for the Android embedding to use to provide info about shared libraries mapped into anonymous mappings

Bleh, just realized that there are some hunks here that belong in the other patch. This is because we have a couple of non-upstreamed patches in m-c. I need to get that fixed.

The TODO in those extraneous hunks indicates that plugin hang reports aren't going to work correctly. I'll file a followup on that. In fact, I'm not sure out-of-process crashes will work at all at the moment, but since content process crash reporting isn't currently hooked up I'll file that as a followup as well.

The FileIDtoGUID bit is kind of dumb, I realized after designing the API that the minidump code actually wants file IDs in a binary GUID format. It's probably more of a PITA for your code to pass things around in that format, and the conversion is probably quick, so it may not be worth bothering with.

Comment 11

[Michael Wu [:mwu]]

Comment on attachment 484679 [details] [diff] [review]
make a file_id host binary that can spit out a Breakpad file id

Do we need boilerplate on file_id.cc?

Comment 12

[Michael Wu [:mwu]]

Comment on attachment 484680 [details] [diff] [review]
add a breakpad api to add info about known modules

Uh I don't know this code too well but it looks mostly ok to me. Guess we'll sort the review out for real upstream.

>@@ -510,9 +515,26 @@
>                                       child,
>                                       child_blamed_thread))
>       return false;
> 
>   return callback ? callback(eh.dump_path_c_, eh.next_minidump_id_c_,
>                              callback_context, true) : true;
> }
> 
>+void ExceptionHandler::AddMappingInfo(const std::string& name,
>+                                      const u_int8_t identifier[sizeof(MDGUID)],
>+                                       uintptr_t start_address,
>+                                       size_t mapping_size,
>+                                       size_t file_offset) {
indentation is off.

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to comment #11)
> Do we need boilerplate on file_id.cc?

Uh yeah, I forgot that, I'll fix it.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 484681 [details] [diff] [review]
Add an API for the Android embedding to use to provide info about shared libraries mapped into anonymous mappings

Looks fine other than not needing to maintain another list but we can sort that out later.

Comment 15

[(not currently active) Ted Mielczarek]

Attachment: Add an API for the Android embedding to use to provide info about shared libraries mapped into anonymous mappings.

Comment 16

[(not currently active) Ted Mielczarek]

Attachment: add a breakpad api to add info about known modules.

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: make a file_id host binary that can spit out a Breakpad file id.

Comment 18

[Michael Wu [:mwu]]

Attachment: Report mapping to crash reporter, v3 (hg patch ready)

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/f2253edf5fc4
http://hg.mozilla.org/mozilla-central/rev/b799dd554e12
http://hg.mozilla.org/mozilla-central/rev/781d0b237470 and
http://hg.mozilla.org/mozilla-central/rev/8fc42e3e185f

Comment 20

[(not currently active) Ted Mielczarek]

I F'ed up one of these patches, additional patch in a sec.

Comment 21

[(not currently active) Ted Mielczarek]

Attachment: Swap endianness on GUIDs when de-stringifying them

This works.

Comment 22

[Michael Wu [:mwu]]

Comment on attachment 485344 [details] [diff] [review]
Swap endianness on GUIDs when de-stringifying them

Looks fine as long as the stringifying code only swaps on little endian systems. Might want to make it more explicit that the guid is big endian.

Comment 23

[(not currently active) Ted Mielczarek]

Pushed that last patch to m-c:
http://hg.mozilla.org/mozilla-central/rev/824f0cefa25d