605832 - Make OOP crash reporting work on Android

Status: RESOLVED FIXED

(Toolkit :: Crash Reporting, defect)

Description

[(not currently active) Ted Mielczarek]

This probably worked until the dlopen stuff landed, and the changes in bug
603592 don't do anything to make it work. Hopefully we can fix it. Shouldn't block enabling this because we don't have content process crash reporting enabled yet, and I doubt plugin crashes are a big problem on Android right now.

mwu: will plugin-container have the same libraries mapped at the same addresses as fennec? If so, we can probably just cheat and use the data we have in the parent process. If not, we'll have to remote the data somehow.

Comment 1

[Michael Wu [:mwu]]

The plugin container should contain exactly the same libraries at the same addresses. However, I think we can just report the mappings like we do in the parent process case.

Comment 2

[(not currently active) Ted Mielczarek]

Yeah, but the caveat there is that we then have to convey that information across to the parent process somehow. (I guess we could just go whole-hog and make the API that reports them use IPC in child processes.)

Comment 3

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

Ted, I'm assuming this falls on your plate, if not please talk to me.

Comment 4

[Michael Wu [:mwu]]

Attachment: Report mapping to crash reporter in the child process

untested but making this available if people want to try it.

Comment 5

[(not currently active) Ted Mielczarek]

That's not actually going to fix this, since the minidump generation is done in the chrome process, and we're not remoting CrashReporter::AddLibraryMapping, so the library mapping info won't be available when we write the minidump.

Comment 6

[Michael Wu [:mwu]]

