Sort out interaction of Android custom library loader and Breakpad

RESOLVED FIXED in mozilla2.0b7

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: ted, Assigned: mwu)

Tracking

(Blocks 1 bug)

Trunk
mozilla2.0b7
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(fennec2.0b2+)

Details

Attachments

(5 attachments, 5 obsolete attachments)

6.79 KB, patch
Details | Diff | Splinter Review
29.51 KB, patch
Details | Diff | Splinter Review
15.20 KB, patch
Details | Diff | Splinter Review
9.64 KB, patch
Details | Diff | Splinter Review
1.16 KB, patch
mwu
: review+
Details | Diff | Splinter Review
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.
tracking-fennec: --- → 2.0b2+
Assignee: nobody → mwu
(Assignee)

Comment 1

9 years ago
try this.
Attachment #484168 - Flags: feedback?(ted.mielczarek)
(Assignee)

Comment 2

9 years ago
now with more APKOpen.h
Attachment #484168 - Attachment is obsolete: true
Attachment #484409 - Flags: feedback?(ted.mielczarek)
Attachment #484168 - Flags: feedback?(ted.mielczarek)
Whiteboard: patch by eod [10/19]
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 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.
Attachment #484409 - Flags: feedback?(ted.mielczarek) → feedback+
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.
Attachment #484679 - Flags: review?(mwu)
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
Attachment #484680 - Flags: review?(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

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.
Attachment #484681 - Flags: review?(mwu)
Blocks: 605831
Blocks: 605832
(Assignee)

Updated

9 years ago
Attachment #484409 - Flags: review?(tglek)
(Assignee)

Comment 11

9 years ago
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?
Attachment #484679 - Flags: review?(mwu) → review+
(Assignee)

Comment 12

9 years ago
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.
Attachment #484680 - Flags: review?(mwu) → review+
(In reply to comment #11)
> Do we need boilerplate on file_id.cc?

Uh yeah, I forgot that, I'll fix it.
(Assignee)

Comment 14

9 years ago
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.
Attachment #484681 - Flags: review?(mwu) → review+
Attachment #484681 - Attachment is obsolete: true
Attachment #484680 - Attachment is obsolete: true
Attachment #484679 - Attachment is obsolete: true
(Assignee)

Comment 18

9 years ago
Attachment #484409 - Attachment is obsolete: true
Attachment #484409 - Flags: review?(tglek)
Whiteboard: patch by eod [10/19]
Blocks: 604725
I F'ed up one of these patches, additional patch in a sec.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 22

9 years ago
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.
Attachment #485344 - Flags: review?(mwu) → review+
Pushed that last patch to m-c:
http://hg.mozilla.org/mozilla-central/rev/824f0cefa25d
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
No longer blocks: 605832
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Depends on: 610970
You need to log in before you can comment on or make changes to this bug.