1153205 - [e10s] ContentParent should annotate crash reports prior to calling GeneratePairedMinidump

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Jim Mathies [:jimm]]

Per bug 1139860, comment 106.

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#1960

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch

Comment 2

[Jim Mathies [:jimm]]

I tested this locally and everything seems to be working. To test on try I pushed this plus a plug that returns false from ContentParent's LoadPlugin. That should trigger this quite a bit in mochitest.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc4f0fac71bc

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 8590803 [details] [diff] [review]
patch

Review of attachment 8590803 [details] [diff] [review]:
-----------------------------------------------------------------

Looking at how empty that .extra file was I feel like we're still missing some annotation bits that get called in the normal hang/crash case. I started looking into it this morning but got a little lost and don't have that much time to devote to sorting it out right now.

Comment 4

[Jim Mathies [:jimm]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #3)
> Comment on attachment 8590803 [details] [diff] [review]
> patch
> 
> Review of attachment 8590803 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looking at how empty that .extra file was I feel like we're still missing
> some annotation bits that get called in the normal hang/crash case. I
> started looking into it this morning but got a little lost and don't have
> that much time to devote to sorting it out right now.

I'll file a follow up on this. Eventually someone will need this data and fix it. :)

Comment 5

[Jim Mathies [:jimm]]

Huh, locally this patch works, on try it crashes for an unknown reason in breakpad code.

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-fc4f0fac71bc/try-win32/try_win7-ix_test-mochitest-e10s-browser-chrome-1-bm109-tests1-windows-build789.txt.gz

Comment 6

[Jim Mathies [:jimm]]

Attachment: wip

Comment 7

[Jim Mathies [:jimm]]

Attachment: wip

Comment 8

[Jim Mathies [:jimm]]

Attachment: tmp.txt

Comment 9

[Jim Mathies [:jimm]]

Attachment: wip

Comment 10

[Jim Mathies [:jimm]]

Using the latest wip, good meta data here. Going to push to try again and trigger a crash, see what we get.

https://crash-stats.mozilla.com/report/index/5b1fe74c-67f6-4710-92f5-852742150410

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 12

[Jim Mathies [:jimm]]

Still getting a crash in google_breakpad::ExceptionHandler::WriteMinidump() in a really weird place - 

http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc#741

Comment 13

[Jim Mathies [:jimm]]

Hmm, this top frame was found through "given as instruction pointer in context".. I wonder if that can be inaccurate. Working with a debug build might be better here.

Comment 14

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #13)
> Hmm, this top frame was found through "given as instruction pointer in
> context".. I wonder if that can be inaccurate. Working with a debug build
> might be better here.

reliable as in, sits at the very bottom of the "trust list" - 

http://mxr.mozilla.org/mozilla-central/source/toolkit/crashreporter/google-breakpad/src/google_breakpad/processor/stack_frame.h#53

Comment 15

[(not currently active) Ted Mielczarek]

Yeah, that's accurate. What did your try log look like?

Comment 16

[Jim Mathies [:jimm]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #15)
> Yeah, that's accurate. What did your try log look like?

It crashed in the same place - 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2e0187da8ef

I'm wondering if this has something to do with that WaitForSingleObject call ExceptionHandler makes when it yields to its worker thread. Maybe something is running out of band, through some sort of callback or something. Just a guess.

The big question I'm left with is how to debug it, since I can't reproduce locally.

Comment 17

[(not currently active) Ted Mielczarek]

So this isn't crashing. Note:
 PROCESS-CRASH | Main app process exited normally | application crashed [@ ntdll.dll + 0x470b4] 

This is just what a minidump from the browser process looks like when you call CreatePairedMinidumps. The problem is that something is calling this code path and either not calling SimpleTest.expectChildProcessCrash(), or something in your patch mucks up how expectChildProcessCrash cleans up the expected dumps.

Comment 18

[Jim Mathies [:jimm]]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> So this isn't crashing. Note:
>  PROCESS-CRASH | Main app process exited normally | application crashed [@
> ntdll.dll + 0x470b4] 
> 
> This is just what a minidump from the browser process looks like when you
> call CreatePairedMinidumps. The problem is that something is calling this
> code path and either not calling SimpleTest.expectChildProcessCrash(), or
> something in your patch mucks up how expectChildProcessCrash cleans up the
> expected dumps.

That's good to hear. Maybe I'm just running into some random test failures here.

Comment 19

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #18)
> (In reply to Ted Mielczarek [:ted.mielczarek] from comment #17)
> > So this isn't crashing. Note:
> >  PROCESS-CRASH | Main app process exited normally | application crashed [@
> > ntdll.dll + 0x470b4] 
> > 
> > This is just what a minidump from the browser process looks like when you
> > call CreatePairedMinidumps. The problem is that something is calling this
> > code path and either not calling SimpleTest.expectChildProcessCrash(), or
> > something in your patch mucks up how expectChildProcessCrash cleans up the
> > expected dumps.
> 
> That's good to hear. Maybe I'm just running into some random test failures
> here.

Ha! Forgot I was doing this on purpose so I could see the crash reports!

https://hg.mozilla.org/try/rev/4fcacd5134ba#l1.12

Comment 20

[Jim Mathies [:jimm]]

 https://treeherder.mozilla.org/#/jobs?repo=try&revision=7398018beb81

Comment 21

[Jim Mathies [:jimm]]

Attachment: patch v.3

Comment 22

[(not currently active) Ted Mielczarek]

Comment on attachment 8591928 [details] [diff] [review]
patch v.3

Review of attachment 8591928 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/ipc/ContentParent.cpp
@@ +3309,5 @@
>  
>  #if defined(MOZ_CRASHREPORTER) && !defined(MOZ_B2G)
> +    // We're about to kill the child process associated with this content.
> +    // Something has gone wrong to get us here, so we generate a minidump
> +    // of the parent and child for submittal to the crash server.

The word you're looking for is "submission". :)

::: dom/ipc/CrashReporterParent.cpp
@@ +145,5 @@
> +        // If we don't have a dump id yet, we haven't called
> +        // CrashReporter::CreatePairedMinidumps. Apply any annotations using the
> +        // generic crash reporter api. When we call CreatePairedMinidumps
> +        // these annotations will get written out to an extra file associated
> +        // with our report.

I'm not sure this is right. These annotations are from the child process, but calling AnnotateCrashReport sticks them into the chrome process' list of annotations. That means that if we later crashed the chrome process we'd get these annotations in the crash report which would be confusing.

@@ +180,5 @@
> +
> +    if (NS_IsMainThread()) {
> +        // Inline, this is the main thread. Get this done.
> +        NotifyCrashService();
> +        return;

It'd be nice if this pattern was easier to write with a one-liner that did the right thing for main-thread-or-not.

Comment 23

[Jim Mathies [:jimm]]

Attachment: patch v.4

Comment 24

[Jim Mathies [:jimm]]

Attachment: patch v.4

Comment 25

[Jim Mathies [:jimm]]

Comment on attachment 8602150 [details] [diff] [review]
patch v.4

this adds the all in one call, and basically just bails if somebody does something in the wrong order.

Comment 26

[(not currently active) Ted Mielczarek]

Comment on attachment 8602150 [details] [diff] [review]
patch v.4

Review of attachment 8602150 [details] [diff] [review]:
-----------------------------------------------------------------

This is all getting very complicated for my brain to handle, but I think I follow.

Comment 27

[Milan Sreckovic [:milan] (needinfo for best results)]

(In reply to Ted Mielczarek [:ted.mielczarek] from comment #22)
> ...
> 
> ::: dom/ipc/CrashReporterParent.cpp
> @@ +145,5 @@
> > +        // If we don't have a dump id yet, we haven't called
> > +        // CrashReporter::CreatePairedMinidumps. Apply any annotations using the
> > +        // generic crash reporter api. When we call CreatePairedMinidumps
> > +        // these annotations will get written out to an extra file associated
> > +        // with our report.
> 
> I'm not sure this is right. These annotations are from the child process,
> but calling AnnotateCrashReport sticks them into the chrome process' list of
> annotations. That means that if we later crashed the chrome process we'd get
> these annotations in the crash report which would be confusing.

I'd actually want to get the annotations from all processes in the crash report, or did I misunderstand the objection?  I don't care if it's separated somehow, as long as the annotations from child processes are available in the crash report even if it was only the chrome process that crashed.

Comment 28

[(not currently active) Ted Mielczarek]

(In reply to Milan Sreckovic [:milan] from comment #27)
> I'd actually want to get the annotations from all processes in the crash
> report, or did I misunderstand the objection?  I don't care if it's
> separated somehow, as long as the annotations from child processes are
> available in the crash report even if it was only the chrome process that
> crashed.

That's not how it works, and I don't see how you could make that workable in the general sense. If we have >1 content process and they all AnnotateCrashReport("URL", <whatever>), you want those annotations to be per-process.

Currently the main process has a set of annotations, and each child process has its own set of annotations. When the main process crashes it uses its set. When a child process crashes it uses its own set along with a subset of the parent process annotations (specifically everything except the ones in the blacklist here: https://hg.mozilla.org/mozilla-central/annotate/baa9c64fea6f/toolkit/crashreporter/nsExceptionHandler.cpp#l308 ).

Comment 29

[Milan Sreckovic [:milan] (needinfo for best results)]

It may be how it works, but that makes the crash report less useful, as it may have been the child process that caused the parent process to crash, and we lose valuable information if we don't see the details of the child process annotations.

Child process crashing picking up a subset of parent annotations sounds good - why can't parent process crashing also pick up annotations from all the child processes?

Comment 30

[Milan Sreckovic [:milan] (needinfo for best results)]

And I understand that the child process is not "crashing" when the parent process crashes, but I'm arguing that there is enough information in the child process annotations that I'd want them picked up even though the child process isn't crashing, as such.

Comment 31

[(not currently active) Ted Mielczarek]

We have multiple child processes. How should we sort out annotations from each? Even in a single content process e10s case we can have that content process + multiple plugin (and GMP) processes.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ce6dbefadccd

Comment 33

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ce6dbefadccd