(In reply to comment #5)
> That's not actually going to fix this, since the minidump generation is done in
> the chrome process, and we're not remoting CrashReporter::AddLibraryMapping, so
> the library mapping info won't be available when we write the minidump.

Yes, I know. I'm just putting this part of the patch up if you or someone else wants to try remoting that data.

Comment 7

[(not currently active) Ted Mielczarek]

Okay, thanks.

Comment 8

[(not currently active) Ted Mielczarek]

Can't really do anything here until bug 581335 is done.

Comment 9

[Benjamin Smedberg]

We have plugins disabled on android, so we can't start with that?

Comment 10

[(not currently active) Ted Mielczarek]

Oh, good question. I have no idea.

Comment 11

[(not currently active) Ted Mielczarek]

Apparently we do build with plugins enabled for Android, they're just preffed off. I should be able to work with that.

Comment 12

[(not currently active) Ted Mielczarek]

Comment on attachment 487290 [details] [diff] [review]
Report mapping to crash reporter in the child process

I'm not sure if we want this yet, I need to figure out how we're going to remote it first. Thanks for the patch, though, I'll wind up using it somehow.

Comment 13

[(not currently active) Ted Mielczarek]

(In reply to comment #11)
> Apparently we do build with plugins enabled for Android, they're just preffed
> off. I should be able to work with that.

Except the test plugin doesn't currently work on Android, and we crash loading plugins right now (bug 611683). Difficult to do anything useful here at the moment.

Comment 14

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Should be possible to kill -11 the content process then submit the report through about:crashes, right?

Comment 15

[(not currently active) Ted Mielczarek]

Yes, although it won't be a useful report until I fix this bug. Our libraries just show up as /dev/AShmem.

Comment 16

[(not currently active) Ted Mielczarek]

Oh, I see what you meant. Yes, I can use that to test this.

Comment 17

[(not currently active) Ted Mielczarek]

Attachment: Remote AddLibraryMapping from the child process, and use the results for child process minidump generation

This patch adds an IPDL protocol and uses it to remote the library mapping
info from the content process, and uses it for writing child minidumps.
It's an awful lot of complexity, but it works. This depends on a small
Breakpad patch as well, which I'll attach in a moment.

Comment 18

[(not currently active) Ted Mielczarek]

Attachment: small Breakpad fixes to allow writing minidumps from the CrashGenerationServer callback

Turns out CrashGenerationServer didn't have everything it needed to
allow the writeDumps setting to work, and to allow you to write dumps
from the callback function.

Comment 19

[(not currently active) Ted Mielczarek]

Comment on attachment 491240 [details] [diff] [review]
Remote AddLibraryMapping from the child process, and use the results for child process minidump generation

I think I missed a bunch of license headers as well, I'll add those before landing.

Comment 20

[(not currently active) Ted Mielczarek]

I'll give these all a spin through the tryserver as well to make sure I didn't break any other platforms.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 491240 [details] [diff] [review]
Remote AddLibraryMapping from the child process, and use the results for child process minidump generation

>diff --git a/dom/ipc/CrashReporterChild.cpp b/dom/ipc/CrashReporterChild.cpp
>new file mode 100644
>--- /dev/null
>+++ b/dom/ipc/CrashReporterChild.cpp
>@@ -0,0 +1,16 @@
>+#include "CrashReporterChild.h"
>+
>+namespace mozilla {
>+namespace dom {
>+CrashReporterChild::CrashReporterChild()
>+{
>+    MOZ_COUNT_CTOR(CrashReporterChild);
>+}
>+
>+CrashReporterChild::~CrashReporterChild()
>+{
>+    MOZ_COUNT_DTOR(CrashReporterChild);
>+}
>+
>+} // namespace dom
>+} // namespace mozilla

Might as well inline these in the header for now, save a file.

>diff --git a/dom/ipc/PCrashReporter.ipdl b/dom/ipc/PCrashReporter.ipdl
>new file mode 100644
>--- /dev/null
>+++ b/dom/ipc/PCrashReporter.ipdl

I think you need a license header here.

>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp
>--- a/toolkit/crashreporter/nsExceptionHandler.cpp
>+++ b/toolkit/crashreporter/nsExceptionHandler.cpp
>@@ -248,20 +251,21 @@
> // so the embedding will provide a list of shared
> // libraries that are mapped into anonymous mappings.
> typedef struct {
>   std::string name;
>   std::string debug_id;
>   uintptr_t   start_address;
>   size_t      length;
>   size_t      file_offset;
> } mapping_info;
> static std::vector<mapping_info> library_mappings;
>+static std::map<PRUint32,google_breakpad::MappingList> child_library_mappings;

|typedef std::map<PRUint32,google_breakpad::MappingList> MappingMap;|
 or something, then |static MappingMap child_library_mappings;|.
 
>+#if defined(__ANDROID__)
>+  // Do dump generation here since the CrashGenerationServer doesn't
>+  // have access to the library mappings.
>+  std::map<PRUint32,google_breakpad::MappingList>::iterator iter = 
>+    child_library_mappings.find(aClientInfo->pid_);

|MappingMap::const_iterator| plz.

>+  if (iter == child_library_mappings.end())
>+    return;

Would like to at least warn here, this condition indicates fail
somewhere (or NYI).

Comment 22

[(not currently active) Ted Mielczarek]

Attachment: small Breakpad fixes to allow writing minidumps from the CrashGenerationServer callback.

Patch ready for checkin, probably shouldn't land until the blocking bug does.

Comment 23

[(not currently active) Ted Mielczarek]

Attachment: Remote AddLibraryMapping from the child process, and use the results for child process minidump generation.

With comments addressed, ready for checkin.

Comment 24

[(not currently active) Ted Mielczarek]

The content process bits of bug 592768 got punted to bug 581335.

Comment 25

[(not currently active) Ted Mielczarek]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/aa662b68d604
http://hg.mozilla.org/mozilla-central/rev/55b7f3d63b7a

Comment 26

[(not currently active) Ted Mielczarek]

And a bustage fix for good luck:
http://hg.mozilla.org/mozilla-central/rev/871cdedc077b
(broke the Maemo QT builds, which are --enable-ipc but --disable-crashreporter)