679238 - [OOPP] Plugin child aborts should trigger parent side stack generation for crash reporter

Status: RESOLVED INCOMPLETE

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Bug 677711 introduces child side abort functionality when the child detects a hung parent. Currently we get a child side stack submitted but not the parent's. Getting this submitted will likely involve the io thread since the ui thread in the parent is not responding.

Comment 1

[Jim Mathies [:jimm]]

Attachment: patch rev 1

Posting the current rev of this work as a single patch.

This is working pretty well and socorro seems happy with the extra data as it's matching the two reports up.

https://crash-stats.mozilla.com/report/index/bp-5a9b4b6f-98ad-4740-9474-e2d752110909
https://crash-stats.mozilla.com/report/index/bp-5a9b4b6f-98ad-4740-9474-e2d752110909

Unfortunately I did have to add a bit of code and make a few changes to breakpad. I'm currently looking for ways to pull this out to avoid it, but I thought I'd post what I have for now. Either way I plan on splitting up what I end up with into smaller patches and kicking off some reviews next week. I'd really like to get this landed in some form or another pretty soon.

Comment 2

[Jim Mathies [:jimm]]

Attachment: patch rev 1

- comment cleanup

Comment 3

[Jim Mathies [:jimm]]

Attachment: breakpad v.1

Comment 4

[Jim Mathies [:jimm]]

Attachment: ex handler v.1

Comment 5

[Jim Mathies [:jimm]]

Attachment: plugins v.1

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 560021 [details] [diff] [review]
breakpad v.1

Breakpad changes: this adds a new event the child can trigger to generate a minidump on the parent side of the connection.

Comment 7

[Jim Mathies [:jimm]]

Comment on attachment 560022 [details] [diff] [review]
ex handler v.1

exception handler callback and xre changes to track and take parent side hangs the child requests.

Comment 8

[Jim Mathies [:jimm]]

Comment on attachment 560023 [details] [diff] [review]
plugins v.1

plugin changes that tag the two minidumps into a pair for crash reporting.

Comment 9

[Benjamin Smedberg]

Comment on attachment 560022 [details] [diff] [review]
ex handler v.1

Can I wait to review these until Ted approves the breakpad change?

Comment 10

[(not currently active) Ted Mielczarek]

Comment on attachment 560021 [details] [diff] [review]
breakpad v.1

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

Sorry, I kept looking at this patch and not having time to get through all of it. This looks good. The only thing I'd like to see is a unit test for the added functionality. Unfortunately our Breakpad test coverage for CrashGenerationServer is pretty poor, so I guess we can let that slide for now. I'll land this upstream for you.

Comment 11

[Jim Mathies [:jimm]]

(In reply to Ted Mielczarek [:ted, :luser] from comment #10)
> Comment on attachment 560021 [details] [diff] [review] [diff] [details] [review]
> breakpad v.1
> 
> Review of attachment 560021 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> Sorry, I kept looking at this patch and not having time to get through all
> of it. This looks good. The only thing I'd like to see is a unit test for
> the added functionality. Unfortunately our Breakpad test coverage for
> CrashGenerationServer is pretty poor, so I guess we can let that slide for
> now. I'll land this upstream for you.

Thanks. Note, this changes the signature of OnChildProcessDumpRequested in the exception handler code, so the ex handler v.1 patch needs to land with it on mc. Does "landing it upstream" include an mc landing, or are you referring to google's code repo, or something else?

Comment 12

[Jim Mathies [:jimm]]

Comment on attachment 560023 [details] [diff] [review]
plugins v.1

spreading these around. bent, would you mind picking up the review on this one?

Comment 13

[(not currently active) Ted Mielczarek]

Landed upstream:
http://code.google.com/p/google-breakpad/source/detail?r=883

I had to fix a few places because we still have a local patch that hasn't been upstreamed, and also I had to fix a few of the Breakpad unittests to match the new method signatures.

Comment 14

[(not currently active) Ted Mielczarek]

By "upstream" I just meant "Breakpad SVN".

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 560023 [details] [diff] [review]
plugins v.1

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

::: dom/plugins/ipc/PluginModuleParent.cpp
@@ +300,5 @@
>  {
>      switch (why) {
>      case AbnormalShutdown: {
>  #ifdef MOZ_CRASHREPORTER
> +        nsCOMPtr<nsILocalFile> browserDump;

Nit: Since this isn't used outside the |if (TakeMinidump(getter_AddRefs(pluginDump))| block I'd move it inside.

@@ +305,2 @@
>          nsCOMPtr<nsILocalFile> pluginDump;
> +        if (TakeMinidump(getter_AddRefs(pluginDump)) && // in pending

Nit: The "in pending" comment here and below is not very descriptive...

@@ +309,5 @@
> +            // requested before purposefully aborting due to a parent side
> +            // hang.
> +            if (XRE_TakeMinidumpForParent(getter_AddRefs(browserDump)) && // in pending
> +                CrashReporter::GetIDFromMinidump(browserDump, mBrowserDumpID) &&
> +                CrashReporter::CreatePairedHangId(&mHangID)) {

The fallback is fine, I guess, but as written we won't be notified of failures in any of these methods, do you think they should be logged?

Comment 16

[Benjamin Smedberg]

Comment on attachment 560022 [details] [diff] [review]
ex handler v.1

>diff --git a/toolkit/crashreporter/nsExceptionHandler.cpp b/toolkit/crashreporter/nsExceptionHandler.cpp

>+#if defined(XP_WIN)
>+class CrashDumpSetupWin : public nsIRunnable {

nit, please put opening brace of classes on the next line

Comment 17

[(not currently active) Ted Mielczarek]

This got backed out upstream for suspected bustage (presumably in Chrome):
http://code.google.com/p/google-breakpad/source/detail?r=891

Comment 18

[Jim Mathies [:jimm]]

Attachment: plugins v.2

Updated to all the code changes that were made to plugin crash reporting. Also working on a new breakpad patch that won't change the callback signatures, which should address any problems google had.

Comment 19

[Benjamin Smedberg]

I don't think this is going to be especially valuable.